perf: optimize parse_string_to_decimal (30-40% faster)#4916
Conversation
andygrove
changed the title
parse_string_to_decimal (30-40% faster)
mbutrovich
left a comment
mbutrovich
left a comment
There was a problem hiding this comment.
First pass, thanks @andygrove!
| (EvalMode::Ansi, "ansi"), | ||
| (EvalMode::Try, "try"), | ||
| ] { | ||
| let mut group = c.benchmark_group(format!("cast_string_to_decimal/{mode_name}")); |
There was a problem hiding this comment.
native/spark-expr/benches/cast_string_to_decimal.rs:72 creates group cast_string_to_decimal/{mode_name} with a decimal_38_10 bench function. The existing native/spark-expr/benches/cast_from_string.rs:124-131 already creates the identical group cast_string_to_decimal/{mode_name} with an identical decimal_38_10 function, over the same three eval modes and the same string shapes (plain, fixed-point, negative, scientific). Criterion keys saved results by group/function, so the two benches write to and overwrite the same target/criterion/cast_string_to_decimal/legacy/decimal_38_10 directory. Whichever bench runs last clobbers the other's baseline, which makes the reported before/after comparison unreliable.
The only thing the new file adds over the existing one is the decimal_18_2 variant and a seeded StdRng (the existing bench uses unseeded rand::random, so its numbers are not reproducible run to run).
Suggested change: do not add a second benchmark file. Fold the improvements into the existing create_decimal_cast_string_batch path in cast_from_string.rs: switch it to a seeded StdRng and add the decimal_18_2 data type to the existing loop at cast_from_string.rs:116-133. Delete benches/cast_string_to_decimal.rs and the corresponding [[bench]] entry in Cargo.toml. If you keep a separate file, rename its group to something distinct like parse_string_to_decimal/... so it does not overwrite the existing baseline.
| /// The first 38 digits always fit (`i128::MAX` is ~1.7e38), so only the digits past | ||
| /// them need the per-digit overflow checks. | ||
| #[inline] | ||
| fn digits_to_i128(digits: &[u8]) -> Option<i128> { |
There was a problem hiding this comment.
digits_to_i128 at native/spark-expr/src/conversion_funcs/string.rs (new helper) splits the digit slice at 38 and skips overflow checks on the first 38 digits, relying on the fact that 38 nines (about 9.99e37) fit under i128::MAX (about 1.7e38). The reasoning is correct, and leading zeros are handled correctly because zeros in the head accumulate to 0 and the tail uses checked_mul/checked_add. But this boundary is exactly where a future edit could regress, and the current Scala fuzz tests (CometCastSuite.scala:958-961) cap the generated digit count at 38, so they never exercise a 39-plus-digit integral part.
Suggested change: add a Rust unit test in the #[cfg(test)] block of string.rs asserting parse results for a 38-digit value (parses), a 39-digit value (overflows to the invalid_decimal_cast error / NULL in non-ANSI), and a 40-digit value with leading zeros that reduce to a small value (parses). This locks the boundary that the optimization now depends on.
|
|
||
| /// `10^exp`, using the precomputed table for the range that fits in an `i128`. | ||
| #[inline] | ||
| fn pow10_i128(exp: u32) -> i128 { |
There was a problem hiding this comment.
pow10_i128 falls back to 10_i128.pow(exp) on a table miss (exp >= 39). Two of the three call sites are guarded by scale_adjustment > 38 / abs_scale_adjustment > 38 checks, so they never miss. The checked_mul(pow10_i128(fractional_scale as u32)) combine step at the end of parse_decimal_str, however, is not bounded: fractional_scale is fractional_part.len(), so an input like "0." + "0"*40 calls pow10_i128(40), misses the table, and evaluates 10_i128.pow(40), which panics in debug and wraps in release. This is identical to the old code (10_i128.pow(fractional_scale as u32)), so the PR introduces no regression, but the new named helper is the natural place to make the fallback total.
Suggested change: make pow10_i128 return Option<i128> (or add a checked_pow10 variant) and thread it through the combine step so an over-long fraction produces the invalid_decimal_cast error path instead of a panic. If you would rather keep this PR scoped to performance, leave a one-line comment on pow10_i128 noting that callers must keep exp <= 38 and that the 10_i128.pow fallback is unreachable in current callers, so it is not silently masking a bug.
2 participants
Which issue does this PR close?
N/A
Rationale for this change
Optimize existing expression.
What changes are included in this PR?
Replaced six redundant scans plus two str::parse calls with a single validating byte pass and direct i128 digit accumulation, gated the seven inf/nan string comparisons behind a first-byte check, and swapped 10_i128.pow for a const power-of-ten table.
How are these changes tested?
Existing tests.
Benchmark (criterion):
Full criterion output: