Add modulus ops into ArrowNativeTypeOp
#2756
Conversation
Signed-off-by: remzi <[email protected]>
Signed-off-by: remzi <[email protected]>
arrow/src/datatypes/native.rs
Outdated
| } | ||
|
|
||
| /// Only check `DivideByZero` error | ||
| fn mod_checked_divide_by_zero(self, rhs: Self) -> Result<Self> { |
There was a problem hiding this comment.
I don't really like to pollute the API arbitrarily. Could you keep two APIs (_wrapping, _checked) for mod?
There was a problem hiding this comment.
The reason I add 3 APIs here is that mod and div is different from other ops.
For add, sub, mul ..., we only need to consider the Overflow error, so two APIs (_wrapping, _checked) are enough.
But for mod and div, there are 2 kinds of errors Overflow and DivideByZero, which means we need to add new APIs if we only want to check one of them. And in the arithmetic kernel, we have 3 different divide APIs (mod is similar ):
divide: doesn't checkDivideByZeroorOverflow(This uses _wrapping)divide_opt: only checks theDivideByZero. (This is similar to the_checked_divided_by_zeroAPI I add, except that it returns anOptionbut not `Result)divide_checked: checks bothDivideByZeroerror andOverflowerror. (this uses _checked)
An alternative I think is that we could make mod_checked_divide_by_zero return an Option to match the behavior of div_opt / mod_opt
There was a problem hiding this comment.
As you can see, it is very easy to do checking of division by zero only check (as did in divide_opt). Just need to do additional is_zero check along with calling wrapping API. Not to mention that this is special case (only divide and modulus). I don't see there is a need to add additional API there. I think it is better to keep a simple API instead of adding new ones when you can use existing APIs to do the same thing.
There was a problem hiding this comment.
I think it is better to keep a simple API instead of adding new ones when you can use existing APIs to do the same thing.
This makes sense. I will update the PR.
Signed-off-by: remzi <[email protected]>
|
Hi @viirya, could we merge this? |
arrow/src/datatypes/native.rs
Outdated
| if rhs.is_zero() { | ||
| Err(ArrowError::DivideByZero) | ||
| } else { | ||
| Ok(self.mod_wrapping(rhs)) |
There was a problem hiding this comment.
| Ok(self.mod_wrapping(rhs)) | |
| Ok(self % rhs) |
There was a problem hiding this comment.
ditto, % is a fallible function.
There was a problem hiding this comment.
This is default implementation for floating point values...
There was a problem hiding this comment.
Yes, the panic only occurs on signed integers
| return simd_checked_divide_op(&left, &right, simd_checked_modulus::<T>, |a, b| { | ||
| a % b | ||
| a.mod_wrapping(b) | ||
| }); |
There was a problem hiding this comment.
SIMD op doesn't support checking/wrapping variants. You just change its scalar op. We should leave it as it is.
There was a problem hiding this comment.
a % b will panic at overflow, but a.wrapping_rem(b) won't:
https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=6d04230ac4454c895c2fe9e417c13938
If we don't want simd_checked_divide_op to panic, I guess we should use mod_wrapping?
There was a problem hiding this comment.
simd_checked_divide_op has two parts, one is SIMD op, one is scalar op. SIMD op doesn't support checking/wrapping variants, and I don't think we should change only scalar op.
There was a problem hiding this comment.
I don't think we should change only scalar op.
Thank you for explanation. This makes sense. I will update the code and add tests to freeze the behaviour.
|
I have two comments. Otherwise looks good for me. |
tustvold
changed the title
ArrawNativeTypeOpArrowNativeTypeOp
|
@HaoYang670 is this PR ready to merge? |
|
Benchmark runs are scheduled for baseline = 35c313b and contender = 191eaef. 191eaef is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
|
lol I didn't do anything except generate some alert spam :) |
Signed-off-by: remzi [email protected]
Which issue does this PR close?
Closes #2753.
Rationale for this change
What changes are included in this PR?
mod_wrapping: panics when divisor is 0 and returns 0 when overflow occurs (signed_int::MIN % -1)mod_checked: checks theDivideByZeroerror andOverflowerror, never panic.n % 0and overflow.Are there any user-facing changes?