Simplify ArrowNativeType #2841
Simplify ArrowNativeType #2841
Conversation
tustvold
force-pushed
the
simplify-arrow-native-type
branch
from
ccf135e to
098bd14
Compare
| + Add<Output = Self> | ||
| + Sub<Output = Self> | ||
| + Mul<Output = Self> | ||
| + Div<Output = Self> | ||
| + Rem<Output = Self> |
There was a problem hiding this comment.
Removing these ensures that kernels are correctly selecting either the wrapping or checked variants
There was a problem hiding this comment.
Hm, I don't get this correctly. Why removing these can ensure that?
There was a problem hiding this comment.
Ensure might be too strong, but if writing a generic function using this as a type constraint the default implementations won't be available. For example, it caught that the SIMD kernels were using the default %
| + Mul<Output = Self> | ||
| + Div<Output = Self> | ||
| + Rem<Output = Self> | ||
| + Zero |
There was a problem hiding this comment.
I opted to remove this as it avoids committing to using the num traits in our public API
| + private::Sealed | ||
| { | ||
| fn add_checked(self, rhs: Self) -> Result<Self> { | ||
| Ok(self + rhs) |
There was a problem hiding this comment.
I perpetually found the default definitions confusing, moving them into the separate macros makes it clear what applies to what types
|
|
||
| fn mod_wrapping(self, rhs: Self) -> Self { | ||
| self % rhs |
There was a problem hiding this comment.
As evidence of the confusion, these are the defaults for the integer types, whereas the above are the defaults for floating point types 😅
| /// | ||
| /// For floating point types, overflow and division by zero follows normal floating point rules | ||
| /// | ||
| /// For integer types overflow will wrap around. Division by zero will currently panic, although |
There was a problem hiding this comment.
I plan to address this in a follow up, the current kernel is effectively useless for integer types
There was a problem hiding this comment.
Because nulls will have the value zero which then panic
| fn test_int_array_modulus_overflow_wrapping() { | ||
| let a = Int32Array::from(vec![i32::MIN]); | ||
| let b = Int32Array::from(vec![-1]); | ||
| let result = modulus(&a, &b).unwrap(); | ||
| assert_eq!(0, result.value(0)) | ||
| } | ||
|
|
||
| #[test] |
There was a problem hiding this comment.
This was an undocumented bug that has now been fixed 🎉
There was a problem hiding this comment.
For example, it caught that the SIMD kernels were using the default %
This?
|
Benchmark runs are scheduled for baseline = 97480a9 and contender = c8321f4. c8321f4 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Which issue does this PR close?
Closes #.
Rationale for this change
#2840 made this public, prior to releasing lets take the opportunity to clean it up a a bit.
What changes are included in this PR?
Tweaks the signature of ArrowNativeType, reduces the constraints on some kernels, and cleans up the documentation
Are there any user-facing changes?
No, ArrowNativeTypeOp is not yet public, and this only removes constraints from public methods