andygrove commented on code in PR #5082:
URL: https://github.com/apache/datafusion-comet/pull/5082#discussion_r3692482336
##########
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:
Good catch — this was a real null-handling bug. Fixed in 285a211bd, though
not quite as suggested: rather than repeat the `Some(0)`/`None` match in this
third branch, `round_integer_scalar!` now checks for a null input *once*, up
front, and only dispatches on the scale band for non-null values. That way all
three bands preserve null by construction instead of each having to remember
to. Covered by the new `test_round_int64_negative_scale_null_scalar`, which
asserts null-in/null-out at scales -9, -19 and -40 under both eval modes.
##########
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:
Done in 285a211bd: `Ok((if x128 >= $HALF { $DIV } else { -$DIV }) as
$NATIVE)`.
##########
native/spark-expr/src/math_funcs/round.rs:
##########
Review Comment:
Done in 285a211bd — hoisted to `let point_abs = (-(*$POINT)) as u32;` in
both `round_integer_array!` and `round_integer_scalar!`, and used for both
`checked_pow` calls.
##########
native/spark-expr/src/math_funcs/round.rs:
##########
@@ -93,6 +123,21 @@ macro_rules! round_integer_scalar {
None => None,
};
Review Comment:
Done in 285a211bd, via `round_scalar_result!`. The duplication actually
shrank further than suggested: because the null check moved out in front of the
band dispatch (see your comment on line 143), each band is now a single
`round_scalar_result!(...)` expression rather than a full `match`.
--
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]
