dylanpulver opened a new pull request, #24851: URL: https://github.com/apache/datafusion/pull/24851
## Which issue does this PR close? - Closes #24850. ## Rationale for this change `SELECT CAST(1 AS DECIMAL(76,0)) < CAST(2 AS DECIMAL(76,52))` panics during planning. The required precision `max(s1, s2) + max(p1 - s1, p2 - s2)` is computed in `i8`, the type of a decimal scale, but `Decimal256` allows a precision and a scale of 76, so `p - s` reaches 152 and the sum reaches 228. Debug builds panic; release builds wrap and return a wrong precision. ## What changes are included in this PR? The expression moves into `required_decimal_precision(p1, s1, p2, s2)`, which computes in `i32` and clamps into `u8`. The five call sites in `get_wider_decimal_type` and `get_wider_decimal_type_cross_variant` use it. The change is inert on inputs that did not overflow. With `s = max(s1, s2) = s1` without loss of generality, `range >= p1 - s1`, so `range + s >= p1 >= 0`: the sum is never negative, the lower clamp never fires, and for values in `0..=127` the widened computation equals the old one bit for bit. ## What is the testing strategy for this PR? `test_decimal256_comparison_coercion_precision_overflow` in `binary/tests/comparison.rs`, three cases: sum 128, sum 152, and a negative scale where `p1 - s1` is 152 before the sum is computed. `cargo test -p datafusion-expr-common --lib`: 207 passed on main, 208 with this branch. Reverting the source with the test kept panics at `binary.rs:1217:41` with `attempt to add with overflow`. The naive fix — `range.saturating_add(s)` at all five sites — still fails, now at `binary.rs:1216:25` with `attempt to subtract with overflow`, which is why the negative-scale case is in the test. End to end, same probe both ways: on main `create_logical_plan` for the query above panics at `binary.rs:1217:41`; with this branch it returns `Projection: CAST(Int64(1) AS Decimal256(76, 0)) < CAST(Int64(2) AS Decimal256(76, 52)) AS r`. `cargo fmt --all --check`, `cargo clippy -p datafusion-expr-common --all-targets --all-features -- -D warnings`, and `cargo test` on `datafusion-expr-common`, `datafusion-expr`, `datafusion-sql`, `datafusion-optimizer` all pass. rustc 1.97.0, matching `rust-toolchain.toml`. No `.slt` case, deliberately — see below. ## Are there any user-facing changes? Two things a reviewer should weigh, both stated because I am not certain they are what you want: **This does not make the query execute.** It moves the panic to `arrow-cast/src/cast/decimal.rs:190`, `(input_precision as i8) + delta_scale <= (output_precision as i8)`, which is the same bug upstream. I measured it: after this fix, planning succeeds and `collect()` panics there. The two overflows are the same quantity, so no SQL can trigger DataFusion's without also triggering arrow's. That is why there is no `.slt` case — one would fail CI. The arrow side needs its own fix. **One release-mode behaviour change.** In `get_wider_decimal_type_cross_variant` with a negative scale, the wrapped `i8` previously produced a small `required_precision` that passed the variant checks and yielded a lossy type; it now yields 152, exceeds every maximum, and returns `None`. Negative scales cannot be written in DDL, but they can arrive from directly constructed `DataType`s such as Parquet or IPC schemas. I believe `None` is correct — there is no common type that holds both — but it is a change, not a no-op, and it is the one part of this I would want a second opinion on. Not run: the workspace-wide clippy and the extended-features test run from `AGENTS.md`, `./dev/rust_lint.sh` as a whole, a release build, and benchmarks. The sqllogictest suite reports the same 39 failures with and without this change, all from an uninitialised `parquet-testing` submodule in my checkout. --- Per the AI-assisted contributions policy: this patch, its test and the measurements above were produced with AI assistance (Claude Opus 4.8). The unknowns are the two items in the section above. -- 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]
