Cleanup `ArrowNativeType` (#1918) #2793

tustvold · 2022-09-27T20:05:41Z

Which issue does this PR close?

Closes #1918

Rationale for this change

The ArrowNativeType has been a pet peeve of mine for a while, this cleans it up a bit:

Deprecates some methods that don't seem to serve a purpose
Remove the use of num as it is unnecessary
Adds an unchecked conversion to usize

What changes are included in this PR?

Are there any user-facing changes?

ArrowNativeType::to_usize and ArrowNativeType::to_isize take values by value, instead of reference

arrow-buffer/src/native.rs

tustvold · 2022-09-27T20:06:29Z

arrow-buffer/src/native.rs

+        impl ArrowNativeType for $t {
+            #[inline]
+            fn from_usize(v: usize) -> Option<Self> {
+                v.try_into().ok()


TryInto behaves the same for integer types as num::FromPrimitive

tustvold · 2022-09-27T20:06:45Z

arrow-buffer/src/util/bit_util.rs

@@ -102,7 +101,7 @@ pub unsafe fn unset_bit_raw(data: *mut u8, i: usize) {
 pub fn ceil(value: usize, divisor: usize) -> usize {
    // Rewrite as `value.div_ceil(&divisor)` after
    // https://github.com/rust-lang/rust/issues/88581 is merged.
-    Integer::div_ceil(&value, &divisor)
+    value / divisor + (0 != value % divisor) as usize


This is copied wholesale from Integer::div_ceil

Remove num dependency from arrow-buffer Deprecate unnecessary methods

tustvold · 2022-09-27T21:15:01Z

arrow-buffer/Cargo.toml

@@ -38,7 +38,6 @@ path = "src/lib.rs"
 bench = false

 [dependencies]
-num = { version = "0.4", default-features = false, features = ["std"] }


This is still a dependency elsewhere, but every little helps 😅

tustvold · 2022-09-27T21:15:39Z

arrow-array/src/array/dictionary_array.rs

@@ -308,20 +308,13 @@ impl<K: ArrowPrimitiveType> DictionaryArray<K> {

    /// Return an iterator over the keys (indexes into the dictionary)
    pub fn keys_iter(&self) -> impl Iterator<Item = Option<usize>> + '_ {
-        self.keys


These checks make no sense as we have already validated the dictionary offsets are non-negative

tustvold · 2022-09-27T21:16:20Z

arrow-buffer/src/native.rs

+    /// Convert to usize according to the [`as`] operator
+    ///
+    /// [`as`]: https://doc.rust-lang.org/reference/expressions/operator-expr.html#numeric-cast
+    fn as_usize(self) -> usize;


I wasn't entirely sure about this, but taking a Copy type by reference seems strange, so I changed to take it by value, happy to revert if people feel strongly

…buffer

tustvold · 2022-09-27T21:21:23Z

arrow-buffer/src/native.rs

-    fn from_usize(_: usize) -> Option<Self> {
-        None
-    }
+    /// Convert native integer type from usize


This doesn't change the behaviour, just documents it

alamb

LGTM

alamb · 2022-09-28T19:40:02Z

arrow-buffer/src/native.rs

-impl private::Sealed for f32 {}
-impl ArrowNativeType for f64 {}
-impl private::Sealed for f64 {}
+native_float!(f16, self, self.to_f32() as _);


TIL that as usize on a f32 does the right thing (truncates to integer) rather than reinterpreting as bytes

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=e8c093e2d8a2b83c9669c16ee04905ee

viirya · 2022-09-28T20:04:37Z

arrow/src/util/reader_parser.rs

@@ -97,20 +97,20 @@ impl Parser for Date32Type {
    fn parse(string: &str) -> Option<i32> {
        use chrono::Datelike;
        let date = string.parse::<chrono::NaiveDate>().ok()?;
-        Self::Native::from_i32(date.num_days_from_ce() - EPOCH_DAYS_FROM_CE)


Hmm, this is redundant.

Yes? That's why I removed it?

Yea, wondering why we had it before. 😄

viirya · 2022-09-28T20:05:58Z

arrow-buffer/src/native.rs

-    #[inline]
+    ///
+    /// Returns `None` if [`Self`] is not `i32`
+    #[deprecated(note = "please use `Option::Some` instead")]


The deprecation message looks a bit confused at first glance. I think it means that like Self::Native::from_i32(date.num_days_from_ce() - EPOCH_DAYS_FROM_CE) such a redundant case?

The only case where the method returned something other than None is when the type matches, in which case Some is the same behaviour

ursabot · 2022-09-28T21:13:57Z

Benchmark runs are scheduled for baseline = 7639f28 and contender = 178319b. 178319b is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ec2-t3-xlarge-us-east-2] ec2-t3-xlarge-us-east-2
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on test-mac-arm] test-mac-arm
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ursa-i9-9960x] ursa-i9-9960x
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ursa-thinkcentre-m75q] ursa-thinkcentre-m75q
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

github-actions bot added the arrow Changes to the arrow crate label Sep 27, 2022

tustvold commented Sep 27, 2022

View reviewed changes

arrow-buffer/src/native.rs Outdated Show resolved Hide resolved

tustvold commented Sep 27, 2022

View reviewed changes

tustvold changed the title ~~Remove num arrow buffer~~ Remove num dependency from arrow-buffer Sep 27, 2022

tustvold mentioned this pull request Sep 27, 2022

Remove some ArrowNativeType from conversions #2792

Closed

Replace checked casts with as for performance (apache#1918)

45aa763

Remove num dependency from arrow-buffer Deprecate unnecessary methods

tustvold force-pushed the remove-num-arrow-buffer branch from 738596a to 45aa763 Compare September 27, 2022 21:12

tustvold changed the title ~~Remove num dependency from arrow-buffer~~ Replace checked casts with as for performance (#1918) Sep 27, 2022

tustvold commented Sep 27, 2022

View reviewed changes

tustvold added the api-change Changes to the arrow API label Sep 27, 2022

Merge remote-tracking branch 'upstream/master' into remove-num-arrow-…

8895927

…buffer

tustvold commented Sep 27, 2022

View reviewed changes

github-actions bot added the parquet Changes to the parquet crate label Sep 27, 2022

tustvold changed the title ~~Replace checked casts with as for performance (#1918)~~ Cleanup ArrowNativeType (#1918) Sep 27, 2022

tustvold marked this pull request as ready for review September 27, 2022 21:28

alamb approved these changes Sep 28, 2022

View reviewed changes

viirya reviewed Sep 28, 2022

View reviewed changes

viirya approved these changes Sep 28, 2022

View reviewed changes

tustvold merged commit 178319b into apache:master Sep 28, 2022

alamb changed the title ~~Cleanup ArrowNativeType (#1918)~~ Cleanup ArrowNativeType (#1918) Sep 30, 2022

tustvold mentioned this pull request Oct 28, 2022

Add a way to get the dictionary index and values array reference #672

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup `ArrowNativeType` (#1918) #2793

Cleanup `ArrowNativeType` (#1918) #2793

tustvold commented Sep 27, 2022 •

edited

Loading

tustvold Sep 27, 2022

tustvold Sep 27, 2022

tustvold Sep 27, 2022

tustvold Sep 27, 2022

tustvold Sep 27, 2022 •

edited

Loading

tustvold Sep 27, 2022

alamb left a comment

alamb Sep 28, 2022

viirya Sep 28, 2022

tustvold Sep 28, 2022

viirya Sep 28, 2022

viirya Sep 28, 2022

tustvold Sep 28, 2022

ursabot commented Sep 28, 2022

Cleanup ArrowNativeType (#1918) #2793

Cleanup ArrowNativeType (#1918) #2793

Conversation

tustvold commented Sep 27, 2022 • edited Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tustvold Sep 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ursabot commented Sep 28, 2022

Cleanup `ArrowNativeType` (#1918) #2793

Cleanup `ArrowNativeType` (#1918) #2793

tustvold commented Sep 27, 2022 •

edited

Loading

tustvold Sep 27, 2022 •

edited

Loading