peterxcli commented on code in PR #5044:
URL: https://github.com/apache/datafusion-comet/pull/5044#discussion_r3738633835
##########
native/spark-expr/src/math_funcs/internal/decimal_rescale_check.rs:
##########
@@ -194,16 +199,17 @@ impl PhysicalExpr for DecimalRescaleCheckOverflow {
let result: Decimal128Array =
arrow::compute::kernels::arity::try_unary(decimal_array,
|value| {
- rescale_and_check(value, delta, scale_factor, bound,
fail_on_error)
+ rescale_and_check(
+ value,
+ delta,
+ scale_factor,
+ bound,
+ fail_on_error,
+ &overflowed,
+ )
})?;
- let result = if !fail_on_error &&
result.values().contains(&i128::MAX) {
- // The rescale pass writes i128::MAX as an overflow
sentinel for values that
- // do not fit the output precision. Only when a sentinel
is present do we need
- // the extra null-masking pass (which allocates a new
array); `contains`
- // short-circuits at the first sentinel, so the common
no-overflow case skips
- // that allocation entirely. ANSI mode raises on overflow
and never produces a
- // sentinel, so it also skips this pass.
Review Comment:
https://github.com/apache/datafusion-comet/pull/5044/changes/BASE..07944c8a52cc3aea6072d8a2fb5cd228010a4b26#r3728643261
> 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.
I guess this comment should be here.
--
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]
