perf: track decimal overflow without rescanning results

[peterxcli]

Which issue does this PR close?

Closes #4943.

Rationale for this change

WideDecimalBinaryExpr previously allocated a null-masked result for every non-ANSI batch, even when nothing overflowed. DecimalRescaleCheckOverflow avoided that allocation but still scanned the completed output buffer for an overflow sentinel.

Both expressions already know when overflow occurs while evaluating each value. Recording that state during evaluation avoids rescanning the result and skips null masking entirely for no-overflow batches.

What changes are included in this PR?

• Track overflow during WideDecimalBinaryExpr binary arithmetic using a per-evaluation Cell<bool>.
• Apply the same approach to DecimalRescaleCheckOverflow unary evaluation.
• Run the allocating null-masking pass only when overflow actually occurs.
• Add Scalar × Scalar legacy-overflow regression coverage.
• Add Criterion cases covering no overflow, nulls, overflow density, overflow at the end of a batch, and ANSI mode.

How are these changes tested?

• cargo test --manifest-path native/Cargo.toml -p datafusion-comet-spark-expr wide_decimal_binary_expr
• cargo test --manifest-path native/Cargo.toml -p datafusion-comet-spark-expr decimal_rescale_check
• cargo clippy --manifest-path native/Cargo.toml -p datafusion-comet-spark-expr --all-targets -- -D warnings
• cargo check --manifest-path native/Cargo.toml -p datafusion-comet-spark-expr --bench wide_decimal
• cargo fmt --manifest-path native/Cargo.toml --all -- --check
• git diff --check

benchmark:

Shape	Before	After	Change
No overflow	52.60 µs	47.64 µs	9.73% faster
No overflow + nulls	57.52 µs	50.89 µs	12.13% faster
Overflow at end of batch	53.74 µs	52.49 µs	No stable change across reruns
ANSI, no overflow	48.25 µs	47.90 µs	No significant change (p = 0.96)

[andygrove]

Nice targeted perf fix, and the new bench shapes make the win legible. One small ask: the sibling optimization in decimal_rescale_check.rs:200 carries a comment explaining that i128::MAX is the overflow sentinel and that contains short-circuits so no-overflow batches skip the extra allocation. Could we add the same comment on the new guard in wide_decimal_binary_expr.rs:284 so both call sites read the same way? Without it, a reader landing on this line has to jump to the doc comment on check_overflow_and_convert to figure out why i128::MAX is the value being probed.

[peterxcli]

@andygrove genteelly ping, I've added the comment as your review, do you think this is ok to merge now? Thanks!

[andygrove]

Thanks for the update, the comment reads much better alongside the sibling in decimal_rescale_check.rs now.

I went through the equivalence argument carefully and I agree the guard is sound. try_binary zero-fills null slots rather than leaving stale values, so the scan cannot false-positive on a null. A non-overflowing result is clamped to ±(10^p_out - 1), and 10^38 - 1 is about 9.99e37 against i128::MAX at roughly 1.70e38, so a legitimate value can never collide with the sentinel. And null_if_overflow_precision nulls exactly the values outside ±(10^p - 1), which is exactly the sentinel set. So when no sentinel is present the pass really is a no-op and the output stays bit-identical.

CI is fully green.

I have a few things I would like to resolve before this merges. The main one is that neither new overflow benchmark actually measures the shape that could regress, so we do not yet have evidence that the overflow path is unaffected. Details inline.

[andygrove]

The two overflow shapes both end up with an overflow at index 0, since 0 % 17 == 0 and 0 % 2 == 0. That means contains returns on the very first element in both cases, so the sparse and dense benches never exercise a scan longer than one element.

Could we add a shape where the only overflow sits at the last index? That is the case where the guard costs the most, because you pay the full 8192-element scan and then still pay the masking pass on top. It would be good to see a number for it. Right now the "No significant change" results for the sparse and dense shapes are really measuring the index-0 case, so they do not tell us much about the overflow path.

[peterxcli]

added. thanks!

[andygrove]

Did you consider tracking the overflow during the arithmetic pass instead of scanning for the sentinel afterwards? check_overflow_and_convert already knows the answer at the moment it writes the sentinel, so contains is re-deriving a fact we had for free. try_binary takes an Fn, so a captured Cell<bool> works and only gets written on the rare overflow branch.

That would drop the extra 128 KB read from both paths, which should make the no-overflow win larger than the 8-10% you measured, and it removes the regression risk on the overflow shapes entirely.

I realize the tradeoff is losing the symmetry with decimal_rescale_check.rs, and the issue text did prescribe contains, so I am happy to hear the case for keeping it as is. But the scan is essentially all that is left of the cost here, so it seems worth weighing.

[peterxcli]

yes, capture the overflow in check_overflow_and_convert makes more sense

[andygrove]

One thing about the wording. "contains short-circuits at the first sentinel, so the common no-overflow case skips that allocation entirely" joins two facts that are not actually connected. In the no-overflow case contains does not short-circuit at all, it reads the entire buffer. The short-circuit only helps when an overflow is present and found early.

Could we say instead that the no-overflow path trades the allocating masking pass for a single read-only scan? Since this comment is the place a reader learns the cost model, it would help for it to be explicit that the common path is not free. The same wording exists in decimal_rescale_check.rs, so it is probably worth fixing in both places.

[peterxcli]

I change this to follow your direction: #5044 (comment), so this no longer a problem.

[andygrove]

Good test, and I like that test_null_propagation already covers the complementary shape where the fast path runs and nulls have to survive.

Since the guard now also governs the Scalar x Scalar path, it might be worth adding a companion to test_scalar_scalar_returns_scalar that overflows in legacy mode and asserts the result comes back as ScalarValue::Decimal128(None, p, s) rather than an array or a raw sentinel. That path routes through ScalarValue::try_from_array after the masking pass, so it seems worth pinning down.

[peterxcli]

added. thanks!

[mbutrovich]

decimal_rescale_check.rs:200 still runs result.values().contains(&i128::MAX) on every batch. In the common no-overflow case that is a full read of the output buffer, the same cost this PR removes from WideDecimalBinaryExpr by tracking overflow with a Cell<bool> inside the try_binary closure instead of scanning for the sentinel afterward.

DecimalRescaleCheckOverflow::evaluate has the same sentinel-then-mask shape, just built on try_unary instead of try_binary. The same closure-capture technique applies there without changes to the surrounding logic. Right now this PR updates the comment on decimal_rescale_check.rs:201-205 to admit the scan still happens, but leaves the scan itself in place, so one of the two call sites gets the actual fix and the other gets a note explaining why it still pays the cost.

Could you either apply the same Cell<bool> guard to decimal_rescale_check.rs, or open a follow-up issue for it so the asymmetry is tracked rather than left implicit in a comment? If it is deliberately out of scope for this PR, a one-line note here (or in the PR description) saying so would help.

[peterxcli]

ok, I've addressed it in this along with this PR, updated the title and description, too.