andygrove commented on PR #4941: URL: https://github.com/apache/datafusion-comet/pull/4941#issuecomment-5147356349
@peterxcli thanks — the negative-scale finding was a real panic, fixed in 6838967c6. **Confirmed your analysis exactly.** `-1i8 as u32` is `4294967295`, and `10_i128.wrapping_pow(4294967295)` is precisely `0` (10^k carries a factor of 2^k, so any k >= 128 wraps to zero). Release therefore divides by zero; debug panics in the `pow`. **Reproduced end to end**, which took some setup 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 only reachable path is a negative-scale value produced mid-plan by one native cast and consumed by the next, which yields: ``` org.apache.comet.CometNativeException: native panic: attempt to multiply with overflow ``` **Took the fallback option**, not scale-aware handling: `CometCast.canCastFromDecimal` now receives the source `DecimalType` and reports negative-scale integral casts `Unsupported`, following the precedent already in `canCastToString`. Doing the multiply correctly needs its own ANSI-overflow and legacy-wrap semantics settled against Spark, which does not belong in a vectorization PR. Gated the integral targets only — those are the ones with a reproduced panic. Both `pow` sites now point at the Scala guard so they cannot drift apart. **One scope clarification.** On `main` the same `pow` 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. Your ask for an all-null regression test was pointing exactly at the part this PR introduced — the test covers both shapes. Also confirming @mbutrovich's two earlier points are in: `test_cast_float64_to_int8_legacy_wraps` covers `3e9` and `±inf` for the saturate-then-narrow path, and each legacy arm documents that `unary` runs the closure on null slots and why the op there is infallible. Verified on spark-3.5: new test passes, 25 decimal cast tests pass, "all valid cast combinations covered" still passes (`DecimalType(10,2)` is unaffected by the gate), 65 `conversion_funcs` unit tests pass. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
