perf: vectorize float/decimal to narrow-integer casts - #4941
Conversation
The four macros for float-to-int and decimal-to-int narrowing casts (cast_float_to_int16_down, cast_float_to_int32_up, cast_decimal_to_int16_down, cast_decimal_to_int32_up) built the output with a per-element iterator-collect over Option/Result. Replace that with Arrow's unary (legacy) and try_unary (ANSI) kernels, which map the values buffer in one pass and carry the null buffer over, following the same pattern used by cast_int_to_int_macro. The decimal macros also hoist the constant scale divisor out of the per-element loop. Overflow, NaN, saturation, and wrap-through-Int semantics are preserved unchanged; only the iteration mechanism changes. Add Rust unit tests for the float-to-Byte and decimal-to-Int/Byte legacy wrap paths and the ANSI overflow error paths (previously covered only by Scala tests), plus a benchmark. Non-overflow casts are 49-91% faster with no regression. Part of apache#4936.
mbutrovich
left a comment
There was a problem hiding this comment.
First pass, thanks @andygrove!
…ulls invariant Extends test_cast_float64_to_int8_legacy_wraps with 3e9, +inf, and -inf so the saturate-i32-then-truncate-to-i8 chain is asserted end to end; a regression that lost the double-narrowing would fail loudly. Adds a one-line comment on each of the four legacy PrimitiveArray::unary arms noting that the closure runs on null slots too and stating why the as-cast / positive-divisor division is infallible for any bit pattern.
mbutrovich
left a comment
There was a problem hiding this comment.
This one LGTM after revision, thanks @andygrove!
|
@copilot resolve the merge conflicts on this branch. |
…ng-casts # Conflicts: # docs/source/contributor-guide/expression-audits/conversion_funcs.md # native/spark-expr/Cargo.toml
| let divisor = 10_i128.pow($scale as u32); | ||
| let output_array: $dest_array_type = match $eval_mode { | ||
| EvalMode::Ansi => cast_array.try_unary::<_, $dest_arrow_type, SparkError>(|value| { | ||
| let truncated = value / divisor; |
There was a problem hiding this comment.
The hoisted divisor assumes scale >= 0, but Arrow and Comet permit negative-scale Decimal128 values. Casting a negative i8 scale to u32 makes pow overflow; in release it can produce a zero divisor, after which Legacy unary divides null-slot backing values by zero. Could we either implement scale-aware negative-scale handling or fall back these casts to Spark, and add an all-null negative-scale regression test?
There was a problem hiding this comment.
Confirmed and fixed in 6838967 — thanks, this was a real panic and I reproduced every step of your reasoning.
-1i8 as u32 is 4294967295, and 10_i128.wrapping_pow(4294967295) is exactly 0 (10^k has a factor of 2^k, so for k >= 128 the wrapped value is 0). So release wraps to a zero divisor and then divides by zero, and debug panics in the pow itself.
End-to-end reproduction took a bit of setting up, which is worth recording: a negative-scale decimal cannot be round-tripped through Parquet (Invalid DECIMAL scale: -4) and the SQL parser rejects DECIMAL(10,-4) regardless of allowNegativeScaleOfDecimal. So the reachable path is a negative-scale value produced mid-plan by one native cast and consumed by the next:
reread.select(col("a").cast(DataTypes.createDecimalType(10, -4)).cast(DataTypes.ByteType))which gives:
org.apache.comet.CometNativeException: native panic: attempt to multiply with overflow
(core/src/num/mod.rs:475 -> pow)
I went with your second option, falling back rather than implementing scale-aware handling: CometCast.canCastFromDecimal now takes the source DecimalType and reports negative-scale integral casts Unsupported, following the negative-scale precedent already in canCastToString. Multiplying correctly would need its own ANSI-overflow and wrap semantics worked out against Spark, which does not belong in a vectorization PR. I gated only the integral targets, since those are the ones with a reproduced panic. Both pow sites now carry a comment pointing at the Scala guard so they do not drift apart.
The regression test covers both shapes you asked about, including all-null.
One clarification on scope, since it affects how you read the fix: on main the same 10_i128.pow(scale as u32) sits inside the per-element closure, so the panic already exists there for any non-null negative-scale value. What this PR changes is specifically the all-null case — unary applies the op to null slots, where the old iterator-collect ran it only for Some values — so an all-null negative-scale column newly reaches the divisor. Your instinct to ask for an all-null test was pointing straight at the part this PR actually introduced.
The hoisted divisor assumes a non-negative scale. A DECIMAL(p, s<0), which
spark.sql.legacy.allowNegativeScaleOfDecimal=true makes constructible, represents
unscaled * 10^-s, so casting it to an integral type has to multiply rather than
divide. `10_i128.pow($scale as u32)` wraps a negative i8 to a huge exponent, and
`10_i128.wrapping_pow(-1i8 as u32)` is exactly 0, so the batch either panics in
the pow (debug) or divides by zero (release).
Reproduced end to end. A negative-scale decimal cannot be stored in Parquet
("Invalid DECIMAL scale: -4") and the SQL parser rejects DECIMAL(10,-4), so the
path is a negative-scale value produced mid-plan by one native cast and consumed
by the next. That gives:
org.apache.comet.CometNativeException: native panic:
attempt to multiply with overflow (core/src/num/mod.rs:475 -> pow)
Fixed by reporting these casts Unsupported in CometCast.canCastFromDecimal, which
falls the cast back to Spark, following the negative-scale precedent already in
canCastToString. canCastFromDecimal now takes the source DecimalType so it can see
the scale. Only the integral targets are gated: they are the ones with a
reproduced panic. The pow sites carry a comment pointing at the Scala guard so the
two do not drift apart.
Worth noting for the record: on main the same `10_i128.pow(scale as u32)` sits
inside the per-element closure, so the panic is pre-existing for any non-null
negative-scale value. What this PR changes is the all-null case -- `unary` applies
the op to null slots, where the old iterator-collect ran it only for Some values --
so an all-null negative-scale column newly reaches the divisor. The regression
test covers both shapes.
Verified on spark-3.5: the new test passes, 25 decimal cast tests pass, "all valid
cast combinations covered" still passes (DecimalType(10,2) is unaffected), and 65
conversion_funcs unit tests pass.
|
@peterxcli thanks — the negative-scale finding was a real panic, fixed in 6838967. Confirmed your analysis exactly. Reproduced end to end, which took some setup worth recording: a negative-scale decimal cannot be round-tripped through Parquet ( Took the fallback option, not scale-aware handling: One scope clarification. On Also confirming @mbutrovich's two earlier points are in: Verified on spark-3.5: new test passes, 25 decimal cast tests pass, "all valid cast combinations covered" still passes ( |
Which issue does this PR close?
Part of #4936.
Rationale for this change
The float-to-int and decimal-to-int narrowing casts built their output with a per-element
iter().map(...).collect::<Result<...>>()overOption/Result, the same slow pattern thatspark_cast_int_to_intmoved off of (that change was up to 100x faster).What changes are included in this PR?
Rewrites the four macros (
cast_float_to_int16_down,cast_float_to_int32_up,cast_decimal_to_int16_down,cast_decimal_to_int32_up) to use Arrow'sunary(legacy/try) andtry_unary(ANSI) kernels, which map the values buffer in one pass and carry the null buffer over, following the existingcast_int_to_int_macro. Each macro gains a destinationArrowPrimitiveTypeparameter, threaded through the 12 call sites. The decimal macros also hoist the constant10^scaledivisor out of the per-element loop.Overflow, NaN, saturation, and the cast-through-Int wrap semantics are preserved exactly; only the iteration mechanism changes.
How are these changes tested?
Added Rust unit tests (these paths previously had only Scala coverage): float64-to-Byte legacy wrap, float64-to-Int ANSI ok + overflow error, decimal-to-Int legacy, decimal-to-Byte legacy wrap, and decimal-to-Int ANSI overflow error. Output is bit-identical to
main.Benchmark (criterion), baseline
mainvs this branch, 8192-row columns: