From d2497d2afb3f60684caf8f79a9d75bfe4f030bec Mon Sep 17 00:00:00 2001 From: remzi <13716567376yh@gmail.com> Date: Sat, 17 Sep 2022 17:33:35 +0800 Subject: [PATCH 1/5] add 3 mod ops and tests Signed-off-by: remzi <13716567376yh@gmail.com> --- arrow/src/compute/kernels/arithmetic.rs | 56 ++++++++++++++++++------- arrow/src/datatypes/native.rs | 42 ++++++++++++++++++- 2 files changed, 83 insertions(+), 15 deletions(-) diff --git a/arrow/src/compute/kernels/arithmetic.rs b/arrow/src/compute/kernels/arithmetic.rs index 7b91a261c7e..49a8d58481e 100644 --- a/arrow/src/compute/kernels/arithmetic.rs +++ b/arrow/src/compute/kernels/arithmetic.rs @@ -22,7 +22,7 @@ //! `RUSTFLAGS="-C target-feature=+avx2"` for example. See the documentation //! [here](https://doc.rust-lang.org/stable/core/arch/) for more information. -use std::ops::{Div, Neg, Rem}; +use std::ops::{Div, Neg}; use num::{One, Zero}; @@ -1075,20 +1075,14 @@ pub fn modulus( ) -> Result> where T: ArrowNumericType, - T::Native: Rem + Zero + One, + T::Native: ArrowNativeTypeOp, { #[cfg(feature = "simd")] return simd_checked_divide_op(&left, &right, simd_checked_modulus::, |a, b| { a % b }); #[cfg(not(feature = "simd"))] - return try_binary(left, right, |a, b| { - if b.is_zero() { - Err(ArrowError::DivideByZero) - } else { - Ok(a % b) - } - }); + return try_binary(left, right, |a, b| a.mod_checked_divide_by_zero(b)); } /// Perform `left / right` operation on two arrays. If either left or right value is null @@ -1191,13 +1185,13 @@ pub fn modulus_scalar( ) -> Result> where T: ArrowNumericType, - T::Native: Rem + Zero, + T::Native: ArrowNativeTypeOp, { if modulo.is_zero() { return Err(ArrowError::DivideByZero); } - Ok(unary(array, |a| a % modulo)) + Ok(unary(array, |a| a.mod_wrapping(modulo))) } /// Divide every value in an array by a scalar. If any value in the array is null then the @@ -1769,7 +1763,7 @@ mod tests { } #[test] - fn test_primitive_array_modulus() { + fn test_int_array_modulus() { let a = Int32Array::from(vec![15, 15, 8, 1, 9]); let b = Int32Array::from(vec![5, 6, 8, 9, 1]); let c = modulus(&a, &b).unwrap(); @@ -1780,6 +1774,24 @@ mod tests { assert_eq!(0, c.value(4)); } + #[test] + #[should_panic( + expected = "called `Result::unwrap()` on an `Err` value: DivideByZero" + )] + fn test_int_array_modulus_divide_by_zero() { + let a = Int32Array::from(vec![1]); + let b = Int32Array::from(vec![0]); + modulus(&a, &b).unwrap(); + } + + #[test] + 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] fn test_primitive_array_divide_scalar() { let a = Int32Array::from(vec![15, 14, 9, 8, 1]); @@ -1842,7 +1854,7 @@ mod tests { } #[test] - fn test_primitive_array_modulus_scalar() { + fn test_int_array_modulus_scalar() { let a = Int32Array::from(vec![15, 14, 9, 8, 1]); let b = 3; let c = modulus_scalar(&a, b).unwrap(); @@ -1851,7 +1863,7 @@ mod tests { } #[test] - fn test_primitive_array_modulus_scalar_sliced() { + fn test_int_array_modulus_scalar_sliced() { let a = Int32Array::from(vec![Some(15), None, Some(9), Some(8), None]); let a = a.slice(1, 4); let a = as_primitive_array(&a); @@ -1860,6 +1872,22 @@ mod tests { assert_eq!(actual, expected); } + #[test] + #[should_panic( + expected = "called `Result::unwrap()` on an `Err` value: DivideByZero" + )] + fn test_int_array_modulus_scalar_divide_by_zero() { + let a = Int32Array::from(vec![1]); + modulus_scalar(&a, 0).unwrap(); + } + + #[test] + fn test_int_array_modulus_scalar_overflow_wrapping() { + let a = Int32Array::from(vec![i32::MIN]); + let result = modulus_scalar(&a, -1).unwrap(); + assert_eq!(0, result.value(0)) + } + #[test] fn test_primitive_array_divide_sliced() { let a = Int32Array::from(vec![0, 0, 0, 15, 15, 8, 1, 9, 0]); diff --git a/arrow/src/datatypes/native.rs b/arrow/src/datatypes/native.rs index dec0cc4b53b..cf77d854b8f 100644 --- a/arrow/src/datatypes/native.rs +++ b/arrow/src/datatypes/native.rs @@ -47,7 +47,7 @@ pub(crate) mod native_op { use super::ArrowNativeType; use crate::error::{ArrowError, Result}; use num::Zero; - use std::ops::{Add, Div, Mul, Sub}; + use std::ops::{Add, Div, Mul, Rem, Sub}; /// Trait for ArrowNativeType to provide overflow-checking and non-overflow-checking /// variants for arithmetic operations. For floating point types, this provides some @@ -65,6 +65,7 @@ pub(crate) mod native_op { + Sub + Mul + Div + + Rem + Zero { fn add_checked(self, rhs: Self) -> Result { @@ -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 { + 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 { + if rhs.is_zero() { + Err(ArrowError::DivideByZero) + } else { + Ok(self.mod_wrapping(rhs)) + } + } + + fn mod_wrapping(self, rhs: Self) -> Self { + self % rhs + } } } @@ -163,6 +186,23 @@ macro_rules! native_type_op { fn div_wrapping(self, rhs: Self) -> Self { self.wrapping_div(rhs) } + + fn mod_fully_checked(self, rhs: Self) -> Result { + if rhs.is_zero() { + Err(ArrowError::DivideByZero) + } else { + self.checked_rem(rhs).ok_or_else(|| { + ArrowError::ComputeError(format!( + "Overflow happened on: {:?} % {:?}", + self, rhs + )) + }) + } + } + + fn mod_wrapping(self, rhs: Self) -> Self { + self.wrapping_rem(rhs) + } } }; } From 40f8166c08a8f1251fcf8fcdc5f7b8b0c66451b1 Mon Sep 17 00:00:00 2001 From: remzi <13716567376yh@gmail.com> Date: Sat, 17 Sep 2022 18:15:19 +0800 Subject: [PATCH 2/5] fix simd error Signed-off-by: remzi <13716567376yh@gmail.com> --- arrow/src/compute/kernels/arithmetic.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/arrow/src/compute/kernels/arithmetic.rs b/arrow/src/compute/kernels/arithmetic.rs index 49a8d58481e..b9586c69023 100644 --- a/arrow/src/compute/kernels/arithmetic.rs +++ b/arrow/src/compute/kernels/arithmetic.rs @@ -168,7 +168,7 @@ fn simd_checked_modulus( right: T::Simd, ) -> Result where - T::Native: One + Zero, + T::Native: ArrowNativeTypeOp + One, { let zero = T::init(T::Native::zero()); let one = T::init(T::Native::one()); @@ -291,7 +291,7 @@ fn simd_checked_divide_op( ) -> Result> where T: ArrowNumericType, - T::Native: One + Zero, + T::Native: ArrowNativeTypeOp, SI: Fn(Option, T::Simd, T::Simd) -> Result, SC: Fn(T::Native, T::Native) -> T::Native, { @@ -1075,11 +1075,11 @@ pub fn modulus( ) -> Result> where T: ArrowNumericType, - T::Native: ArrowNativeTypeOp, + T::Native: ArrowNativeTypeOp + One, { #[cfg(feature = "simd")] return simd_checked_divide_op(&left, &right, simd_checked_modulus::, |a, b| { - a % b + a.mod_wrapping(b) }); #[cfg(not(feature = "simd"))] return try_binary(left, right, |a, b| a.mod_checked_divide_by_zero(b)); From ac7dbb5bb60c05f382f1f05d2031702e9089c464 Mon Sep 17 00:00:00 2001 From: remzi <13716567376yh@gmail.com> Date: Sun, 18 Sep 2022 10:46:37 +0800 Subject: [PATCH 3/5] remove_mod_divide_by_zero Signed-off-by: remzi <13716567376yh@gmail.com> --- arrow/src/compute/kernels/arithmetic.rs | 8 +++++++- arrow/src/datatypes/native.rs | 14 ++------------ 2 files changed, 9 insertions(+), 13 deletions(-) diff --git a/arrow/src/compute/kernels/arithmetic.rs b/arrow/src/compute/kernels/arithmetic.rs index b9586c69023..600b4daf901 100644 --- a/arrow/src/compute/kernels/arithmetic.rs +++ b/arrow/src/compute/kernels/arithmetic.rs @@ -1082,7 +1082,13 @@ where a.mod_wrapping(b) }); #[cfg(not(feature = "simd"))] - return try_binary(left, right, |a, b| a.mod_checked_divide_by_zero(b)); + return try_binary(left, right, |a, b| { + if b.is_zero() { + Err(ArrowError::DivideByZero) + } else { + Ok(a.mod_wrapping(b)) + } + }); } /// Perform `left / right` operation on two arrays. If either left or right value is null diff --git a/arrow/src/datatypes/native.rs b/arrow/src/datatypes/native.rs index cf77d854b8f..7ed0d37c891 100644 --- a/arrow/src/datatypes/native.rs +++ b/arrow/src/datatypes/native.rs @@ -104,17 +104,7 @@ pub(crate) mod native_op { self / rhs } - /// Check `DivideByZero` error and `Overflow` error. - fn mod_fully_checked(self, rhs: Self) -> Result { - 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 { + fn mod_checked(self, rhs: Self) -> Result { if rhs.is_zero() { Err(ArrowError::DivideByZero) } else { @@ -187,7 +177,7 @@ macro_rules! native_type_op { self.wrapping_div(rhs) } - fn mod_fully_checked(self, rhs: Self) -> Result { + fn mod_checked(self, rhs: Self) -> Result { if rhs.is_zero() { Err(ArrowError::DivideByZero) } else { From efa2c74d898f1c4027fb19922fdb185008b10dc5 Mon Sep 17 00:00:00 2001 From: remzi <13716567376yh@gmail.com> Date: Thu, 22 Sep 2022 21:11:28 +0800 Subject: [PATCH 4/5] overflow panic simd Signed-off-by: remzi <13716567376yh@gmail.com> --- arrow/src/compute/kernels/arithmetic.rs | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/arrow/src/compute/kernels/arithmetic.rs b/arrow/src/compute/kernels/arithmetic.rs index 600b4daf901..f917d83d398 100644 --- a/arrow/src/compute/kernels/arithmetic.rs +++ b/arrow/src/compute/kernels/arithmetic.rs @@ -1079,7 +1079,7 @@ where { #[cfg(feature = "simd")] return simd_checked_divide_op(&left, &right, simd_checked_modulus::, |a, b| { - a.mod_wrapping(b) + a % b }); #[cfg(not(feature = "simd"))] return try_binary(left, right, |a, b| { @@ -1791,6 +1791,7 @@ mod tests { } #[test] + #[cfg(not(feature = "simd"))] fn test_int_array_modulus_overflow_wrapping() { let a = Int32Array::from(vec![i32::MIN]); let b = Int32Array::from(vec![-1]); @@ -1798,6 +1799,15 @@ mod tests { assert_eq!(0, result.value(0)) } + #[test] + #[cfg(feature = "simd")] + #[should_panic(expected = "attempt to calculate the remainder with overflow")] + fn test_int_array_modulus_overflow_panic() { + let a = Int32Array::from(vec![i32::MIN]); + let b = Int32Array::from(vec![-1]); + let _ = modulus(&a, &b).unwrap(); + } + #[test] fn test_primitive_array_divide_scalar() { let a = Int32Array::from(vec![15, 14, 9, 8, 1]); From 5680942333e47b80b28f4e2dbcdf5732ad5cfc0c Mon Sep 17 00:00:00 2001 From: remzi <13716567376yh@gmail.com> Date: Fri, 30 Sep 2022 15:40:57 +0800 Subject: [PATCH 5/5] address comment Signed-off-by: remzi <13716567376yh@gmail.com> --- arrow/src/datatypes/native.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arrow/src/datatypes/native.rs b/arrow/src/datatypes/native.rs index 7c3f4e5ef55..654b939500a 100644 --- a/arrow/src/datatypes/native.rs +++ b/arrow/src/datatypes/native.rs @@ -87,7 +87,7 @@ pub(crate) mod native_op { if rhs.is_zero() { Err(ArrowError::DivideByZero) } else { - Ok(self.mod_wrapping(rhs)) + Ok(self % rhs) } }