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/13 04:51:01 UTC
[GitHub] [arrow-rs] HaoYang670 opened a new pull request, #2717: Update `try_binary` and `checked_ops`, and remove `math_checked_op`
HaoYang670 opened a new pull request, #2717:
URL: https://github.com/apache/arrow-rs/pull/2717
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 #2715.
# Rationale for this change
1. `try_binary` should not panic on unequaled array length
2. The checked ops (checked_add, checked_mul ...) is better to return Result. Because
- we can return the error directly in `try_binary` and other `try_...` kernels
- Different ops have different Errors (Overflow, dividebyZero)
3. Remove `math_checked_op` because it is the same as `try_binary`
# What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
# 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] viirya commented on a diff in pull request #2717: Update `try_binary` and `checked_ops`, and remove `math_checked_op`
Posted by GitBox <gi...@apache.org>.
viirya commented on code in PR #2717:
URL: https://github.com/apache/arrow-rs/pull/2717#discussion_r973507935
##########
arrow/src/compute/kernels/arithmetic.rs:
##########
@@ -1225,12 +1143,17 @@ where
pub fn divide_dyn(left: &dyn Array, right: &dyn Array) -> Result<ArrayRef> {
match left.data_type() {
DataType::Dictionary(_, _) => {
- typed_dict_math_op!(left, right, |a, b| a / b, math_divide_checked_op_dict)
+ typed_dict_math_op!(
+ left,
+ right,
+ |a, b| a.div_checked(b),
Review Comment:
I think `divide_dyn` is the kernel which only checks division by zero error.
--
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] ursabot commented on pull request #2717: Update `try_binary` and `checked_ops`, and remove `math_checked_op`
Posted by GitBox <gi...@apache.org>.
ursabot commented on PR #2717:
URL: https://github.com/apache/arrow-rs/pull/2717#issuecomment-1249220297
Benchmark runs are scheduled for baseline = 43d912c010b7374ceb3a632eedda4f55f90545d0 and contender = f572ec1bef4a66a00b78f1d80a39992d63444ec2. f572ec1bef4a66a00b78f1d80a39992d63444ec2 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped :warning: Benchmarking of arrow-rs-commits is not supported on ec2-t3-xlarge-us-east-2] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/ff017699752944fd87e0af7700e233a1...eb1c878852e64af9b83fc8e06ce24429/)
[Skipped :warning: Benchmarking of arrow-rs-commits is not supported on test-mac-arm] [test-mac-arm](https://conbench.ursa.dev/compare/runs/cca2f88db1eb4fa9ae6a4fcf937b58d3...6712e40081a2439b9d1330dac398c302/)
[Skipped :warning: Benchmarking of arrow-rs-commits is not supported on ursa-i9-9960x] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/5dc80f4caecd495dba6b80692bf20c53...d25d38a7b78b495ba6e88cfcc239b2c4/)
[Skipped :warning: Benchmarking of arrow-rs-commits is not supported on ursa-thinkcentre-m75q] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/a25b0e98e2b448a5a716afe732b60354...04e5b9da80c94797a9dc02430b96dcc5/)
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
--
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] tustvold commented on pull request #2717: Update `try_binary` and `checked_ops`, and remove `math_checked_op`
Posted by GitBox <gi...@apache.org>.
tustvold commented on PR #2717:
URL: https://github.com/apache/arrow-rs/pull/2717#issuecomment-1249209443
Currently running the benchmarks for this, just in case, LLVM is a fickle beast
--
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] tustvold commented on a diff in pull request #2717: Update `try_binary` and `checked_ops`, and remove `math_checked_op`
Posted by GitBox <gi...@apache.org>.
tustvold commented on code in PR #2717:
URL: https://github.com/apache/arrow-rs/pull/2717#discussion_r973211107
##########
arrow/src/compute/kernels/arithmetic.rs:
##########
@@ -1225,12 +1143,17 @@ where
pub fn divide_dyn(left: &dyn Array, right: &dyn Array) -> Result<ArrayRef> {
match left.data_type() {
DataType::Dictionary(_, _) => {
- typed_dict_math_op!(left, right, |a, b| a / b, math_divide_checked_op_dict)
+ typed_dict_math_op!(
+ left,
+ right,
+ |a, b| a.div_checked(b),
Review Comment:
How would division overflow?
Edit: oh floating points :sweat_smile:
--
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 #2717: Update `try_binary` and `checked_ops`, and remove `math_checked_op`
Posted by GitBox <gi...@apache.org>.
viirya commented on code in PR #2717:
URL: https://github.com/apache/arrow-rs/pull/2717#discussion_r973173982
##########
arrow/src/compute/kernels/arithmetic.rs:
##########
@@ -1225,12 +1143,17 @@ where
pub fn divide_dyn(left: &dyn Array, right: &dyn Array) -> Result<ArrayRef> {
match left.data_type() {
DataType::Dictionary(_, _) => {
- typed_dict_math_op!(left, right, |a, b| a / b, math_divide_checked_op_dict)
+ typed_dict_math_op!(
+ left,
+ right,
+ |a, b| a.div_checked(b),
Review Comment:
Err...why you changed it to use `div_checked`...
`divide_dyn` is no-overflow checking kernel.
I'm adding checked variant in another 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] viirya commented on a diff in pull request #2717: Update `try_binary` and `checked_ops`, and remove `math_checked_op`
Posted by GitBox <gi...@apache.org>.
viirya commented on code in PR #2717:
URL: https://github.com/apache/arrow-rs/pull/2717#discussion_r973173982
##########
arrow/src/compute/kernels/arithmetic.rs:
##########
@@ -1225,12 +1143,17 @@ where
pub fn divide_dyn(left: &dyn Array, right: &dyn Array) -> Result<ArrayRef> {
match left.data_type() {
DataType::Dictionary(_, _) => {
- typed_dict_math_op!(left, right, |a, b| a / b, math_divide_checked_op_dict)
+ typed_dict_math_op!(
+ left,
+ right,
+ |a, b| a.div_checked(b),
Review Comment:
Err...why you checked it to use `div_checked`...
`divide_dyn` is no-overflow checking kernel.
I'm adding checked variant in another 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 #2717: Update `try_binary` and `checked_ops`, and remove `math_checked_op`
Posted by GitBox <gi...@apache.org>.
HaoYang670 commented on code in PR #2717:
URL: https://github.com/apache/arrow-rs/pull/2717#discussion_r973504534
##########
arrow/src/compute/kernels/arithmetic.rs:
##########
@@ -1225,12 +1143,17 @@ where
pub fn divide_dyn(left: &dyn Array, right: &dyn Array) -> Result<ArrayRef> {
match left.data_type() {
DataType::Dictionary(_, _) => {
- typed_dict_math_op!(left, right, |a, b| a / b, math_divide_checked_op_dict)
+ typed_dict_math_op!(
+ left,
+ right,
+ |a, b| a.div_checked(b),
Review Comment:
> Err...why you changed it to use div_checked...
divide_dyn is no-overflow checking kernel.
Sorry for the delayed reply @viirya.
`divide_dyn` is a checked kernel I guess:
```rust
/// Perform `left / right` operation on two arrays. If either left or right value is null
/// then the result is also null. If any right hand value is zero then the result of this
/// operation will be `Err(ArrowError::DivideByZero)`.
pub fn divide_dyn(left: &dyn Array, right: &dyn Array) -> Result<ArrayRef> {
```
And it used `math_checked_divide_op` and `math_checked_divide_op_on_iters` to wrap a checking on an unchecked op: https://github.com/apache/arrow-rs/pull/2717/files#diff-f5fde646664693b58364e63a5dff0c05c82fa28c34e72135bbb9a412be5d53b4L169-L173
What I did is pushing down all checking to the `ArrowNativeTypeOp`, and there should be no bahaviour changes in the compute kernels
--
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 #2717: Update `try_binary` and `checked_ops`, and remove `math_checked_op`
Posted by GitBox <gi...@apache.org>.
viirya commented on code in PR #2717:
URL: https://github.com/apache/arrow-rs/pull/2717#discussion_r973507935
##########
arrow/src/compute/kernels/arithmetic.rs:
##########
@@ -1225,12 +1143,17 @@ where
pub fn divide_dyn(left: &dyn Array, right: &dyn Array) -> Result<ArrayRef> {
match left.data_type() {
DataType::Dictionary(_, _) => {
- typed_dict_math_op!(left, right, |a, b| a / b, math_divide_checked_op_dict)
+ typed_dict_math_op!(
+ left,
+ right,
+ |a, b| a.div_checked(b),
Review Comment:
I think `divide_dyn` is the kernel which only checks division by zero error.
edit: Oh, I misread your comment. But I don't think it is good idea to add too many APIs there. It is pretty easy to do extra division by zero check + wrapping division.
--
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 #2717: Update `try_binary` and `checked_ops`, and remove `math_checked_op`
Posted by GitBox <gi...@apache.org>.
HaoYang670 commented on PR #2717:
URL: https://github.com/apache/arrow-rs/pull/2717#issuecomment-1248808071
@viirya @liukun4515 @tustvold PTAL, thank you.
--
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] tustvold commented on a diff in pull request #2717: Update `try_binary` and `checked_ops`, and remove `math_checked_op`
Posted by GitBox <gi...@apache.org>.
tustvold commented on code in PR #2717:
URL: https://github.com/apache/arrow-rs/pull/2717#discussion_r973215572
##########
arrow/src/compute/kernels/arithmetic.rs:
##########
@@ -1225,12 +1143,17 @@ where
pub fn divide_dyn(left: &dyn Array, right: &dyn Array) -> Result<ArrayRef> {
match left.data_type() {
DataType::Dictionary(_, _) => {
- typed_dict_math_op!(left, right, |a, b| a / b, math_divide_checked_op_dict)
+ typed_dict_math_op!(
+ left,
+ right,
+ |a, b| a.div_checked(b),
Review Comment:
I think we're fine the definition of `div_checked` for integral types is
```
if rhs.is_zero() {
Err(ArrowError::DivideByZero)
} else {
self.checked_div(rhs).ok_or_else(|| {
ArrowError::ComputeError(format!(
"Overflow happened on: {:?} / {:?}",
self, rhs
))
})
}
```
The checked_div is technically redundant as we've already checked that the value isn't zero, i.e. this code could be simplified to
```
self.checked_div(rhs).ok_or_else(|| {
ArrowError::DivideByZero
})
```
The definition for floating points is
```
if rhs.is_zero() {
Err(ArrowError::DivideByZero)
} else {
Ok(self / rhs)
}
```
Which will not check for overflow, as it isn't meaningful to do so for floats?
--
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] tustvold commented on a diff in pull request #2717: Update `try_binary` and `checked_ops`, and remove `math_checked_op`
Posted by GitBox <gi...@apache.org>.
tustvold commented on code in PR #2717:
URL: https://github.com/apache/arrow-rs/pull/2717#discussion_r973242513
##########
arrow/src/compute/kernels/arithmetic.rs:
##########
@@ -1225,12 +1143,17 @@ where
pub fn divide_dyn(left: &dyn Array, right: &dyn Array) -> Result<ArrayRef> {
match left.data_type() {
DataType::Dictionary(_, _) => {
- typed_dict_math_op!(left, right, |a, b| a / b, math_divide_checked_op_dict)
+ typed_dict_math_op!(
+ left,
+ right,
+ |a, b| a.div_checked(b),
Review Comment:
But the only thing that is checked is if the rhs is 0? https://doc.rust-lang.org/std/primitive.i32.html#method.checked_div
--
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 #2717: Update `try_binary` and `checked_ops`, and remove `math_checked_op`
Posted by GitBox <gi...@apache.org>.
HaoYang670 commented on code in PR #2717:
URL: https://github.com/apache/arrow-rs/pull/2717#discussion_r972912387
##########
arrow/Cargo.toml:
##########
@@ -51,7 +51,7 @@ serde_json = { version = "1.0", default-features = false, features = ["std"], op
indexmap = { version = "1.9", default-features = false, features = ["std"] }
rand = { version = "0.8", default-features = false, features = ["std", "std_rng"], optional = true }
num = { version = "0.4", default-features = false, features = ["std"] }
-half = { version = "2.0", default-features = false }
+half = { version = "2.0", default-features = false, features = ["num-traits"]}
Review Comment:
Because the `f16` implements `num::Zero` only when enabling this feature.
--
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 #2717: Update `try_binary` and `checked_ops`, and remove `math_checked_op`
Posted by GitBox <gi...@apache.org>.
HaoYang670 commented on PR #2717:
URL: https://github.com/apache/arrow-rs/pull/2717#issuecomment-1245361772
Hmm, tests failed on SIMD. Let me find a good solution.
--
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 #2717: Update `try_binary` and `checked_ops`, and remove `math_checked_op`
Posted by GitBox <gi...@apache.org>.
viirya commented on code in PR #2717:
URL: https://github.com/apache/arrow-rs/pull/2717#discussion_r969861636
##########
arrow/src/datatypes/native.rs:
##########
@@ -172,32 +175,56 @@ pub(crate) mod native_op {
macro_rules! native_type_op {
($t:tt) => {
impl native_op::ArrowNativeTypeOp for $t {
- fn add_checked(self, rhs: Self) -> Option<Self> {
- self.checked_add(rhs)
+ fn add_checked(self, rhs: Self) -> Result<Self> {
+ self.checked_add(rhs).ok_or_else(|| {
+ ArrowError::ComputeError(format!(
+ "Overflow happened on: {:?} + {:?}",
+ self, rhs
+ ))
+ })
}
fn add_wrapping(self, rhs: Self) -> Self {
self.wrapping_add(rhs)
}
- fn sub_checked(self, rhs: Self) -> Option<Self> {
- self.checked_sub(rhs)
+ fn sub_checked(self, rhs: Self) -> Result<Self> {
+ self.checked_sub(rhs).ok_or_else(|| {
+ ArrowError::ComputeError(format!(
+ "Overflow happened on: {:?} - {:?}",
+ self, rhs
+ ))
+ })
}
fn sub_wrapping(self, rhs: Self) -> Self {
self.wrapping_sub(rhs)
}
- fn mul_checked(self, rhs: Self) -> Option<Self> {
- self.checked_mul(rhs)
+ fn mul_checked(self, rhs: Self) -> Result<Self> {
+ self.checked_mul(rhs).ok_or_else(|| {
+ ArrowError::ComputeError(format!(
+ "Overflow happened on: {:?} * {:?}",
+ self, rhs
+ ))
+ })
}
fn mul_wrapping(self, rhs: Self) -> Self {
self.wrapping_mul(rhs)
}
- fn div_checked(self, rhs: Self) -> Option<Self> {
- self.checked_div(rhs)
+ fn div_checked(self, rhs: Self) -> Result<Self> {
+ if rhs.is_zero() {
+ Err(ArrowError::DivideByZero)
+ } else {
Review Comment:
Ehh, I don't think this is correct. Your proposal #2719 is only checking division by zero error for integer division. But it doesn't match the behavior of C++ DivideChecked kernel, and Spark divide expression. We have clean distinction between checked and non-checked kernels. The checked kernel should catch such invalid inputs at the runtime.
--
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 #2717: Update `try_binary` and `checked_ops`, and remove `math_checked_op`
Posted by GitBox <gi...@apache.org>.
viirya commented on code in PR #2717:
URL: https://github.com/apache/arrow-rs/pull/2717#discussion_r969861636
##########
arrow/src/datatypes/native.rs:
##########
@@ -172,32 +175,56 @@ pub(crate) mod native_op {
macro_rules! native_type_op {
($t:tt) => {
impl native_op::ArrowNativeTypeOp for $t {
- fn add_checked(self, rhs: Self) -> Option<Self> {
- self.checked_add(rhs)
+ fn add_checked(self, rhs: Self) -> Result<Self> {
+ self.checked_add(rhs).ok_or_else(|| {
+ ArrowError::ComputeError(format!(
+ "Overflow happened on: {:?} + {:?}",
+ self, rhs
+ ))
+ })
}
fn add_wrapping(self, rhs: Self) -> Self {
self.wrapping_add(rhs)
}
- fn sub_checked(self, rhs: Self) -> Option<Self> {
- self.checked_sub(rhs)
+ fn sub_checked(self, rhs: Self) -> Result<Self> {
+ self.checked_sub(rhs).ok_or_else(|| {
+ ArrowError::ComputeError(format!(
+ "Overflow happened on: {:?} - {:?}",
+ self, rhs
+ ))
+ })
}
fn sub_wrapping(self, rhs: Self) -> Self {
self.wrapping_sub(rhs)
}
- fn mul_checked(self, rhs: Self) -> Option<Self> {
- self.checked_mul(rhs)
+ fn mul_checked(self, rhs: Self) -> Result<Self> {
+ self.checked_mul(rhs).ok_or_else(|| {
+ ArrowError::ComputeError(format!(
+ "Overflow happened on: {:?} * {:?}",
+ self, rhs
+ ))
+ })
}
fn mul_wrapping(self, rhs: Self) -> Self {
self.wrapping_mul(rhs)
}
- fn div_checked(self, rhs: Self) -> Option<Self> {
- self.checked_div(rhs)
+ fn div_checked(self, rhs: Self) -> Result<Self> {
+ if rhs.is_zero() {
+ Err(ArrowError::DivideByZero)
+ } else {
Review Comment:
Ehh, I don't think this is correct. Your proposal #2719 is only checking division by zero error for integer division. But it doesn't match the behavior of C++ DivideChecked kernel, and Spark divide expression. We have clear distinction between checked and non-checked kernels. The checked kernel should catch such invalid inputs at the runtime.
--
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 #2717: Update `try_binary` and `checked_ops`, and remove `math_checked_op`
Posted by GitBox <gi...@apache.org>.
viirya commented on code in PR #2717:
URL: https://github.com/apache/arrow-rs/pull/2717#discussion_r970311389
##########
arrow/src/datatypes/native.rs:
##########
@@ -172,32 +175,56 @@ pub(crate) mod native_op {
macro_rules! native_type_op {
($t:tt) => {
impl native_op::ArrowNativeTypeOp for $t {
- fn add_checked(self, rhs: Self) -> Option<Self> {
- self.checked_add(rhs)
+ fn add_checked(self, rhs: Self) -> Result<Self> {
+ self.checked_add(rhs).ok_or_else(|| {
+ ArrowError::ComputeError(format!(
+ "Overflow happened on: {:?} + {:?}",
+ self, rhs
+ ))
+ })
}
fn add_wrapping(self, rhs: Self) -> Self {
self.wrapping_add(rhs)
}
- fn sub_checked(self, rhs: Self) -> Option<Self> {
- self.checked_sub(rhs)
+ fn sub_checked(self, rhs: Self) -> Result<Self> {
+ self.checked_sub(rhs).ok_or_else(|| {
+ ArrowError::ComputeError(format!(
+ "Overflow happened on: {:?} - {:?}",
+ self, rhs
+ ))
+ })
}
fn sub_wrapping(self, rhs: Self) -> Self {
self.wrapping_sub(rhs)
}
- fn mul_checked(self, rhs: Self) -> Option<Self> {
- self.checked_mul(rhs)
+ fn mul_checked(self, rhs: Self) -> Result<Self> {
+ self.checked_mul(rhs).ok_or_else(|| {
+ ArrowError::ComputeError(format!(
+ "Overflow happened on: {:?} * {:?}",
+ self, rhs
+ ))
+ })
}
fn mul_wrapping(self, rhs: Self) -> Self {
self.wrapping_mul(rhs)
}
- fn div_checked(self, rhs: Self) -> Option<Self> {
- self.checked_div(rhs)
+ fn div_checked(self, rhs: Self) -> Result<Self> {
+ if rhs.is_zero() {
+ Err(ArrowError::DivideByZero)
+ } else {
Review Comment:
`divide_checked` doc has mentioned that it returns `ArrowError::DivideByZero if any right side value is zero. `divide` doc also describes that for floating point types, the result of dividing by zero follows normal floating point rules. Only for other numeric types, division by zero will panic.
I think it already said what you meant?
--
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] tustvold merged pull request #2717: Update `try_binary` and `checked_ops`, and remove `math_checked_op`
Posted by GitBox <gi...@apache.org>.
tustvold merged PR #2717:
URL: https://github.com/apache/arrow-rs/pull/2717
--
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] tustvold commented on a diff in pull request #2717: Update `try_binary` and `checked_ops`, and remove `math_checked_op`
Posted by GitBox <gi...@apache.org>.
tustvold commented on code in PR #2717:
URL: https://github.com/apache/arrow-rs/pull/2717#discussion_r973215572
##########
arrow/src/compute/kernels/arithmetic.rs:
##########
@@ -1225,12 +1143,17 @@ where
pub fn divide_dyn(left: &dyn Array, right: &dyn Array) -> Result<ArrayRef> {
match left.data_type() {
DataType::Dictionary(_, _) => {
- typed_dict_math_op!(left, right, |a, b| a / b, math_divide_checked_op_dict)
+ typed_dict_math_op!(
+ left,
+ right,
+ |a, b| a.div_checked(b),
Review Comment:
I think we're fine the definition of `div_checked` for integral types is
```
if rhs.is_zero() {
Err(ArrowError::DivideByZero)
} else {
self.checked_div(rhs).ok_or_else(|| {
ArrowError::ComputeError(format!(
"Overflow happened on: {:?} / {:?}",
self, rhs
))
})
}
```
The checked_div is technically redundant as we've already checked that the value isn't zero, i.e. this code could be simplified to
```
self.checked_div(rhs).ok_or_else(|| {
ArrowError::DivideByZero
})
```
The definition for floating points is
```
if rhs.is_zero() {
Err(ArrowError::DivideByZero)
} else {
Ok(self / rhs)
}
```
Which will not check for overflow, as it isn't meaningful to do so for floats?
TLDR: I don't think overflow checking for division makes sense? The only types that could overlow, i.e. floats, already have a way to represent that
--
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 #2717: Update `try_binary` and `checked_ops`, and remove `math_checked_op`
Posted by GitBox <gi...@apache.org>.
HaoYang670 commented on code in PR #2717:
URL: https://github.com/apache/arrow-rs/pull/2717#discussion_r973505926
##########
arrow/src/compute/kernels/arithmetic.rs:
##########
@@ -1225,12 +1143,17 @@ where
pub fn divide_dyn(left: &dyn Array, right: &dyn Array) -> Result<ArrayRef> {
match left.data_type() {
DataType::Dictionary(_, _) => {
- typed_dict_math_op!(left, right, |a, b| a / b, math_divide_checked_op_dict)
+ typed_dict_math_op!(
+ left,
+ right,
+ |a, b| a.div_checked(b),
Review Comment:
Op, I am wrong, sorry.
The `divide_dyn` just checked the `dividebyzero` error, but not the overflow error.
--
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 #2717: Update `try_binary` and `checked_ops`, and remove `math_checked_op`
Posted by GitBox <gi...@apache.org>.
HaoYang670 commented on PR #2717:
URL: https://github.com/apache/arrow-rs/pull/2717#issuecomment-1244895448
Mark this as a draft now because I also need to update the `math_checked_divide_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] tustvold commented on a diff in pull request #2717: Update `try_binary` and `checked_ops`, and remove `math_checked_op`
Posted by GitBox <gi...@apache.org>.
tustvold commented on code in PR #2717:
URL: https://github.com/apache/arrow-rs/pull/2717#discussion_r973207039
##########
arrow/src/compute/kernels/arithmetic.rs:
##########
@@ -1225,12 +1143,17 @@ where
pub fn divide_dyn(left: &dyn Array, right: &dyn Array) -> Result<ArrayRef> {
match left.data_type() {
DataType::Dictionary(_, _) => {
- typed_dict_math_op!(left, right, |a, b| a / b, math_divide_checked_op_dict)
+ typed_dict_math_op!(
+ left,
+ right,
+ |a, b| a.div_checked(b),
Review Comment:
I believe this fixes #2647 perhaps? I have to confess to being a little lost now, I had thought we'd collectively agreed that division didn't make sense to have an unchecked variant as Rust always checks division :sweat_smile:
--
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 #2717: Update `try_binary` and `checked_ops`, and remove `math_checked_op`
Posted by GitBox <gi...@apache.org>.
viirya commented on code in PR #2717:
URL: https://github.com/apache/arrow-rs/pull/2717#discussion_r973210671
##########
arrow/src/compute/kernels/arithmetic.rs:
##########
@@ -1225,12 +1143,17 @@ where
pub fn divide_dyn(left: &dyn Array, right: &dyn Array) -> Result<ArrayRef> {
match left.data_type() {
DataType::Dictionary(_, _) => {
- typed_dict_math_op!(left, right, |a, b| a / b, math_divide_checked_op_dict)
+ typed_dict_math_op!(
+ left,
+ right,
+ |a, b| a.div_checked(b),
Review Comment:
Oh, I meant overflow-checking. I think this change doesn't change division by zero behavior of `divide_dyn` kernel. It returns `ArrowError::DivideByZero` with/without this 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] tustvold commented on pull request #2717: Update `try_binary` and `checked_ops`, and remove `math_checked_op`
Posted by GitBox <gi...@apache.org>.
tustvold commented on PR #2717:
URL: https://github.com/apache/arrow-rs/pull/2717#issuecomment-1249216129
The benchmarks regularly fluctuate by +- 25%, this is not all that surprising given we're talking microseconds, but importantly we are not seeing anything greater which would imply a failure to vectorise correctly or similar :+1:
--
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 #2717: Update `try_binary` and `checked_ops`, and remove `math_checked_op`
Posted by GitBox <gi...@apache.org>.
HaoYang670 commented on code in PR #2717:
URL: https://github.com/apache/arrow-rs/pull/2717#discussion_r970277843
##########
arrow/src/datatypes/native.rs:
##########
@@ -172,32 +175,56 @@ pub(crate) mod native_op {
macro_rules! native_type_op {
($t:tt) => {
impl native_op::ArrowNativeTypeOp for $t {
- fn add_checked(self, rhs: Self) -> Option<Self> {
- self.checked_add(rhs)
+ fn add_checked(self, rhs: Self) -> Result<Self> {
+ self.checked_add(rhs).ok_or_else(|| {
+ ArrowError::ComputeError(format!(
+ "Overflow happened on: {:?} + {:?}",
+ self, rhs
+ ))
+ })
}
fn add_wrapping(self, rhs: Self) -> Self {
self.wrapping_add(rhs)
}
- fn sub_checked(self, rhs: Self) -> Option<Self> {
- self.checked_sub(rhs)
+ fn sub_checked(self, rhs: Self) -> Result<Self> {
+ self.checked_sub(rhs).ok_or_else(|| {
+ ArrowError::ComputeError(format!(
+ "Overflow happened on: {:?} - {:?}",
+ self, rhs
+ ))
+ })
}
fn sub_wrapping(self, rhs: Self) -> Self {
self.wrapping_sub(rhs)
}
- fn mul_checked(self, rhs: Self) -> Option<Self> {
- self.checked_mul(rhs)
+ fn mul_checked(self, rhs: Self) -> Result<Self> {
+ self.checked_mul(rhs).ok_or_else(|| {
+ ArrowError::ComputeError(format!(
+ "Overflow happened on: {:?} * {:?}",
+ self, rhs
+ ))
+ })
}
fn mul_wrapping(self, rhs: Self) -> Self {
self.wrapping_mul(rhs)
}
- fn div_checked(self, rhs: Self) -> Option<Self> {
- self.checked_div(rhs)
+ fn div_checked(self, rhs: Self) -> Result<Self> {
+ if rhs.is_zero() {
+ Err(ArrowError::DivideByZero)
+ } else {
Review Comment:
> it doesn't match the behavior of C++ DivideChecked kernel
Thank you @viirya, I find the code in C++: https://github.com/apache/arrow/blob/master/cpp/src/arrow/compute/kernels/base_arithmetic_internal.h#L410-L419 and it does the check for float point values.
If we follow the c++ behaviour, maybe we should add some docs such as
```
Float point Division by zero will return an error in `checked_div` but not panic when using `unchecked_div`
--
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] tustvold commented on a diff in pull request #2717: Update `try_binary` and `checked_ops`, and remove `math_checked_op`
Posted by GitBox <gi...@apache.org>.
tustvold commented on code in PR #2717:
URL: https://github.com/apache/arrow-rs/pull/2717#discussion_r972892271
##########
arrow/Cargo.toml:
##########
@@ -51,7 +51,7 @@ serde_json = { version = "1.0", default-features = false, features = ["std"], op
indexmap = { version = "1.9", default-features = false, features = ["std"] }
rand = { version = "0.8", default-features = false, features = ["std", "std_rng"], optional = true }
num = { version = "0.4", default-features = false, features = ["std"] }
-half = { version = "2.0", default-features = false }
+half = { version = "2.0", default-features = false, features = ["num-traits"]}
Review Comment:
Why this change?
Edit: Oh for num::Zero
--
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] tustvold commented on a diff in pull request #2717: Update `try_binary` and `checked_ops`, and remove `math_checked_op`
Posted by GitBox <gi...@apache.org>.
tustvold commented on code in PR #2717:
URL: https://github.com/apache/arrow-rs/pull/2717#discussion_r973207039
##########
arrow/src/compute/kernels/arithmetic.rs:
##########
@@ -1225,12 +1143,17 @@ where
pub fn divide_dyn(left: &dyn Array, right: &dyn Array) -> Result<ArrayRef> {
match left.data_type() {
DataType::Dictionary(_, _) => {
- typed_dict_math_op!(left, right, |a, b| a / b, math_divide_checked_op_dict)
+ typed_dict_math_op!(
+ left,
+ right,
+ |a, b| a.div_checked(b),
Review Comment:
I believe this fixes #2647 perhaps?
--
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 #2717: Update `try_binary` and `checked_ops`, and remove `math_checked_op`
Posted by GitBox <gi...@apache.org>.
viirya commented on code in PR #2717:
URL: https://github.com/apache/arrow-rs/pull/2717#discussion_r973261368
##########
arrow/src/compute/kernels/arithmetic.rs:
##########
@@ -1225,12 +1143,17 @@ where
pub fn divide_dyn(left: &dyn Array, right: &dyn Array) -> Result<ArrayRef> {
match left.data_type() {
DataType::Dictionary(_, _) => {
- typed_dict_math_op!(left, right, |a, b| a / b, math_divide_checked_op_dict)
+ typed_dict_math_op!(
+ left,
+ right,
+ |a, b| a.div_checked(b),
Review Comment:
No, overflow will be checked too:
`assert_eq!(i32::MIN.checked_div(-1), None);`
--
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] tustvold commented on a diff in pull request #2717: Update `try_binary` and `checked_ops`, and remove `math_checked_op`
Posted by GitBox <gi...@apache.org>.
tustvold commented on code in PR #2717:
URL: https://github.com/apache/arrow-rs/pull/2717#discussion_r972892271
##########
arrow/Cargo.toml:
##########
@@ -51,7 +51,7 @@ serde_json = { version = "1.0", default-features = false, features = ["std"], op
indexmap = { version = "1.9", default-features = false, features = ["std"] }
rand = { version = "0.8", default-features = false, features = ["std", "std_rng"], optional = true }
num = { version = "0.4", default-features = false, features = ["std"] }
-half = { version = "2.0", default-features = false }
+half = { version = "2.0", default-features = false, features = ["num-traits"]}
Review Comment:
Why this change?
--
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] tustvold commented on a diff in pull request #2717: Update `try_binary` and `checked_ops`, and remove `math_checked_op`
Posted by GitBox <gi...@apache.org>.
tustvold commented on code in PR #2717:
URL: https://github.com/apache/arrow-rs/pull/2717#discussion_r973211107
##########
arrow/src/compute/kernels/arithmetic.rs:
##########
@@ -1225,12 +1143,17 @@ where
pub fn divide_dyn(left: &dyn Array, right: &dyn Array) -> Result<ArrayRef> {
match left.data_type() {
DataType::Dictionary(_, _) => {
- typed_dict_math_op!(left, right, |a, b| a / b, math_divide_checked_op_dict)
+ typed_dict_math_op!(
+ left,
+ right,
+ |a, b| a.div_checked(b),
Review Comment:
How would division overflow?
--
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 #2717: Update `try_binary` and `checked_ops`, and remove `math_checked_op`
Posted by GitBox <gi...@apache.org>.
HaoYang670 commented on code in PR #2717:
URL: https://github.com/apache/arrow-rs/pull/2717#discussion_r973506921
##########
arrow/src/compute/kernels/arithmetic.rs:
##########
@@ -1225,12 +1143,17 @@ where
pub fn divide_dyn(left: &dyn Array, right: &dyn Array) -> Result<ArrayRef> {
match left.data_type() {
DataType::Dictionary(_, _) => {
- typed_dict_math_op!(left, right, |a, b| a / b, math_divide_checked_op_dict)
+ typed_dict_math_op!(
+ left,
+ right,
+ |a, b| a.div_checked(b),
Review Comment:
Should we add another `divide` ops in the `ArrowNativeTypeOp` that only checks the `dividebyzero` error ? For example:
```rust
// check dividebyzero and overflow
fn div_fully_checked(self, rhs: Self) -> Result<Self> {
if rhs.is_zero() {
Err(ArrowError::DivideByZero)
} else {
self.checked_div(rhs).ok_or_else(|| {
ArrowError::ComputeError(format!(
"Overflow happened on: {:?} / {:?}",
self, rhs
))
})
}
}
// only check the dividebyzero error
fn div_checked_divideByZero(self, rhs: Self) -> Result<Self> {}
// no check
fn div_wrapping(self, rhs: Self) -> Self {
self.wrapping_div(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] viirya commented on a diff in pull request #2717: Update `try_binary` and `checked_ops`, and remove `math_checked_op`
Posted by GitBox <gi...@apache.org>.
viirya commented on code in PR #2717:
URL: https://github.com/apache/arrow-rs/pull/2717#discussion_r970311389
##########
arrow/src/datatypes/native.rs:
##########
@@ -172,32 +175,56 @@ pub(crate) mod native_op {
macro_rules! native_type_op {
($t:tt) => {
impl native_op::ArrowNativeTypeOp for $t {
- fn add_checked(self, rhs: Self) -> Option<Self> {
- self.checked_add(rhs)
+ fn add_checked(self, rhs: Self) -> Result<Self> {
+ self.checked_add(rhs).ok_or_else(|| {
+ ArrowError::ComputeError(format!(
+ "Overflow happened on: {:?} + {:?}",
+ self, rhs
+ ))
+ })
}
fn add_wrapping(self, rhs: Self) -> Self {
self.wrapping_add(rhs)
}
- fn sub_checked(self, rhs: Self) -> Option<Self> {
- self.checked_sub(rhs)
+ fn sub_checked(self, rhs: Self) -> Result<Self> {
+ self.checked_sub(rhs).ok_or_else(|| {
+ ArrowError::ComputeError(format!(
+ "Overflow happened on: {:?} - {:?}",
+ self, rhs
+ ))
+ })
}
fn sub_wrapping(self, rhs: Self) -> Self {
self.wrapping_sub(rhs)
}
- fn mul_checked(self, rhs: Self) -> Option<Self> {
- self.checked_mul(rhs)
+ fn mul_checked(self, rhs: Self) -> Result<Self> {
+ self.checked_mul(rhs).ok_or_else(|| {
+ ArrowError::ComputeError(format!(
+ "Overflow happened on: {:?} * {:?}",
+ self, rhs
+ ))
+ })
}
fn mul_wrapping(self, rhs: Self) -> Self {
self.wrapping_mul(rhs)
}
- fn div_checked(self, rhs: Self) -> Option<Self> {
- self.checked_div(rhs)
+ fn div_checked(self, rhs: Self) -> Result<Self> {
+ if rhs.is_zero() {
+ Err(ArrowError::DivideByZero)
+ } else {
Review Comment:
I think `divide_checked` doc has mentioned that it returns `ArrowError::DivideByZero if any right side value is zero. `divide` doc also describes that for floating point types, the result of dividing by zero follows normal floating point rules. Only for other numeric types, division by zero will panic.
--
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] tustvold commented on a diff in pull request #2717: Update `try_binary` and `checked_ops`, and remove `math_checked_op`
Posted by GitBox <gi...@apache.org>.
tustvold commented on code in PR #2717:
URL: https://github.com/apache/arrow-rs/pull/2717#discussion_r973215572
##########
arrow/src/compute/kernels/arithmetic.rs:
##########
@@ -1225,12 +1143,17 @@ where
pub fn divide_dyn(left: &dyn Array, right: &dyn Array) -> Result<ArrayRef> {
match left.data_type() {
DataType::Dictionary(_, _) => {
- typed_dict_math_op!(left, right, |a, b| a / b, math_divide_checked_op_dict)
+ typed_dict_math_op!(
+ left,
+ right,
+ |a, b| a.div_checked(b),
Review Comment:
I think we're fine the definition of `div_checked` for integral types is
```
if rhs.is_zero() {
Err(ArrowError::DivideByZero)
} else {
self.checked_div(rhs).ok_or_else(|| {
ArrowError::ComputeError(format!(
"Overflow happened on: {:?} / {:?}",
self, rhs
))
})
}
```
The checked_div is technically redundant as we've already checked that the value isn't zero, i.e. this code could be simplified to
```
self.checked_div(rhs).ok_or_else(|| {
ArrowError::DivideByZero
})
```
The definition for floating points is
```
if rhs.is_zero() {
Err(ArrowError::DivideByZero)
} else {
Ok(self / rhs)
}
```
Which will not check for overflow, as it isn't meaningful to do so for floats?
TLDR: I don't think overflow checking for division makes sense?
--
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] tustvold commented on a diff in pull request #2717: Update `try_binary` and `checked_ops`, and remove `math_checked_op`
Posted by GitBox <gi...@apache.org>.
tustvold commented on code in PR #2717:
URL: https://github.com/apache/arrow-rs/pull/2717#discussion_r973262784
##########
arrow/src/compute/kernels/arithmetic.rs:
##########
@@ -1225,12 +1143,17 @@ where
pub fn divide_dyn(left: &dyn Array, right: &dyn Array) -> Result<ArrayRef> {
match left.data_type() {
DataType::Dictionary(_, _) => {
- typed_dict_math_op!(left, right, |a, b| a / b, math_divide_checked_op_dict)
+ typed_dict_math_op!(
+ left,
+ right,
+ |a, b| a.div_checked(b),
Review Comment:
Oh my it hadn't even occurred to me that signed integer division could overflow, that's wild :sweat_smile:
--
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 #2717: Update `try_binary` and `checked_ops`, and remove `math_checked_op`
Posted by GitBox <gi...@apache.org>.
HaoYang670 commented on code in PR #2717:
URL: https://github.com/apache/arrow-rs/pull/2717#discussion_r970590478
##########
arrow/src/compute/kernels/arithmetic.rs:
##########
@@ -76,32 +76,6 @@ where
Ok(binary(left, right, op))
}
-/// This is similar to `math_op` as it performs given operation between two input primitive arrays.
-/// But the given operation can return `None` if overflow is detected. For the case, this function
-/// returns an `Err`.
-fn math_checked_op<LT, RT, F>(
Review Comment:
We don't need this function any more because this is the same as `try_binary` after we updating the `try_binary` and the type of `op`
##########
arrow/src/compute/kernels/arithmetic.rs:
##########
@@ -2026,31 +1967,57 @@ mod tests {
#[test]
#[should_panic(expected = "DivideByZero")]
- fn test_primitive_array_divide_by_zero_with_checked() {
+ fn test_int_array_divide_by_zero_with_checked() {
Review Comment:
rename the test case to make it clearer.
##########
arrow/src/datatypes/native.rs:
##########
@@ -134,33 +138,38 @@ pub(crate) mod native_op {
+ Sub<Output = Self>
+ Mul<Output = Self>
+ Div<Output = Self>
+ + Zero
{
- fn add_checked(self, rhs: Self) -> Option<Self> {
- Some(self + rhs)
+ fn add_checked(self, rhs: Self) -> Result<Self> {
+ Ok(self + rhs)
}
fn add_wrapping(self, rhs: Self) -> Self {
self + rhs
}
- fn sub_checked(self, rhs: Self) -> Option<Self> {
- Some(self - rhs)
+ fn sub_checked(self, rhs: Self) -> Result<Self> {
+ Ok(self - rhs)
}
fn sub_wrapping(self, rhs: Self) -> Self {
self - rhs
}
- fn mul_checked(self, rhs: Self) -> Option<Self> {
- Some(self * rhs)
+ fn mul_checked(self, rhs: Self) -> Result<Self> {
+ Ok(self * rhs)
}
fn mul_wrapping(self, rhs: Self) -> Self {
self * rhs
}
- fn div_checked(self, rhs: Self) -> Option<Self> {
- Some(self / rhs)
+ fn div_checked(self, rhs: Self) -> Result<Self> {
Review Comment:
As for float type, we also need to check the `DivideByZero` error, make this as the default behaviour.
##########
arrow/src/compute/kernels/arithmetic.rs:
##########
@@ -119,26 +93,9 @@ where
LT: ArrowNumericType,
RT: ArrowNumericType,
RT::Native: One + Zero,
- F: Fn(LT::Native, RT::Native) -> Option<LT::Native>,
+ F: Fn(LT::Native, RT::Native) -> Result<LT::Native>,
{
- if left.len() != right.len() {
Review Comment:
I tried to remove this function but met some error from the type system. I will file a follow-up ticket to remove 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] viirya commented on a diff in pull request #2717: Update `try_binary` and `checked_ops`, and remove `math_checked_op`
Posted by GitBox <gi...@apache.org>.
viirya commented on code in PR #2717:
URL: https://github.com/apache/arrow-rs/pull/2717#discussion_r973240102
##########
arrow/src/compute/kernels/arithmetic.rs:
##########
@@ -1225,12 +1143,17 @@ where
pub fn divide_dyn(left: &dyn Array, right: &dyn Array) -> Result<ArrayRef> {
match left.data_type() {
DataType::Dictionary(_, _) => {
- typed_dict_math_op!(left, right, |a, b| a / b, math_divide_checked_op_dict)
+ typed_dict_math_op!(
+ left,
+ right,
+ |a, b| a.div_checked(b),
Review Comment:
It is checked for integer division.
--
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 #2717: Update `try_binary` and `checked_ops`, and remove `math_checked_op`
Posted by GitBox <gi...@apache.org>.
viirya commented on code in PR #2717:
URL: https://github.com/apache/arrow-rs/pull/2717#discussion_r970311389
##########
arrow/src/datatypes/native.rs:
##########
@@ -172,32 +175,56 @@ pub(crate) mod native_op {
macro_rules! native_type_op {
($t:tt) => {
impl native_op::ArrowNativeTypeOp for $t {
- fn add_checked(self, rhs: Self) -> Option<Self> {
- self.checked_add(rhs)
+ fn add_checked(self, rhs: Self) -> Result<Self> {
+ self.checked_add(rhs).ok_or_else(|| {
+ ArrowError::ComputeError(format!(
+ "Overflow happened on: {:?} + {:?}",
+ self, rhs
+ ))
+ })
}
fn add_wrapping(self, rhs: Self) -> Self {
self.wrapping_add(rhs)
}
- fn sub_checked(self, rhs: Self) -> Option<Self> {
- self.checked_sub(rhs)
+ fn sub_checked(self, rhs: Self) -> Result<Self> {
+ self.checked_sub(rhs).ok_or_else(|| {
+ ArrowError::ComputeError(format!(
+ "Overflow happened on: {:?} - {:?}",
+ self, rhs
+ ))
+ })
}
fn sub_wrapping(self, rhs: Self) -> Self {
self.wrapping_sub(rhs)
}
- fn mul_checked(self, rhs: Self) -> Option<Self> {
- self.checked_mul(rhs)
+ fn mul_checked(self, rhs: Self) -> Result<Self> {
+ self.checked_mul(rhs).ok_or_else(|| {
+ ArrowError::ComputeError(format!(
+ "Overflow happened on: {:?} * {:?}",
+ self, rhs
+ ))
+ })
}
fn mul_wrapping(self, rhs: Self) -> Self {
self.wrapping_mul(rhs)
}
- fn div_checked(self, rhs: Self) -> Option<Self> {
- self.checked_div(rhs)
+ fn div_checked(self, rhs: Self) -> Result<Self> {
+ if rhs.is_zero() {
+ Err(ArrowError::DivideByZero)
+ } else {
Review Comment:
`divide_checked` doc has mentioned that it returns "ArrowError::DivideByZero if any right side value is zero". `divide` doc also describes that for floating point types, the result of dividing by zero follows normal floating point rules. Only for other numeric types, division by zero will panic.
I think it already said what you meant?
--
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 #2717: Update `try_binary` and `checked_ops`, and remove `math_checked_op`
Posted by GitBox <gi...@apache.org>.
HaoYang670 commented on code in PR #2717:
URL: https://github.com/apache/arrow-rs/pull/2717#discussion_r970357481
##########
arrow/src/datatypes/native.rs:
##########
@@ -172,32 +175,56 @@ pub(crate) mod native_op {
macro_rules! native_type_op {
($t:tt) => {
impl native_op::ArrowNativeTypeOp for $t {
- fn add_checked(self, rhs: Self) -> Option<Self> {
- self.checked_add(rhs)
+ fn add_checked(self, rhs: Self) -> Result<Self> {
+ self.checked_add(rhs).ok_or_else(|| {
+ ArrowError::ComputeError(format!(
+ "Overflow happened on: {:?} + {:?}",
+ self, rhs
+ ))
+ })
}
fn add_wrapping(self, rhs: Self) -> Self {
self.wrapping_add(rhs)
}
- fn sub_checked(self, rhs: Self) -> Option<Self> {
- self.checked_sub(rhs)
+ fn sub_checked(self, rhs: Self) -> Result<Self> {
+ self.checked_sub(rhs).ok_or_else(|| {
+ ArrowError::ComputeError(format!(
+ "Overflow happened on: {:?} - {:?}",
+ self, rhs
+ ))
+ })
}
fn sub_wrapping(self, rhs: Self) -> Self {
self.wrapping_sub(rhs)
}
- fn mul_checked(self, rhs: Self) -> Option<Self> {
- self.checked_mul(rhs)
+ fn mul_checked(self, rhs: Self) -> Result<Self> {
+ self.checked_mul(rhs).ok_or_else(|| {
+ ArrowError::ComputeError(format!(
+ "Overflow happened on: {:?} * {:?}",
+ self, rhs
+ ))
+ })
}
fn mul_wrapping(self, rhs: Self) -> Self {
self.wrapping_mul(rhs)
}
- fn div_checked(self, rhs: Self) -> Option<Self> {
- self.checked_div(rhs)
+ fn div_checked(self, rhs: Self) -> Result<Self> {
+ if rhs.is_zero() {
+ Err(ArrowError::DivideByZero)
+ } else {
Review Comment:
Great. I'll update the issue and 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] tustvold commented on a diff in pull request #2717: Update `try_binary` and `checked_ops`, and remove `math_checked_op`
Posted by GitBox <gi...@apache.org>.
tustvold commented on code in PR #2717:
URL: https://github.com/apache/arrow-rs/pull/2717#discussion_r973211107
##########
arrow/src/compute/kernels/arithmetic.rs:
##########
@@ -1225,12 +1143,17 @@ where
pub fn divide_dyn(left: &dyn Array, right: &dyn Array) -> Result<ArrayRef> {
match left.data_type() {
DataType::Dictionary(_, _) => {
- typed_dict_math_op!(left, right, |a, b| a / b, math_divide_checked_op_dict)
+ typed_dict_math_op!(
+ left,
+ right,
+ |a, b| a.div_checked(b),
Review Comment:
How would division overflow?
--
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] tustvold commented on a diff in pull request #2717: Update `try_binary` and `checked_ops`, and remove `math_checked_op`
Posted by GitBox <gi...@apache.org>.
tustvold commented on code in PR #2717:
URL: https://github.com/apache/arrow-rs/pull/2717#discussion_r973242513
##########
arrow/src/compute/kernels/arithmetic.rs:
##########
@@ -1225,12 +1143,17 @@ where
pub fn divide_dyn(left: &dyn Array, right: &dyn Array) -> Result<ArrayRef> {
match left.data_type() {
DataType::Dictionary(_, _) => {
- typed_dict_math_op!(left, right, |a, b| a / b, math_divide_checked_op_dict)
+ typed_dict_math_op!(
+ left,
+ right,
+ |a, b| a.div_checked(b),
Review Comment:
But the only thing that is checked is if the rhs is 0? https://doc.rust-lang.org/std/primitive.i32.html#method.checked_div
(which is redundant as this has already been checked, and so we can/should simplify the logic but it won't be user-visible)
--
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 #2717: Update `try_binary` and `checked_ops`, and remove `math_checked_op`
Posted by GitBox <gi...@apache.org>.
viirya commented on code in PR #2717:
URL: https://github.com/apache/arrow-rs/pull/2717#discussion_r973506573
##########
arrow/src/compute/kernels/arithmetic.rs:
##########
@@ -1225,12 +1143,17 @@ where
pub fn divide_dyn(left: &dyn Array, right: &dyn Array) -> Result<ArrayRef> {
match left.data_type() {
DataType::Dictionary(_, _) => {
- typed_dict_math_op!(left, right, |a, b| a / b, math_divide_checked_op_dict)
+ typed_dict_math_op!(
+ left,
+ right,
+ |a, b| a.div_checked(b),
Review Comment:
No worries. I've fixed in the PR to add checked variant.
##########
arrow/src/compute/kernels/arithmetic.rs:
##########
@@ -1225,12 +1143,17 @@ where
pub fn divide_dyn(left: &dyn Array, right: &dyn Array) -> Result<ArrayRef> {
match left.data_type() {
DataType::Dictionary(_, _) => {
- typed_dict_math_op!(left, right, |a, b| a / b, math_divide_checked_op_dict)
+ typed_dict_math_op!(
+ left,
+ right,
+ |a, b| a.div_checked(b),
Review Comment:
No worries. I've fixed it in the PR to add checked variant.
--
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 #2717: Update `try_binary` and `checked_ops`, and remove `math_checked_op`
Posted by GitBox <gi...@apache.org>.
viirya commented on code in PR #2717:
URL: https://github.com/apache/arrow-rs/pull/2717#discussion_r973505662
##########
arrow/src/compute/kernels/arithmetic.rs:
##########
@@ -1225,12 +1143,17 @@ where
pub fn divide_dyn(left: &dyn Array, right: &dyn Array) -> Result<ArrayRef> {
match left.data_type() {
DataType::Dictionary(_, _) => {
- typed_dict_math_op!(left, right, |a, b| a / b, math_divide_checked_op_dict)
+ typed_dict_math_op!(
+ left,
+ right,
+ |a, b| a.div_checked(b),
Review Comment:
For "checked" kernel, I meant overflow checking behavior.
--
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