You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2022/09/17 09:40:59 UTC
[GitHub] [arrow-rs] HaoYang670 opened a new pull request, #2756: Add modulus ops into `ArrawNativeTypeOp`
HaoYang670 opened a new pull request, #2756:
URL: https://github.com/apache/arrow-rs/pull/2756
Signed-off-by: remzi <13...@gmail.com>
# Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
Closes #2753.
# Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
# What changes are included in this PR?
1. Add 3 kinds of modulus ops:
- `mod_wrapping`: panics when divisor is 0 and returns 0 when overflow occurs (signed_int::MIN % -1)
- `mod_checked_divide_by_zero`: only checks the `DivideByZero` error and never panic.
- `mod_fully_checked`: checks the `DivideByZero` error and `Overflow` error, never panic.
2. Update `modulus` and `modulus_scalar` to use `mod_checked_divide_by_zero`
3. Add tests to check the behaviour of `n % 0` and overflow.
# Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!---
If there are any breaking changes to public APIs, please add the `breaking change` label.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] HaoYang670 commented on a diff in pull request #2756: Add modulus ops into `ArrowNativeTypeOp`
Posted by GitBox <gi...@apache.org>.
HaoYang670 commented on code in PR #2756:
URL: https://github.com/apache/arrow-rs/pull/2756#discussion_r984303864
##########
arrow/src/datatypes/native.rs:
##########
@@ -102,6 +103,18 @@ pub(crate) mod native_op {
fn div_wrapping(self, rhs: Self) -> Self {
self / rhs
}
+
+ fn mod_checked(self, rhs: Self) -> Result<Self> {
+ if rhs.is_zero() {
+ Err(ArrowError::DivideByZero)
+ } else {
+ Ok(self.mod_wrapping(rhs))
Review Comment:
Done!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] viirya commented on a diff in pull request #2756: Add modulus ops into `ArrawNativeTypeOp`
Posted by GitBox <gi...@apache.org>.
viirya commented on code in PR #2756:
URL: https://github.com/apache/arrow-rs/pull/2756#discussion_r977115695
##########
arrow/src/datatypes/native.rs:
##########
@@ -102,6 +103,18 @@ pub(crate) mod native_op {
fn div_wrapping(self, rhs: Self) -> Self {
self / rhs
}
+
+ fn mod_checked(self, rhs: Self) -> Result<Self> {
+ if rhs.is_zero() {
+ Err(ArrowError::DivideByZero)
+ } else {
+ Ok(self.mod_wrapping(rhs))
Review Comment:
```suggestion
Ok(self % rhs)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] HaoYang670 commented on pull request #2756: Add modulus ops into `ArrawNativeTypeOp`
Posted by GitBox <gi...@apache.org>.
HaoYang670 commented on PR #2756:
URL: https://github.com/apache/arrow-rs/pull/2756#issuecomment-1254394383
Hi @viirya, could we merge this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] HaoYang670 commented on a diff in pull request #2756: Add modulus ops into `ArrawNativeTypeOp`
Posted by GitBox <gi...@apache.org>.
HaoYang670 commented on code in PR #2756:
URL: https://github.com/apache/arrow-rs/pull/2756#discussion_r973647643
##########
arrow/src/datatypes/native.rs:
##########
@@ -102,6 +103,28 @@ pub(crate) mod native_op {
fn div_wrapping(self, rhs: Self) -> Self {
self / rhs
}
+
+ /// Check `DivideByZero` error and `Overflow` error.
+ fn mod_fully_checked(self, rhs: Self) -> Result<Self> {
+ if rhs.is_zero() {
+ Err(ArrowError::DivideByZero)
+ } else {
+ Ok(self.mod_wrapping(rhs))
+ }
+ }
+
+ /// Only check `DivideByZero` error
+ fn mod_checked_divide_by_zero(self, rhs: Self) -> Result<Self> {
Review 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 check `DivideByZero` or `Overflow`
- `divide_opt`: only checks the `DivideByZero`. (This is similar to the `mod_checked_divided_by_zero` API I add, except that it returns an `Option` but not `Result)
- `divide_checked`: checks both `DivideByZero` error and `Overflow` error.
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`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] viirya commented on a diff in pull request #2756: Add modulus ops into `ArrawNativeTypeOp`
Posted by GitBox <gi...@apache.org>.
viirya commented on code in PR #2756:
URL: https://github.com/apache/arrow-rs/pull/2756#discussion_r977183837
##########
arrow/src/datatypes/native.rs:
##########
@@ -102,6 +103,18 @@ pub(crate) mod native_op {
fn div_wrapping(self, rhs: Self) -> Self {
self / rhs
}
+
+ fn mod_checked(self, rhs: Self) -> Result<Self> {
+ if rhs.is_zero() {
+ Err(ArrowError::DivideByZero)
+ } else {
+ Ok(self.mod_wrapping(rhs))
Review Comment:
This is default implementation for floating point values...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] viirya commented on a diff in pull request #2756: Add modulus ops into `ArrawNativeTypeOp`
Posted by GitBox <gi...@apache.org>.
viirya commented on code in PR #2756:
URL: https://github.com/apache/arrow-rs/pull/2756#discussion_r973648355
##########
arrow/src/datatypes/native.rs:
##########
@@ -102,6 +103,28 @@ pub(crate) mod native_op {
fn div_wrapping(self, rhs: Self) -> Self {
self / rhs
}
+
+ /// Check `DivideByZero` error and `Overflow` error.
+ fn mod_fully_checked(self, rhs: Self) -> Result<Self> {
+ if rhs.is_zero() {
+ Err(ArrowError::DivideByZero)
+ } else {
+ Ok(self.mod_wrapping(rhs))
+ }
+ }
+
+ /// Only check `DivideByZero` error
+ fn mod_checked_divide_by_zero(self, rhs: Self) -> Result<Self> {
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] HaoYang670 commented on a diff in pull request #2756: Add modulus ops into `ArrawNativeTypeOp`
Posted by GitBox <gi...@apache.org>.
HaoYang670 commented on code in PR #2756:
URL: https://github.com/apache/arrow-rs/pull/2756#discussion_r977624743
##########
arrow/src/compute/kernels/arithmetic.rs:
##########
@@ -1075,18 +1075,18 @@ pub fn modulus<T>(
) -> Result<PrimitiveArray<T>>
where
T: ArrowNumericType,
- T::Native: Rem<Output = T::Native> + Zero + One,
+ T::Native: ArrowNativeTypeOp + One,
{
#[cfg(feature = "simd")]
return simd_checked_divide_op(&left, &right, simd_checked_modulus::<T>, |a, b| {
- a % b
+ a.mod_wrapping(b)
});
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] alamb commented on pull request #2756: Add modulus ops into `ArrowNativeTypeOp`
Posted by GitBox <gi...@apache.org>.
alamb commented on PR #2756:
URL: https://github.com/apache/arrow-rs/pull/2756#issuecomment-1265453369
@HaoYang670 is this PR ready to merge?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] HaoYang670 commented on a diff in pull request #2756: Add modulus ops into `ArrawNativeTypeOp`
Posted by GitBox <gi...@apache.org>.
HaoYang670 commented on code in PR #2756:
URL: https://github.com/apache/arrow-rs/pull/2756#discussion_r973649010
##########
arrow/src/datatypes/native.rs:
##########
@@ -102,6 +103,28 @@ pub(crate) mod native_op {
fn div_wrapping(self, rhs: Self) -> Self {
self / rhs
}
+
+ /// Check `DivideByZero` error and `Overflow` error.
+ fn mod_fully_checked(self, rhs: Self) -> Result<Self> {
+ if rhs.is_zero() {
+ Err(ArrowError::DivideByZero)
+ } else {
+ Ok(self.mod_wrapping(rhs))
+ }
+ }
+
+ /// Only check `DivideByZero` error
+ fn mod_checked_divide_by_zero(self, rhs: Self) -> Result<Self> {
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] HaoYang670 commented on a diff in pull request #2756: Add modulus ops into `ArrawNativeTypeOp`
Posted by GitBox <gi...@apache.org>.
HaoYang670 commented on code in PR #2756:
URL: https://github.com/apache/arrow-rs/pull/2756#discussion_r973647643
##########
arrow/src/datatypes/native.rs:
##########
@@ -102,6 +103,28 @@ pub(crate) mod native_op {
fn div_wrapping(self, rhs: Self) -> Self {
self / rhs
}
+
+ /// Check `DivideByZero` error and `Overflow` error.
+ fn mod_fully_checked(self, rhs: Self) -> Result<Self> {
+ if rhs.is_zero() {
+ Err(ArrowError::DivideByZero)
+ } else {
+ Ok(self.mod_wrapping(rhs))
+ }
+ }
+
+ /// Only check `DivideByZero` error
+ fn mod_checked_divide_by_zero(self, rhs: Self) -> Result<Self> {
Review 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 check `DivideByZero` or `Overflow` (This uses _wrapping)
- `divide_opt`: only checks the `DivideByZero`. (This is similar to the `_checked_divided_by_zero` API I add, except that it returns an `Option` but not `Result)
- `divide_checked`: checks both `DivideByZero` error and `Overflow` error. (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`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] viirya commented on pull request #2756: Add modulus ops into `ArrawNativeTypeOp`
Posted by GitBox <gi...@apache.org>.
viirya commented on PR #2756:
URL: https://github.com/apache/arrow-rs/pull/2756#issuecomment-1254415404
I have two comments. Otherwise looks good for me.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] viirya commented on a diff in pull request #2756: Add modulus ops into `ArrawNativeTypeOp`
Posted by GitBox <gi...@apache.org>.
viirya commented on code in PR #2756:
URL: https://github.com/apache/arrow-rs/pull/2756#discussion_r977117208
##########
arrow/src/compute/kernels/arithmetic.rs:
##########
@@ -1075,18 +1075,18 @@ pub fn modulus<T>(
) -> Result<PrimitiveArray<T>>
where
T: ArrowNumericType,
- T::Native: Rem<Output = T::Native> + Zero + One,
+ T::Native: ArrowNativeTypeOp + One,
{
#[cfg(feature = "simd")]
return simd_checked_divide_op(&left, &right, simd_checked_modulus::<T>, |a, b| {
- a % b
+ a.mod_wrapping(b)
});
Review Comment:
SIMD op doesn't support checking/wrapping variants. You just change its scalar op. We should leave it as it is.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] viirya commented on a diff in pull request #2756: Add modulus ops into `ArrawNativeTypeOp`
Posted by GitBox <gi...@apache.org>.
viirya commented on code in PR #2756:
URL: https://github.com/apache/arrow-rs/pull/2756#discussion_r977868778
##########
arrow/src/datatypes/native.rs:
##########
@@ -102,6 +103,18 @@ pub(crate) mod native_op {
fn div_wrapping(self, rhs: Self) -> Self {
self / rhs
}
+
+ fn mod_checked(self, rhs: Self) -> Result<Self> {
+ if rhs.is_zero() {
+ Err(ArrowError::DivideByZero)
+ } else {
+ Ok(self.mod_wrapping(rhs))
Review Comment:
This is not changed yet.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] HaoYang670 commented on a diff in pull request #2756: Add modulus ops into `ArrawNativeTypeOp`
Posted by GitBox <gi...@apache.org>.
HaoYang670 commented on code in PR #2756:
URL: https://github.com/apache/arrow-rs/pull/2756#discussion_r977297492
##########
arrow/src/datatypes/native.rs:
##########
@@ -102,6 +103,18 @@ pub(crate) mod native_op {
fn div_wrapping(self, rhs: Self) -> Self {
self / rhs
}
+
+ fn mod_checked(self, rhs: Self) -> Result<Self> {
+ if rhs.is_zero() {
+ Err(ArrowError::DivideByZero)
+ } else {
+ Ok(self.mod_wrapping(rhs))
Review Comment:
Yes, the panic only occurs on signed integers
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] HaoYang670 commented on a diff in pull request #2756: Add modulus ops into `ArrawNativeTypeOp`
Posted by GitBox <gi...@apache.org>.
HaoYang670 commented on code in PR #2756:
URL: https://github.com/apache/arrow-rs/pull/2756#discussion_r977145512
##########
arrow/src/datatypes/native.rs:
##########
@@ -102,6 +103,18 @@ pub(crate) mod native_op {
fn div_wrapping(self, rhs: Self) -> Self {
self / rhs
}
+
+ fn mod_checked(self, rhs: Self) -> Result<Self> {
+ if rhs.is_zero() {
+ Err(ArrowError::DivideByZero)
+ } else {
+ Ok(self.mod_wrapping(rhs))
Review Comment:
ditto, `%` is a fallible function.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] viirya commented on a diff in pull request #2756: Add modulus ops into `ArrawNativeTypeOp`
Posted by GitBox <gi...@apache.org>.
viirya commented on code in PR #2756:
URL: https://github.com/apache/arrow-rs/pull/2756#discussion_r977185103
##########
arrow/src/compute/kernels/arithmetic.rs:
##########
@@ -1075,18 +1075,18 @@ pub fn modulus<T>(
) -> Result<PrimitiveArray<T>>
where
T: ArrowNumericType,
- T::Native: Rem<Output = T::Native> + Zero + One,
+ T::Native: ArrowNativeTypeOp + One,
{
#[cfg(feature = "simd")]
return simd_checked_divide_op(&left, &right, simd_checked_modulus::<T>, |a, b| {
- a % b
+ a.mod_wrapping(b)
});
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] HaoYang670 commented on a diff in pull request #2756: Add modulus ops into `ArrawNativeTypeOp`
Posted by GitBox <gi...@apache.org>.
HaoYang670 commented on code in PR #2756:
URL: https://github.com/apache/arrow-rs/pull/2756#discussion_r977143368
##########
arrow/src/compute/kernels/arithmetic.rs:
##########
@@ -1075,18 +1075,18 @@ pub fn modulus<T>(
) -> Result<PrimitiveArray<T>>
where
T: ArrowNumericType,
- T::Native: Rem<Output = T::Native> + Zero + One,
+ T::Native: ArrowNativeTypeOp + One,
{
#[cfg(feature = "simd")]
return simd_checked_divide_op(&left, &right, simd_checked_modulus::<T>, |a, b| {
- a % b
+ a.mod_wrapping(b)
});
Review 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`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] viirya commented on a diff in pull request #2756: Add modulus ops into `ArrawNativeTypeOp`
Posted by GitBox <gi...@apache.org>.
viirya commented on code in PR #2756:
URL: https://github.com/apache/arrow-rs/pull/2756#discussion_r973603267
##########
arrow/src/datatypes/native.rs:
##########
@@ -102,6 +103,28 @@ pub(crate) mod native_op {
fn div_wrapping(self, rhs: Self) -> Self {
self / rhs
}
+
+ /// Check `DivideByZero` error and `Overflow` error.
+ fn mod_fully_checked(self, rhs: Self) -> Result<Self> {
+ if rhs.is_zero() {
+ Err(ArrowError::DivideByZero)
+ } else {
+ Ok(self.mod_wrapping(rhs))
+ }
+ }
+
+ /// Only check `DivideByZero` error
+ fn mod_checked_divide_by_zero(self, rhs: Self) -> Result<Self> {
Review Comment:
I don't really like to pollute the API arbitrarily. Could you keep two APIs (_wrapping, _checked) for mod?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org