andygrove commented on code in PR #5082:
URL: https://github.com/apache/datafusion-comet/pull/5082#discussion_r3692487166
##########
native/spark-expr/src/math_funcs/round.rs:
##########
@@ -280,4 +325,102 @@ mod test {
assert_eq!(result, 125.23);
Ok(())
}
+
+ // Regression tests for
https://github.com/apache/datafusion-comet/issues/5070:
+ // round(Int64, scale) where `10^(-scale)` does not fit in i64. For
scale=-19,
+ // values with |x| >= 5e18 round to sign(x)*1e19, which does not fit in a
long:
+ // Spark throws under ANSI and wraps (low-order 64 bits) under legacy.
+
+ // 1e19 truncated to the low 64 bits, matching `BigDecimal.longValue`.
+ const WRAPPED_POS_1E19: i64 = 10_000_000_000_000_000_000u64 as i64;
+ const WRAPPED_NEG_1E19: i64 = -WRAPPED_POS_1E19;
Review Comment:
Agreed, the name asserted a sign that does not hold. Renamed to
`WRAPPED_1E19` / `WRAPPED_MINUS_1E19` in 285a211bd, with a comment spelling out
that the value is negative (-8446744073709551616) because 1e19 exceeds
`i64::MAX`, and that rounding -5e18 therefore wraps to a *positive* value.
##########
native/spark-expr/src/math_funcs/round.rs:
##########
@@ -280,4 +325,102 @@ mod test {
assert_eq!(result, 125.23);
Ok(())
}
+
+ // Regression tests for
https://github.com/apache/datafusion-comet/issues/5070:
+ // round(Int64, scale) where `10^(-scale)` does not fit in i64. For
scale=-19,
+ // values with |x| >= 5e18 round to sign(x)*1e19, which does not fit in a
long:
+ // Spark throws under ANSI and wraps (low-order 64 bits) under legacy.
+
+ // 1e19 truncated to the low 64 bits, matching `BigDecimal.longValue`.
+ const WRAPPED_POS_1E19: i64 = 10_000_000_000_000_000_000u64 as i64;
+ const WRAPPED_NEG_1E19: i64 = -WRAPPED_POS_1E19;
+
+ fn assert_round_int64_ansi_overflows(value: ColumnarValue) {
+ let args = vec![value,
ColumnarValue::Scalar(ScalarValue::Int64(Some(-19)))];
+ let err = spark_round(&args, &DataType::Int64, true).unwrap_err();
+ assert!(
+ err.to_string().to_ascii_lowercase().contains("overflow"),
+ "expected arithmetic overflow error, got: {err}"
+ );
+ }
+
+ #[test]
+ fn test_round_int64_negative_scale_overflow_ansi() {
+ // 5e18 rounds up to 1e19, which overflows i64.
+
assert_round_int64_ansi_overflows(ColumnarValue::Array(Arc::new(Int64Array::from(vec![
+ 5_000_000_000_000_000_000i64,
+ ]))));
+ }
+
+ #[test]
+ fn test_round_int64_negative_scale_overflow_ansi_scalar() {
+
assert_round_int64_ansi_overflows(ColumnarValue::Scalar(ScalarValue::Int64(Some(
+ 5_000_000_000_000_000_000i64,
+ ))));
+ }
Review Comment:
Done in 285a211bd — both ANSI tests (array and scalar) now loop over `[5e18,
-5e18]`, so the negative side is covered in ANSI mode as well as legacy.
##########
native/spark-expr/src/math_funcs/round.rs:
##########
@@ -280,4 +325,102 @@ mod test {
assert_eq!(result, 125.23);
Ok(())
}
+
+ // Regression tests for
https://github.com/apache/datafusion-comet/issues/5070:
+ // round(Int64, scale) where `10^(-scale)` does not fit in i64. For
scale=-19,
+ // values with |x| >= 5e18 round to sign(x)*1e19, which does not fit in a
long:
+ // Spark throws under ANSI and wraps (low-order 64 bits) under legacy.
+
+ // 1e19 truncated to the low 64 bits, matching `BigDecimal.longValue`.
+ const WRAPPED_POS_1E19: i64 = 10_000_000_000_000_000_000u64 as i64;
+ const WRAPPED_NEG_1E19: i64 = -WRAPPED_POS_1E19;
+
+ fn assert_round_int64_ansi_overflows(value: ColumnarValue) {
+ let args = vec![value,
ColumnarValue::Scalar(ScalarValue::Int64(Some(-19)))];
+ let err = spark_round(&args, &DataType::Int64, true).unwrap_err();
+ assert!(
+ err.to_string().to_ascii_lowercase().contains("overflow"),
+ "expected arithmetic overflow error, got: {err}"
+ );
+ }
+
+ #[test]
+ fn test_round_int64_negative_scale_overflow_ansi() {
+ // 5e18 rounds up to 1e19, which overflows i64.
+
assert_round_int64_ansi_overflows(ColumnarValue::Array(Arc::new(Int64Array::from(vec![
+ 5_000_000_000_000_000_000i64,
+ ]))));
+ }
+
+ #[test]
+ fn test_round_int64_negative_scale_overflow_ansi_scalar() {
+
assert_round_int64_ansi_overflows(ColumnarValue::Scalar(ScalarValue::Int64(Some(
+ 5_000_000_000_000_000_000i64,
+ ))));
+ }
+
+ #[test]
+ fn test_round_int64_negative_scale_overflow_legacy() -> Result<()> {
+ // Under legacy mode, ±1e19 wraps to its low-order 64 bits.
+ let args = vec![
+ ColumnarValue::Array(Arc::new(Int64Array::from(vec![
+ 5_000_000_000_000_000_000i64,
+ -5_000_000_000_000_000_000i64,
+ 4_999_999_999_999_999_999i64,
+ 0i64,
+ i64::MAX,
+ i64::MIN,
+ ]))),
+ ColumnarValue::Scalar(ScalarValue::Int64(Some(-19))),
+ ];
+ let ColumnarValue::Array(result) = spark_round(&args,
&DataType::Int64, false)? else {
+ unreachable!()
+ };
+ let longs = as_int64_array(&result)?;
+ let expected = Int64Array::from(vec![
+ WRAPPED_POS_1E19,
+ WRAPPED_NEG_1E19,
+ 0i64,
+ 0i64,
+ WRAPPED_POS_1E19,
+ WRAPPED_NEG_1E19,
+ ]);
+ assert_eq!(longs, &expected);
+ Ok(())
+ }
+
+ #[test]
+ fn test_round_int64_negative_scale_below_threshold() -> Result<()> {
+ // scale=-20: threshold is 5e19, which exceeds i64::MAX, so every long
+ // rounds to 0 under both ANSI and legacy.
+ let arr = Int64Array::from(vec![i64::MAX, i64::MIN, 0,
1_000_000_000_000_000_000]);
+ for fail_on_error in [false, true] {
+ let args = vec![
+ ColumnarValue::Array(Arc::new(arr.clone())),
+ ColumnarValue::Scalar(ScalarValue::Int64(Some(-20))),
+ ];
+ let ColumnarValue::Array(result) = spark_round(&args,
&DataType::Int64, fail_on_error)?
+ else {
+ unreachable!()
+ };
+ let longs = as_int64_array(&result)?;
+ assert_eq!(longs, &Int64Array::from(vec![0i64; arr.len()]));
+ }
+ Ok(())
+ }
+
+ #[test]
+ fn test_round_int64_negative_scale_legacy_scalar() -> Result<()> {
+ let args = vec![
+
ColumnarValue::Scalar(ScalarValue::Int64(Some(5_000_000_000_000_000_000i64))),
+ ColumnarValue::Scalar(ScalarValue::Int64(Some(-19))),
+ ];
+ let ColumnarValue::Scalar(ScalarValue::Int64(Some(result))) =
+ spark_round(&args, &DataType::Int64, false)?
+ else {
+ unreachable!()
+ };
+ assert_eq!(result, WRAPPED_POS_1E19);
+ Ok(())
+ }
Review Comment:
Done in 285a211bd, as `test_round_int64_negative_scale_null_scalar`. It goes
a bit wider than suggested: it asserts null-in/null-out at scale -9 (`10^9`
fits in i64), -19 (fits only in i128) and -40 (fits in neither), each under
both eval modes, so all three bands are pinned. The -40 case is the one that
was actually broken — see the fix on your line 143 comment.
##########
native/spark-expr/src/math_funcs/round.rs:
##########
@@ -68,7 +91,14 @@ macro_rules! round_integer_array {
arrow::compute::kernels::arity::try_unary(array, |x| {
integer_round!(x, div, half, $FAIL_ON_ERROR)
})?
+ } else if let Some(div) = 10_i128.checked_pow((-(*$POINT)) as u32) {
+ let half = div / 2;
+ arrow::compute::kernels::arity::try_unary(array, |x| {
+ integer_round_widened!(x, div, half, $NATIVE, $FAIL_ON_ERROR)
+ })?
} else {
+ // Even i128 cannot hold 10^(-point); every bounded native
+ // integer rounds to 0.
arrow::compute::kernels::arity::try_unary(array, |_| Ok(0))?
Review Comment:
Done in 285a211bd, as `test_round_int64_scale_below_i128_range` at scale
-40, asserting `0` for `i64::MAX`, `i64::MIN`, `0` and `5e18` under both eval
modes, for both the array and scalar paths. Together with the existing
scale=-20 test and the new scale=-19 tests, all three bands and both boundaries
between them now have coverage.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
