perf: skip the null-masking pass in DecimalRescaleCheckOverflow when nothing overflows#4938
perf: skip the null-masking pass in DecimalRescaleCheckOverflow when nothing overflows#4938andygrove wants to merge 2 commits into
DecimalRescaleCheckOverflow when nothing overflows#4938Conversation
…nothing overflows DecimalRescaleCheckOverflow rescales decimal values and checks precision in a single try_unary pass, writing i128::MAX as an overflow sentinel. In legacy (non-ANSI) mode it then always ran null_if_overflow_precision, a second full pass that allocates a new array to turn the sentinels into nulls, even when no value overflowed (the common decimal-cast case). Only run that pass when a sentinel is actually present. The check short-circuits at the first sentinel, so the overflow path is unaffected while the common no-overflow path skips the extra allocation. Non-overflow shapes are 8-26% faster; overflow and ANSI shapes are unchanged within noise across samples. Add a legacy test that mixes overflow and null inputs, and a criterion benchmark. Part of apache#4936.
mbutrovich
left a comment
mbutrovich
left a comment
There was a problem hiding this comment.
First pass, thanks @andygrove!
| })?; | ||
|
|
||
| let result = if !fail_on_error { | ||
| let result = if !fail_on_error && result.values().contains(&i128::MAX) { |
There was a problem hiding this comment.
decimal_rescale_check.rs:200-206. The new comment reads "any short-circuits at the first sentinel," but the code calls result.values().contains(&i128::MAX). Both short-circuit, so behavior is right, the prose just names the wrong method and will confuse the next reader.
Suggested change: replace "any" with "contains" in the comment.
| } | ||
|
|
||
| #[test] | ||
| fn test_overflow_with_nulls_legacy() { |
There was a problem hiding this comment.
The added test_overflow_with_nulls_legacy (decimal_rescale_check.rs:352) covers the mixed case, and existing tests cover single-value overflow. There is no array test where every value overflows, which is the shape that exercises contains finding the sentinel at index 0 and the masking pass nulling the whole array.
Suggested change: add a legacy test over vec![Some(10_000), Some(20_000), Some(30_000)] into precision 4 and assert all three slots are null.
| assert!(result.is_err()); | ||
| } | ||
|
|
||
| #[test] |
There was a problem hiding this comment.
No test pins the value exactly at 10^p - 1 (fits) versus 10^p (overflows) for the same output precision, which is the boundary the whole predicate turns on.
Suggested change: add a legacy test with output precision 4 over vec![Some(9999), Some(10_000)] at scale 0, asserting slot 0 keeps 9999 and slot 1 is null. This makes the exact bound a regression guard rather than an implied property.
2 participants
Which issue does this PR close?
Part of #4936.
Rationale for this change
DecimalRescaleCheckOverflowimplements the fusedCheckOverflow(Cast(expr, Decimal128(p,s)))used by decimal-to-decimal casts, which are common in TPC-DS. It rescales and precision-checks in a singletry_unarypass, writingi128::MAXas an overflow sentinel. In legacy (non-ANSI) mode it then always rannull_if_overflow_precision, a second full pass that allocates a new array to convert sentinels into nulls, even when no value overflowed (the common case).What changes are included in this PR?
Runs the
null_if_overflow_precisionmasking pass only when a sentinel is actually present (result.values().contains(&i128::MAX)). The check short-circuits at the first sentinel, so the overflow path is unaffected, while the common no-overflow path skips the extra allocation. ANSI mode never produces a sentinel and is unchanged.Also adds a legacy unit test that mixes valid, overflowing, and null inputs (exercising the sentinel fallback with nulls present) and a criterion benchmark.
How are these changes tested?
Existing unit tests cover scale-up, scale-down with HALF_UP rounding, precision-only checks, overflow nulling (legacy) and raising (ANSI), null propagation, and the scalar path. Added a legacy overflow-with-nulls test. Output is bit-identical to
main: when no sentinel is present,null_if_overflow_precisionwould have nulled nothing, so skipping it yields the same array.Benchmark (criterion), baseline
mainvs this branch, 8192-row Decimal128 columns, confirmed across multiple samples:The no-overflow shapes (the common case) skip the second allocation; overflow shapes short-circuit the sentinel check and run the unchanged masking pass.