andygrove commented on code in PR #5044:
URL: https://github.com/apache/datafusion-comet/pull/5044#discussion_r3728643269
##########
native/spark-expr/src/math_funcs/wide_decimal_binary_expr.rs:
##########
@@ -329,20 +331,22 @@ impl PhysicalExpr for WideDecimalBinaryExpr {
}
/// Check if the i256 result fits in the output precision. In Ansi mode,
return an error
-/// on overflow. In Legacy/Try mode, return i128::MAX as a sentinel value that
will be
-/// nullified by `null_if_overflow_precision`.
+/// on overflow. In Legacy/Try mode, record the overflow and return i128::MAX
as a sentinel
+/// value that will be nullified by `null_if_overflow_precision`.
#[inline]
fn check_overflow_and_convert(
result: i256,
bound: i256,
neg_bound: i256,
eval_mode: EvalMode,
+ overflowed: &Cell<bool>,
) -> Result<i128, ArrowError> {
if result > bound || result < neg_bound {
if eval_mode == EvalMode::Ansi {
return Err(ArrowError::ComputeError("Arithmetic
overflow".to_string()));
}
// Sentinel value — will be nullified by null_if_overflow_precision
+ overflowed.set(true);
Review Comment:
Now that the flag records the overflow, the `i128::MAX` sentinel and the
masking pass it feeds are both redundant. Would you be up for filing a
follow-up issue to drop them and write the null bit directly at the point the
overflow is detected? That would remove the extra pass and the `Cell` side
effect, which is what is costing the 3% to 5% on the overflow shapes.
Clearly out of scope here. I would just like it tracked rather than left
implicit, and a link to the issue in this thread would be enough.
##########
native/spark-expr/src/math_funcs/wide_decimal_binary_expr.rs:
##########
@@ -276,12 +278,12 @@ impl PhysicalExpr for WideDecimalBinaryExpr {
} else {
raw
};
- check_overflow_and_convert(result, bound, neg_bound,
eval_mode)
+ check_overflow_and_convert(result, bound, neg_bound,
eval_mode, &overflowed)
})?
}
};
- let result = if eval_mode != EvalMode::Ansi {
+ let result = if overflowed.get() {
Review Comment:
This dropped the only comment in the codebase explaining why skipping the
masking pass is safe. The six lines removed from `decimal_rescale_check.rs` are
gone, and neither guard has a replacement, so both call sites are now a bare
`if overflowed.get()`.
The invariant is not obvious from reading the guard. It rests on the fact
that every non-sentinel value the closure returns is already clamped inside
`±(10^p_out - 1)`, which is exactly the set `null_if_overflow_precision` leaves
untouched, so the pass can only ever null sentinels. It is also worth saying
that ANSI mode returns `Err` before setting the flag, so ANSI still skips the
pass for the same reason it did before.
Could we add that back at both sites, here and at
`decimal_rescale_check.rs:212`? We worked the argument out in this thread, and
it would be good for it to live in the code rather than only in the PR
conversation.
--
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]
