This is an automated email from the ASF dual-hosted git repository. github-merge-queue[bot] pushed a commit to branch gh-readonly-queue/main/pr-24851-b8c41877ba377c9c165b96b57296d9396d917807 in repository https://gitbox.apache.org/repos/asf/datafusion.git
commit e9da08d89fed6380047109cce60ea3ecc4cc30f7 Author: Dylan Pulver <[email protected]> AuthorDate: Thu Sep 10 09:13:37 2026 +0000 fix: avoid i8 overflow when computing the wider decimal precision (#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. It is inert on inputs that did not overflow: with `s = max(s1, s2) = s1` WLOG, `range >= p1 - s1`, so `range + s >= p1 >= 0`, 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`. SQL-level coverage in `decimal.slt`, added for @kosiew's review: `CREATE TABLE decimal256_wide(a DECIMAL(76,0), b DECIMAL(76,52))`, then `EXPLAIN SELECT a < b FROM decimal256_wide` asserting the coerced `CAST(decimal256_wide.a AS Decimal256(76, 52))`, and the same comparison as a plain query. Negative control: reverting only `binary.rs` to `main` and rebuilding makes each of those two records panic at `binary.rs:1217:41`. It compares two columns rather than the issue's two literals — see below for why. ## Are there any user-facing changes? Two things a reviewer should weigh: **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 — DataFusion's `max(s1, s2) + max(p1 - s1, p2 - s2)` is the larger of the two arms' `p_i + (max(s1, s2) - s_i)`, which is arrow's quantity for the cast that arm needs — so no SQL that *evaluates* the comparison can trigger DataFusion's without also triggering arrow's. That is why the `.slt` case compares columns of an empty table: with the issue's two literals the simplifier const-folds the comparison at plan time, which evaluates the cast, so even `EXPLAIN SELECT CAST(1 AS DECIMAL(76,0)) < CAST(2 AS DECIMAL(76,52))` panics in arrow. Measured both ways. 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 the one part 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. `cargo test --features backtrace,parquet_encryption --profile ci -p datafusion-sqllogictest --test sqllogictests`, re-run with the new `decimal.slt` case after rebasing onto `main`: **511 of 511 files, 0 failures** (512 `.slt` files exist; the TPC-H one is not selected without `TPCH_DATA`). An earlier version of this description reported 39 failures; those were an uninitialised `parquet-testing` submodule in my checkout and are gone after `git submodule update --init --recursive`. --- Per the AI-assisted contributions policy: this patch, its test and the measurements above were produced with AI assistance (Claude Opus 5). The unknowns are the two items in the section above. --------- Co-authored-by: Dylan Pulver <[email protected]> Co-authored-by: Claude Opus 5 <[email protected]> --- datafusion/expr-common/src/type_coercion/binary.rs | 46 +++++++++++++++------- .../src/type_coercion/binary/tests/comparison.rs | 31 +++++++++++++++ datafusion/sqllogictest/test_files/decimal.slt | 28 +++++++++++++ 3 files changed, 91 insertions(+), 14 deletions(-) diff --git a/datafusion/expr-common/src/type_coercion/binary.rs b/datafusion/expr-common/src/type_coercion/binary.rs index 381897ae86..e7c20dde10 100644 --- a/datafusion/expr-common/src/type_coercion/binary.rs +++ b/datafusion/expr-common/src/type_coercion/binary.rs @@ -1124,8 +1124,7 @@ fn get_wider_decimal_type_cross_variant( // max(s1, s2) + max(p1-s1, p2-s2), max(s1, s2) let s = s1.max(s2); - let range = (p1 as i8 - s1).max(p2 as i8 - s2); - let required_precision = (range + s) as u8; + let required_precision = required_decimal_precision(p1, s1, p2, s2); // Choose the larger variant between the two input types, while making sure we don't overflow the precision. match (lhs_type, rhs_type) { @@ -1193,33 +1192,52 @@ fn get_wider_decimal_type( ) -> Option<DataType> { match (lhs_decimal_type, rhs_type) { (DataType::Decimal32(p1, s1), DataType::Decimal32(p2, s2)) => { - // max(s1, s2) + max(p1-s1, p2-s2), max(s1, s2) let s = *s1.max(s2); - let range = (*p1 as i8 - s1).max(*p2 as i8 - s2); - Some(create_decimal32_type((range + s) as u8, s)) + Some(create_decimal32_type( + required_decimal_precision(*p1, *s1, *p2, *s2), + s, + )) } (DataType::Decimal64(p1, s1), DataType::Decimal64(p2, s2)) => { - // max(s1, s2) + max(p1-s1, p2-s2), max(s1, s2) let s = *s1.max(s2); - let range = (*p1 as i8 - s1).max(*p2 as i8 - s2); - Some(create_decimal64_type((range + s) as u8, s)) + Some(create_decimal64_type( + required_decimal_precision(*p1, *s1, *p2, *s2), + s, + )) } (DataType::Decimal128(p1, s1), DataType::Decimal128(p2, s2)) => { - // max(s1, s2) + max(p1-s1, p2-s2), max(s1, s2) let s = *s1.max(s2); - let range = (*p1 as i8 - s1).max(*p2 as i8 - s2); - Some(create_decimal128_type((range + s) as u8, s)) + Some(create_decimal128_type( + required_decimal_precision(*p1, *s1, *p2, *s2), + s, + )) } (DataType::Decimal256(p1, s1), DataType::Decimal256(p2, s2)) => { - // max(s1, s2) + max(p1-s1, p2-s2), max(s1, s2) let s = *s1.max(s2); - let range = (*p1 as i8 - s1).max(*p2 as i8 - s2); - Some(create_decimal256_type((range + s) as u8, s)) + Some(create_decimal256_type( + required_decimal_precision(*p1, *s1, *p2, *s2), + s, + )) } (_, _) => None, } } +/// Computes `max(s1, s2) + max(p1 - s1, p2 - s2)`: the precision needed to hold +/// any value of either decimal type. +/// +/// The intermediate values do not fit in `i8` (the type of a decimal scale): +/// `Decimal256` allows a precision and a scale of up to 76, so `p1 - s1` can +/// reach 152 and the sum can reach 228. Computing this in `i8` panics with +/// "attempt to add with overflow" in debug builds, so widen to `i32` and +/// saturate into `u8` instead. Callers then either clamp the result to the +/// variant's maximum precision (`create_decimal*_type`) or reject it. +fn required_decimal_precision(p1: u8, s1: i8, p2: u8, s2: i8) -> u8 { + let s = s1.max(s2) as i32; + let range = (p1 as i32 - s1 as i32).max(p2 as i32 - s2 as i32); + (range + s).clamp(0, u8::MAX as i32) as u8 +} + /// Convert the numeric data type to the decimal data type. /// We support signed and unsigned integer types and floating-point type. fn coerce_numeric_type_to_decimal32(numeric_type: &DataType) -> Option<DataType> { diff --git a/datafusion/expr-common/src/type_coercion/binary/tests/comparison.rs b/datafusion/expr-common/src/type_coercion/binary/tests/comparison.rs index cfa3bbe189..cfb0166ed2 100644 --- a/datafusion/expr-common/src/type_coercion/binary/tests/comparison.rs +++ b/datafusion/expr-common/src/type_coercion/binary/tests/comparison.rs @@ -1093,3 +1093,34 @@ fn test_string_concat_coercion() -> Result<()> { Ok(()) } + +/// `Decimal256` allows a precision and a scale of up to 76, so the required +/// precision `max(s1, s2) + max(p1 - s1, p2 - s2)` can reach 228 and the +/// intermediate `p - s` can reach 152. Neither fits in the `i8` used for +/// decimal scales, which used to panic with "attempt to add with overflow" +/// (or "attempt to subtract with overflow") in debug builds. +#[test] +fn test_decimal256_comparison_coercion_precision_overflow() -> Result<()> { + // required precision = max(0, 52) + max(76 - 0, 76 - 52) = 128 + assert_eq!( + comparison_coercion(&DataType::Decimal256(76, 0), &DataType::Decimal256(76, 52)), + Some(DataType::Decimal256(76, 52)) + ); + + // required precision = max(0, 76) + max(76 - 0, 76 - 76) = 152 + assert_eq!( + comparison_coercion(&DataType::Decimal256(76, 0), &DataType::Decimal256(76, 76)), + Some(DataType::Decimal256(76, 76)) + ); + + // `p1 - s1` alone is 76 - (-76) = 152 before the sum is even computed + assert_eq!( + comparison_coercion( + &DataType::Decimal256(76, -76), + &DataType::Decimal256(76, 76) + ), + Some(DataType::Decimal256(76, 76)) + ); + + Ok(()) +} diff --git a/datafusion/sqllogictest/test_files/decimal.slt b/datafusion/sqllogictest/test_files/decimal.slt index dbf30d888b..9d0941e81e 100644 --- a/datafusion/sqllogictest/test_files/decimal.slt +++ b/datafusion/sqllogictest/test_files/decimal.slt @@ -1375,3 +1375,31 @@ query RB select arrow_cast('1.2300', 'Decimal32(9, 4)'), arrow_cast(arrow_cast('1.2300', 'Decimal32(9, 4)'), 'Utf8') == '1.2300'; ---- 1.2300 true + +# Regression for https://github.com/apache/datafusion/issues/24850: coercing +# Decimal256(76, 0) against Decimal256(76, 52) needs precision 128, computed in +# `i8` before. Columns, not literals: literals are const-folded at plan time and +# hit the same overflow inside arrow's decimal cast, which is a separate bug. + +statement ok +CREATE TABLE decimal256_wide(a DECIMAL(76,0), b DECIMAL(76,52)); + +statement ok +set datafusion.explain.logical_plan_only = true; + +query TT +EXPLAIN SELECT a < b FROM decimal256_wide; +---- +logical_plan +01)Projection: CAST(decimal256_wide.a AS Decimal256(76, 52)) < decimal256_wide.b +02)--TableScan: decimal256_wide projection=[a, b] + +statement ok +RESET datafusion.explain.logical_plan_only; + +query B +SELECT a < b FROM decimal256_wide; +---- + +statement ok +DROP TABLE decimal256_wide; --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
