perf: vectorize floating-point-to-decimal cast#4940
Conversation
cast_floating_point_to_decimal128 used a per-element Decimal128Builder loop with a null check and branch per row. Replace it with a single vectorized unary_opt pass: values with no in-range integer form (NaN / infinity) or that do not fit the output precision map 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 (15-36%) with no regression, including the overflow case. Add unit tests for the fast path, ANSI no-overflow, and ANSI overflow-error paths, plus a benchmark. Part of apache#4936.
mbutrovich
left a comment
mbutrovich
left a comment
There was a problem hiding this comment.
First pass, thanks @andygrove!
|
|
||
| #[test] | ||
| #[cfg_attr(miri, ignore)] | ||
| fn test_cast_float_to_decimal_ansi_overflow_errors() { |
There was a problem hiding this comment.
test_cast_float_to_decimal_ansi_overflow_errors (diff, new test) only exercises a finite precision overflow (4242.42 into Decimal(4,2)). The rescan at numeric.rs (new ANSI block) also has to raise for NaN and infinity, since those make to_i128() return None and get nulled by unary_opt exactly like a precision overflow. The legacy-mode test_cast_float_to_decimal at numeric.rs:1265 covers NaN/Inf-to-null, but nothing covers NaN/Inf under ANSI, which is a distinct code path (closure returns None for a non-finite value, then rescan must recompute fits == false and error). If a future change made the rescan use a finite-only overflow predicate, this would silently regress with no failing test.
Suggested change: extend the ANSI error test with f64::NAN and f64::INFINITY inputs and assert is_err(), and assert the reported value string is "NaN" / "inf" to lock the message.
let a: ArrayRef = Arc::new(Float64Array::from(vec![Some(1.0), Some(f64::NAN)]));
assert!(cast_floating_point_to_decimal128::<Float64Type>(&a, 10, 2, EvalMode::Ansi).is_err());
let a: ArrayRef = Arc::new(Float64Array::from(vec![Some(f64::INFINITY)]));
assert!(cast_floating_point_to_decimal128::<Float64Type>(&a, 10, 2, EvalMode::Ansi).is_err());|
|
||
| #[test] | ||
| #[cfg_attr(miri, ignore)] | ||
| fn test_cast_float_to_decimal_ansi_overflow_errors() { |
There was a problem hiding this comment.
test_cast_float_to_decimal_ansi_overflow_errors asserts only result.is_err(). The whole point of the rescan is to reproduce Spark's exact NumericValueOutOfRange { value, precision, scale } with the first offending value in row order. A bare is_err() would still pass if the rescan returned the wrong variant, the wrong value, or the second offender instead of the first.
Suggested change: match on the error and assert the fields.
match cast_floating_point_to_decimal128::<Float64Type>(&a, 4, 2, EvalMode::Ansi) {
Err(SparkError::NumericValueOutOfRange { value, precision, scale }) => {
assert_eq!(value, "4242.42");
assert_eq!(precision, 4);
assert_eq!(scale, 2);
}
other => panic!("expected NumericValueOutOfRange, got {other:?}"),
}| let result = cast_floating_point_to_decimal128::<Float64Type>(&a, 4, 2, EvalMode::Ansi); | ||
| assert!(result.is_err()); | ||
| } | ||
|
|
There was a problem hiding this comment.
The rescan iterates 0..input.len() and returns the first row where fits is false, matching the old loop that erred on the first offending row. No test pins this. A batch with two overflowing rows where the second has a different value would not distinguish "first" from "last".
Suggested change: add an ANSI input like [Some(1.0), Some(overflow_a), Some(overflow_b)] where overflow_a and overflow_b stringify differently, and assert the error reports overflow_a.
2 participants
Which issue does this PR close?
Part of #4936.
Rationale for this change
CAST(<float|double> AS DECIMAL(p, s))is used in TPC-DS. The native path,cast_floating_point_to_decimal128, 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 with no in-range integer form (NaN / infinity) or that 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?
The existing
test_cast_float_to_decimal(which covers NaN / infinity / precision-overflow to null in legacy mode) still passes unchanged. Added tests for the no-overflow fast path (values and nulls preserved), ANSI no-overflow, and ANSI overflow-error. Output is bit-identical tomain.Because it is a single pass for every eval mode and shape, there is no regression on any shape: