Shahzeb-DataEngineer opened a new pull request, #26652:
URL: https://github.com/apache/datafusion/pull/26652
## Which issue does this PR close?
- Closes #26648
## Rationale for this change
`sum()` over a DECIMAL column silently wraps on overflow and returns a wrong
result. Summing two maximum `DECIMAL(38, 0)` values returns a negative number
with 39 digits under a declared `Decimal128(38, 0)` result type:
```sql
SELECT sum(v) FROM (VALUES
(CAST('99999999999999999999999999999999999999' AS DECIMAL(38,0))),
(CAST('99999999999999999999999999999999999999' AS DECIMAL(38,0)))) t(v);
-- -140282366920938463463374607431768211458
```
This is inconsistent with the rest of DataFusion: scalar decimal `+` and `*`
raise an Arrow error on overflow, and `avg()` over the same data fails cleanly
with "Arithmetic Overflow in AvgAccumulator". A silent wrong answer from an
aggregate is worse than an error, so `sum()` should error the same way.
Integer sums are intentionally unchanged: wrapping integer addition is
DataFusion's documented lenient (non-ANSI) behavior. Only the decimal
accumulator paths are affected.
## What changes are included in this PR?
- `datafusion/functions-aggregate-common/src/utils.rs`: add
`decimal_precision`, `add_decimal_checked` and `sub_decimal_checked` helpers.
Decimal addition is checked against both the native type width and the declared
precision (the bounded sum of two in-range decimals can exceed the declared
precision without overflowing the native type, e.g. two `DECIMAL(38, 0)` maxima
in an `i128`), and reports `Arithmetic Overflow in SumAccumulator`, mirroring
the existing AvgAccumulator wording.
- `datafusion/functions-aggregate/src/sum.rs`: `SumAccumulator` and
`SlidingSumAccumulator` accumulate decimal inputs with the checked helpers in
`update_batch`, `merge_batch` and `retract_batch` instead of the wrapping
kernel sum. `Sum::groups_accumulator_supported` now returns false for decimal
return types, because the primitive groups accumulator applies an infallible
operation and cannot report overflow; decimal GROUP BY sums fall back to the
fallible accumulator.
-
`datafusion/functions-aggregate-common/src/aggregate/sum_distinct/numeric.rs`:
`DistinctSumAccumulator::evaluate` sums decimal values with the checked helper.
## What is the testing strategy for this PR?
- New Rust unit tests: `decimal128_sum_overflow_errors`,
`decimal128_sum_below_precision_limit_is_exact` and
`decimal128_sliding_sum_overflow_errors` in `sum.rs`, and
`decimal128_distinct_sum_overflow_errors` in `sum_distinct/numeric.rs`.
- New sqllogictest cases at the end of
`datafusion/sqllogictest/test_files/decimal.slt`: two-row Decimal128 overflow
(plain, GROUP BY, and SUM(DISTINCT)), a Decimal256 overflow, and a
large-but-valid sum that still computes exactly.
- Ran: `cargo test -p datafusion-functions-aggregate sum`, `cargo test -p
datafusion-functions-aggregate-common --lib`, and the `decimal` sqllogictests;
all pass. `cargo fmt --all` and clippy on the touched crates are clean.
## Are there any user-facing changes?
Yes: `sum()` over a DECIMAL now returns an `Arithmetic Overflow in
SumAccumulator` execution error when the result does not fit in the result
type, instead of returning a silently wrapped value. Non-overflowing decimal
sums are unchanged, and integer sums keep their existing wrapping behavior.
This PR was prepared with AI assistance; all changes and tests were reviewed
and verified by me.
--
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]
