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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
