mbutrovich commented on code in PR #5082:
URL: https://github.com/apache/datafusion-comet/pull/5082#discussion_r3691532127
##########
native/spark-expr/src/math_funcs/round.rs:
##########
Review Comment:
round.rs:89, 94, 111, 126
`(-(*$POINT)) as u32` is written out twice in each macro: once for the
native `checked_pow` and again for the `i128` `checked_pow`. Fix: compute it
once, e.g. `let point_abs = (-(*$POINT)) as u32;` right after `let ten: $NATIVE
= 10;`, and reuse it in both `checked_pow` calls in `round_integer_array!` and
`round_integer_scalar!`.
##########
native/spark-expr/src/math_funcs/round.rs:
##########
@@ -93,6 +123,21 @@ macro_rules! round_integer_scalar {
None => None,
};
Review Comment:
round.rs:113-124, 128-139
The native-fits and widened branches of `round_integer_scalar!` each repeat
the identical `match ... { Ok(v) => Some(v), Err(e) => return
Err(DataFusionError::ArrowError(Box::from(e),
Some(DataFusionError::get_back_trace()))) }` boilerplate, differing only in
which inner macro is invoked. Fix: factor this into a small helper macro, e.g.
`unwrap_round_result!($RESULT)` expanding to that same match, and call
`Some(unwrap_round_result!(integer_round!(...)))` /
`Some(unwrap_round_result!(integer_round_widened!(...)))` in each branch.
##########
native/spark-expr/src/math_funcs/round.rs:
##########
@@ -59,6 +59,29 @@ macro_rules! integer_round {
}};
}
+// Round a single native integer when `10^(-point)` does not fit in the native
+// integer type but still fits in i128. The caller has already excluded the
+// case where `div` fits the native type, so `|x| < div` and the result is
+// either `0` (when `|x| < half`) or `sign(x) * div` — the latter always
+// overflows the native type. Under ANSI we throw, under legacy we wrap by
+// truncation (matches `BigDecimal.longValue`'s low-64-bit semantics).
+macro_rules! integer_round_widened {
+ ($X:expr, $DIV:expr, $HALF:expr, $NATIVE:ty, $FAIL_ON_ERROR:expr) => {{
+ let x128 = $X as i128;
+ if x128 > -$HALF && x128 < $HALF {
+ Ok(0 as $NATIVE)
+ } else if $FAIL_ON_ERROR {
+ Err(ArrowError::ComputeError(
+ arithmetic_overflow_error("integer").to_string(),
+ ))
+ } else if x128 >= $HALF {
+ Ok($DIV as $NATIVE)
+ } else {
+ Ok((-$DIV) as $NATIVE)
+ }
+ }};
+}
Review Comment:
The final two arms of `integer_round_widened!` (`x128 >= $HALF -> Ok($DIV as
$NATIVE)`, else `Ok((-$DIV) as $NATIVE)`) differ only by sign, and both apply
only once `$FAIL_ON_ERROR` has already been ruled out above. Fix: collapse to
one arm: `Ok((if x128 >= $HALF { $DIV } else { -$DIV }) as $NATIVE)`. Same
behavior, one fewer branch.
##########
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:
`const WRAPPED_POS_1E19: i64 = 10_000_000_000_000_000_000u64 as i64;` is
named as if it holds a positive value, but the truncating cast actually
produces a negative number: `1e19 mod 2^64` is less than `2^64` but greater
than `i64::MAX`, so reinterpreting those bits as `i64` yields
`-8446744073709551616`. The name implies the wrapped result of rounding `+5e18`
up to `+1e19` stays positive, which is false and will mislead anyone reading
the test without independently working out the two's-complement truncation.
Fix: rename to something that does not assert a sign, e.g. `WRAPPED_1E19`, and
add a comment stating the wrapped value is negative (`-8446744073709551616`)
because `1e19` exceeds `i64::MAX`.
##########
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:
Both new ANSI tests (`test_round_int64_negative_scale_overflow_ansi`,
`test_round_int64_negative_scale_overflow_ansi_scalar`) only exercise the
positive-overflow case (`+5e18`). There is no ANSI-mode test for the negative
side (`-5e18`), even though the legacy test at lines 362-390 covers both signs.
Fix: extend `assert_round_int64_ansi_overflows` calls (or add a third test) to
also assert ANSI overflow for `-5_000_000_000_000_000_000i64`.
##########
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:
The i128-overflow fallback (`scale <= -38` for `Int64`) is untouched by this
PR but is now reached through a three-way branch instead of two, and the PR's
own reasoning documents the boundary between "fits native" and "fits i128"
carefully while leaving this third boundary undocumented by a test. The
existing `test_round_int64_negative_scale_below_threshold` test (scale=-20)
exercises the "fits i128 but rounds to 0" case, not the "doesn't even fit i128"
case. Fix: add a test at `scale = -40` (or any scale where
`10_i128.checked_pow` returns `None`) asserting the result is `0` under both
modes, to pin down this boundary now that it sits behind a new middle branch.
##########
native/spark-expr/src/math_funcs/round.rs:
##########
@@ -59,6 +59,29 @@ macro_rules! integer_round {
}};
}
+// Round a single native integer when `10^(-point)` does not fit in the native
+// integer type but still fits in i128. The caller has already excluded the
+// case where `div` fits the native type, so `|x| < div` and the result is
+// either `0` (when `|x| < half`) or `sign(x) * div` — the latter always
+// overflows the native type. Under ANSI we throw, under legacy we wrap by
+// truncation (matches `BigDecimal.longValue`'s low-64-bit semantics).
+macro_rules! integer_round_widened {
+ ($X:expr, $DIV:expr, $HALF:expr, $NATIVE:ty, $FAIL_ON_ERROR:expr) => {{
+ let x128 = $X as i128;
+ if x128 > -$HALF && x128 < $HALF {
+ Ok(0 as $NATIVE)
+ } else if $FAIL_ON_ERROR {
+ Err(ArrowError::ComputeError(
+ arithmetic_overflow_error("integer").to_string(),
+ ))
+ } else if x128 >= $HALF {
+ Ok($DIV as $NATIVE)
+ } else {
+ Ok((-$DIV) as $NATIVE)
+ }
+ }};
+}
Review Comment:
`integer_round_widened!` is a second, separate rounding macro alongside
`integer_round!`, correct only because of an unstated invariant: `div`
overflowed the native type, so `|x| <= NATIVE::MAX < div`, hence `x % div ==
x`. That's a narrow algebraic shortcut for this specific overflow band, not the
same mechanism `decimal_round_f` uses one function below it (lines 232-247),
which always computes in `i128` regardless of how large `div` is and needs no
special-cased middle branch. Fix: generalize `integer_round!`'s rem/half
formula to operate in `i128` uniformly, with a single checked-or-wrapping
downcast to `$NATIVE` at the end, replacing the current three-way dispatch
(native-fits / i128-widened / i128-overflows) with one code path. This removes
`integer_round_widened!` entirely and matches the pattern already established
in this same file for decimals.
##########
native/spark-expr/src/math_funcs/round.rs:
##########
@@ -93,6 +123,21 @@ macro_rules! round_integer_scalar {
None => None,
};
Ok(ColumnarValue::Scalar($TYPE(scalar_opt)))
+ } else if let Some(div) = 10_i128.checked_pow((-(*$POINT)) as u32) {
+ let half = div / 2;
+ let scalar_opt = match $SCALAR {
+ Some(x) => match integer_round_widened!(*x, div, half,
$NATIVE, $FAIL_ON_ERROR) {
+ Ok(v) => Some(v),
+ Err(e) => {
+ return Err(DataFusionError::ArrowError(
+ Box::from(e),
+ Some(DataFusionError::get_back_trace()),
+ ))
+ }
+ },
+ None => None,
+ };
+ Ok(ColumnarValue::Scalar($TYPE(scalar_opt)))
} else {
Ok(ColumnarValue::Scalar($TYPE(Some(0))))
}
Review Comment:
This is the pre-existing catch-all branch of `round_integer_scalar!` for
when even `i128` cannot hold `10^(-scale)` (roughly `scale <= -39`). It returns
`Ok(ColumnarValue::Scalar($TYPE(Some(0))))` unconditionally, without checking
whether `$SCALAR` was `None`. A null `Int64` scalar input at `scale <= -39`
comes out as `Some(0)` instead of `None`. This predates the PR, but the PR
inserts a new branch directly above it (lines 126-140) that gets this exact
case right (`match $SCALAR { Some(x) => ..., None => None }`), so the
inconsistency is now more visible, and it's a one-line fix while this exact
function is already being reworked. Fix: match on `$SCALAR` here too, e.g. `let
scalar_opt = match $SCALAR { Some(_) => Some(0), None => None };
Ok(ColumnarValue::Scalar($TYPE(scalar_opt)))`.
##########
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:
None of the six new tests pass a null `Int64` scalar through the widened
path (`scale <= -19`) or the i128-overflow fallback (`scale <= -38`). A test
with `ColumnarValue::Scalar(ScalarValue::Int64(None))` at `scale = -19` would
confirm the widened-scalar branch preserves null (it does, by inspection of
lines 126-140), and the same test at `scale = -40` would catch the
null-handling bug at lines 141-143. Fix: add
`test_round_int64_negative_scale_null_scalar` covering both scale bands.
--
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]
