From 22b31d192c4a47b631ab4f6990b2c81a22b00a9e Mon Sep 17 00:00:00 2001 From: Kirk Mitchener Date: Fri, 16 Sep 2022 10:33:13 -0400 Subject: [PATCH 1/3] fix divide by zero for decimal, add tests --- .../src/expressions/binary/kernels_arrow.rs | 52 ++++++++++++++++--- 1 file changed, 45 insertions(+), 7 deletions(-) diff --git a/datafusion/physical-expr/src/expressions/binary/kernels_arrow.rs b/datafusion/physical-expr/src/expressions/binary/kernels_arrow.rs index e99fbf0c9630c..0d3e1a4392858 100644 --- a/datafusion/physical-expr/src/expressions/binary/kernels_arrow.rs +++ b/datafusion/physical-expr/src/expressions/binary/kernels_arrow.rs @@ -378,6 +378,9 @@ pub(crate) fn divide_decimal( ) -> Result { let mul = 10_f64.powi(left.scale() as i32); let array = arith_decimal(left, right, |left, right| { + if right == 0 { + return Err(DataFusionError::ArrowError(ArrowError::DivideByZero)); + } let l_value = left as f64; let r_value = right as f64; let result = ((l_value / r_value) * mul) as i128; @@ -393,6 +396,9 @@ pub(crate) fn divide_decimal_scalar( ) -> Result { let mul = 10_f64.powi(left.scale() as i32); let array = arith_decimal_scalar(left, right, |left, right| { + if right == 0 { + return Err(DataFusionError::ArrowError(ArrowError::DivideByZero)); + } let l_value = left as f64; let r_value = right as f64; let result = ((l_value / r_value) * mul) as i128; @@ -615,34 +621,66 @@ mod tests { assert_eq!(expect, result); // divide let left_decimal_array = create_decimal_array( - &[Some(1234567), None, Some(1234567), Some(1234567)], + &[ + Some(1234567), + None, + Some(1234567), + Some(1234567), + Some(1234567), + ], + 25, + 3, + ); + let right_decimal_array = create_decimal_array( + &[Some(10), Some(100), Some(55), Some(-123), None], 25, 3, ); - let right_decimal_array = - create_decimal_array(&[Some(10), Some(100), Some(55), Some(-123)], 25, 3); let result = divide_decimal(&left_decimal_array, &right_decimal_array)?; let expect = create_decimal_array( - &[Some(123456700), None, Some(22446672), Some(-10037130)], + &[Some(123456700), None, Some(22446672), Some(-10037130), None], 25, 3, ); assert_eq!(expect, result); let result = divide_decimal_scalar(&left_decimal_array, 10)?; let expect = create_decimal_array( - &[Some(123456700), None, Some(123456700), Some(123456700)], + &[ + Some(123456700), + None, + Some(123456700), + Some(123456700), + Some(123456700), + ], 25, 3, ); assert_eq!(expect, result); // modulus let result = modulus_decimal(&left_decimal_array, &right_decimal_array)?; - let expect = create_decimal_array(&[Some(7), None, Some(37), Some(16)], 25, 3); + let expect = + create_decimal_array(&[Some(7), None, Some(37), Some(16), None], 25, 3); assert_eq!(expect, result); let result = modulus_decimal_scalar(&left_decimal_array, 10)?; - let expect = create_decimal_array(&[Some(7), None, Some(7), Some(7)], 25, 3); + let expect = + create_decimal_array(&[Some(7), None, Some(7), Some(7), Some(7)], 25, 3); assert_eq!(expect, result); Ok(()) } + + #[test] + fn arithmetic_decimal_divide_by_zero() { + let left_decimal_array = create_decimal_array(&[Some(101)], 10, 1); + let right_decimal_array = create_decimal_array(&[Some(0)], 10, 1); + + let _result = divide_decimal(&left_decimal_array, &right_decimal_array) + .expect_err("expecting DivideByZero error"); + let _result = divide_decimal_scalar(&left_decimal_array, 0) + .expect_err("expecting DivideByZero error"); + let _result = modulus_decimal(&left_decimal_array, &right_decimal_array) + .expect_err("expecting DivideByZero error"); + let _result = modulus_decimal_scalar(&left_decimal_array, 0) + .expect_err("expecting DivideByZero error"); + } } From e455c7b57debb79488b3e289e906a0620e649764 Mon Sep 17 00:00:00 2001 From: Kirk Mitchener Date: Fri, 16 Sep 2022 15:23:09 -0400 Subject: [PATCH 2/3] check for actual error strings --- .../src/expressions/binary/kernels_arrow.rs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/datafusion/physical-expr/src/expressions/binary/kernels_arrow.rs b/datafusion/physical-expr/src/expressions/binary/kernels_arrow.rs index 0d3e1a4392858..5d0e061cbbefd 100644 --- a/datafusion/physical-expr/src/expressions/binary/kernels_arrow.rs +++ b/datafusion/physical-expr/src/expressions/binary/kernels_arrow.rs @@ -674,13 +674,13 @@ mod tests { let left_decimal_array = create_decimal_array(&[Some(101)], 10, 1); let right_decimal_array = create_decimal_array(&[Some(0)], 10, 1); - let _result = divide_decimal(&left_decimal_array, &right_decimal_array) - .expect_err("expecting DivideByZero error"); - let _result = divide_decimal_scalar(&left_decimal_array, 0) - .expect_err("expecting DivideByZero error"); - let _result = modulus_decimal(&left_decimal_array, &right_decimal_array) - .expect_err("expecting DivideByZero error"); - let _result = modulus_decimal_scalar(&left_decimal_array, 0) - .expect_err("expecting DivideByZero error"); + let err = divide_decimal(&left_decimal_array, &right_decimal_array).unwrap_err(); + assert_eq!("Arrow error: Divide by zero error", err.to_string()); + let err = divide_decimal_scalar(&left_decimal_array, 0).unwrap_err(); + assert_eq!("Arrow error: Divide by zero error", err.to_string()); + let err = modulus_decimal(&left_decimal_array, &right_decimal_array).unwrap_err(); + assert_eq!("Arrow error: Divide by zero error", err.to_string()); + let err = modulus_decimal_scalar(&left_decimal_array, 0).unwrap_err(); + assert_eq!("Arrow error: Divide by zero error", err.to_string()); } } From 3ddffe029cf04f60bb3b4d05af04c5955576797e Mon Sep 17 00:00:00 2001 From: Kirk Mitchener Date: Fri, 16 Sep 2022 17:27:37 -0400 Subject: [PATCH 3/3] move scalar divide by zero check out of the loop. fix precision in test. --- .../physical-expr/src/expressions/binary/kernels_arrow.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/datafusion/physical-expr/src/expressions/binary/kernels_arrow.rs b/datafusion/physical-expr/src/expressions/binary/kernels_arrow.rs index 5d0e061cbbefd..8caed7191fb28 100644 --- a/datafusion/physical-expr/src/expressions/binary/kernels_arrow.rs +++ b/datafusion/physical-expr/src/expressions/binary/kernels_arrow.rs @@ -394,11 +394,11 @@ pub(crate) fn divide_decimal_scalar( left: &Decimal128Array, right: i128, ) -> Result { + if right == 0 { + return Err(DataFusionError::ArrowError(ArrowError::DivideByZero)); + } let mul = 10_f64.powi(left.scale() as i32); let array = arith_decimal_scalar(left, right, |left, right| { - if right == 0 { - return Err(DataFusionError::ArrowError(ArrowError::DivideByZero)); - } let l_value = left as f64; let r_value = right as f64; let result = ((l_value / r_value) * mul) as i128; @@ -672,7 +672,7 @@ mod tests { #[test] fn arithmetic_decimal_divide_by_zero() { let left_decimal_array = create_decimal_array(&[Some(101)], 10, 1); - let right_decimal_array = create_decimal_array(&[Some(0)], 10, 1); + let right_decimal_array = create_decimal_array(&[Some(0)], 1, 1); let err = divide_decimal(&left_decimal_array, &right_decimal_array).unwrap_err(); assert_eq!("Arrow error: Divide by zero error", err.to_string());