perf: optimize cast_decimal128_to_utf8 in datafusion-comet-spark-expr#4924
Conversation
mbutrovich
left a comment
There was a problem hiding this comment.
First pass, thanks @andygrove!
| // values needing more than one 64-bit digit group | ||
| assert_eq!(fmt(10_000_000_000_000_000_000, 0), "10000000000000000000"); | ||
| assert_eq!(fmt(i128::MAX, 0), i128::MAX.to_string()); | ||
| assert_eq!(fmt(i128::MIN, 0), i128::MIN.to_string()); | ||
| assert_eq!( | ||
| fmt(99_999_999_999_999_999_999_999_999_999_999_999_999, 10), | ||
| "9999999999999999999999999999.9999999999" |
There was a problem hiding this comment.
native/spark-expr/src/conversion_funcs/numeric.rs:1499-1506 (new tests) covers i128::MAX, i128::MIN, and the max-precision plain-notation coefficient, but every multi-group case is either non-negative or in plain notation. The sign push and the coefficient-splitting for scientific notation (&coeff[..1] / &coeff[1..]) are the parts most likely to regress if someone later refactors render_digits, and neither is exercised on a coefficient that spans two 64-bit groups.
Suggested change: add cases such as
// multi-group coefficient in scientific notation, both signs
assert_eq!(fmt(i128::MAX, 45), "1.70141183460469231731687303715884105727E-7");
assert_eq!(fmt(-i128::MAX, 45), "-1.70141183460469231731687303715884105727E-7");(compute the expected strings from BigDecimal(unscaled, scale).toString() or the pre-PR implementation so they are anchored to Spark, not to the new code).
| assert_eq!(fmt(-123, -2), "-1.23E+4"); | ||
|
|
||
| // values needing more than one 64-bit digit group | ||
| assert_eq!(fmt(10_000_000_000_000_000_000, 0), "10000000000000000000"); |
There was a problem hiding this comment.
native/spark-expr/src/conversion_funcs/numeric.rs:1500 tests 10_000_000_000_000_000_000 (scale 0), which is the smallest two-group value, but no test covers a value like 10^19 + 5 where the lower group is 0000000000000000005 and the emitted leading zeros of that group are load-bearing. This is exactly the case the code comment at render_digits calls out ("emitting its leading zeroes is correct"), so it deserves a guard test.
Suggested change:
assert_eq!(fmt(10_000_000_000_000_000_005, 0), "10000000000000000005");
Which issue does this PR close?
N/A
Rationale for this change
Optimize existing expression.
What changes are included in this PR?
Replaced the per-row u128 to_string heap allocation, format! machinery, and "0".repeat in the LEGACY Decimal128-to-string cast with an allocation-free stack digit buffer that renders the coefficient in 19-digit 64-bit chunks and appends slices directly to the reused output buffer.
How are these changes tested?
Existing tests + expanded unit tests.
Benchmark (criterion):
Full criterion output: