perf: vectorize integer-to-decimal cast#4939
Conversation
6da4ddf to
94b9a45
Compare
cast_int_to_decimal128_internal used a per-element Decimal128Builder loop with a null check and branch per row. Replace it with a single vectorized unary_opt pass: a value that overflows the multiply or does not fit the output precision maps to null, and the input null buffer is carried over. ANSI mode must raise on out-of-range values rather than nulling them. unary_opt only nulls non-null inputs that overflow, so a null count beyond the input's signals an overflow; that O(1) check gates a rare element-wise rescan that reports the first offending value with Spark's exact error. This is a single pass for every eval mode and shape, so all shapes are faster (28-50%) with no regression, including the overflow case. Add unit tests for the legacy null-on-overflow, ANSI no-overflow, and ANSI overflow-error paths, plus a criterion benchmark. Part of apache#4936.
94b9a45 to
d62d695
Compare
mbutrovich
left a comment
There was a problem hiding this comment.
First pass, thanks @andygrove!
|
|
||
| #[test] | ||
| fn test_cast_int_to_decimal128_overflow_legacy_nulls() { | ||
| // 1000 * 10^2 = 100000 does not fit precision 3 -> null (legacy). Valid values and the |
There was a problem hiding this comment.
native/spark-expr/src/conversion_funcs/numeric.rs:1159-1160 (new test test_cast_int_to_decimal128_overflow_legacy_nulls) says the test exercises "the vectorized sentinel + masking path." The new implementation has no sentinel value and no second masking pass. unary_opt writes nulls directly. The comment is left over from an earlier design and will mislead the next reader.
Suggested change: replace with something like "exercises the vectorized null-on-overflow path with valid values and an input null preserved."
| assert!(decimal_array.is_null(2)); | ||
| } | ||
|
|
||
| #[test] |
There was a problem hiding this comment.
The three new tests cover Legacy null-on-overflow, ANSI no-overflow, and ANSI overflow-error. None cover EvalMode::Try. Try shares the Legacy null branch today, so this is untested rather than broken, but Try is a distinct enum arm that a future edit could regress silently, and the PR text claims all eval modes are covered.
Suggested change: add a test_cast_int_to_decimal128_overflow_try_nulls mirroring the Legacy test with EvalMode::Try, asserting overflow maps to null and the input null is preserved.
| // Single vectorized pass: a value that overflows the multiply or does not fit the output | ||
| // precision maps to null. `unary_opt` only applies the closure to non-null slots and carries | ||
| // the input null buffer over, replacing the per-element builder loop without a second pass. | ||
| let result: Decimal128Array = array.unary_opt::<_, Decimal128Type>(|v| { |
There was a problem hiding this comment.
native/spark-expr/src/conversion_funcs/numeric.rs: the fast path closure computes v.checked_mul(multiplier).filter(|scaled| is_validate_decimal_precision(*scaled, precision)), and the ANSI rescan recomputes the same thing inverted as checked_mul(multiplier).map(|scaled| !is_validate_decimal_precision(...)).unwrap_or(true). Two spellings of one predicate invite drift where one is updated and the other is not.
Suggested change: extract a small local closure or helper, for example:
let fits = |v: i128| -> Option<i128> {
v.checked_mul(multiplier)
.filter(|scaled| is_validate_decimal_precision(*scaled, precision))
};
let result: Decimal128Array = array.unary_opt::<_, Decimal128Type>(|v| fits(v.into()));
// rescan:
if fits(v).is_none() { return Err(SparkError::NumericValueOutOfRange { .. }); }
Which issue does this PR close?
Part of #4936.
Rationale for this change
CAST(<int> AS DECIMAL(p, s))is common in TPC-DS (decimal casts are the second most frequent cast afterAS DATE). The native path,cast_int_to_decimal128_internal, used a per-elementDecimal128Builderloop with a null check and branch on every row.What changes are included in this PR?
Replaces the element-wise loop with a single vectorized
unary_optpass: a value that overflows the multiply or does not fit the output precision maps to null, and the input null buffer is carried over.unary_optapplies the closure only to non-null slots, so it reproduces the loop's behavior in one pass with no sentinel and no second masking pass.ANSI mode must raise on out-of-range values instead of nulling them.
unary_optonly nulls non-null inputs that overflow, so a null count beyond the input's signals an overflow. That check is O(1) and gates a rare element-wise rescan that reports the first offending value with Spark's exactNumericValueOutOfRangeerror.How are these changes tested?
Added unit tests for the legacy null-on-overflow (with nulls preserved), ANSI no-overflow, and ANSI overflow-error paths; the existing no-overflow test still passes. Output is bit-identical to
main.Because it is a single pass for every eval mode and shape, there is no regression on any shape: