fix: Handle potential overflow in internal state for avg(decimal)#22714
Merged
Conversation
github-actions
Bot
added
sqllogictest
kosiew
requested changes
Jun 5, 2026
kosiew
left a comment
kosiew
left a comment
Contributor
There was a problem hiding this comment.
Thanks for working on this. The widening approach for Decimal32/64 looks good, but I think there are still correctness gaps around Decimal128 accumulation that need to be addressed before this can land. I also have a couple of non-blocking suggestions around SLT normalization and reducing duplication in the decimal averaging paths.
| target_precision: *target_precision, | ||
| target_scale: *target_scale, | ||
| })), | ||
| ) => Ok(Box::new(DecimalAvgAccumulator::<Decimal128Type>::new( |
Contributor
There was a problem hiding this comment.
I think there is still a correctness issue here for Decimal128 AVG. The accumulator state remains Decimal128Type, and the new summation path uses add_wrapping (for example at lines 651, 672, and 947). That means intermediate overflow can still silently wrap before DecimalAverager gets a chance to run.
For example, avg(arrow_cast('999999999999999999999999999999.9999', 'Decimal128(34, 4)')) over roughly 20,000 rows has a valid Decimal128(38, 8) result, but the intermediate Decimal128 sum exceeds i128::MAX and wraps along the way. At that point the average is already corrupted even though the final result would be representable.
Could we widen the Decimal128 accumulation state (for example to Decimal256) or otherwise use checked/compensated accumulation so intermediate overflow does not invalidate averages whose final result fits?
Contributor
Author
There was a problem hiding this comment.
So it currently doesn't compile because the current code here assumes that the type we use for the sum can be Into<I>, but turns out that doesn't exist for i256 and i128, I opened a PR to arrow, which might delay this part but I'll fix everything else.
Contributor
There was a problem hiding this comment.
I think the same issue still exists for AVG(DISTINCT) on decimals. The distinct path constructs DecimalDistinctAvgAccumulator::<Decimal32Type>, Decimal64Type, and Decimal128Type, and those accumulators still sum distinct values in the native type using wrapping arithmetic.
AVG(DISTINCT) is still an average, so it can hit the same intermediate overflow problem whenever the average is representable but the sum is not. Could the widening/state-type fix be applied here as well? Otherwise it would be good to explicitly narrow the supported contract and add tests that document the unsupported behavior.
Contributor
Author
There was a problem hiding this comment.
I'll take a look, I tried to keep this PR a bit smaller but I definitely suspect it has the same issue.
| DataType::Float16 | ||
| | DataType::Float32 | ||
| | DataType::Float64 | ||
| | DataType::Decimal32(_, _) |
Contributor
There was a problem hiding this comment.
Small suggestion: mapping Decimal32/Decimal64 globally to DFColumnType::Float makes SLT comparisons approximate for these decimal types and could hide formatting or rounding regressions that would otherwise be caught.
If the motivation is only the affected power/round queries, would it make sense to keep exact/text comparisons there instead? At minimum, it may be worth documenting why all Decimal32/64 SLT output is now treated approximately.
Contributor
Author
There was a problem hiding this comment.
I think display should be exact, I even made sure to extend it to decimal32/64 here
Contributor
There was a problem hiding this comment.
# example of passing test (DFColumnType::Float makes SLT comparisons approximate for these decimal types) which should fail
+query R
+select arrow_cast('1.2300', 'Decimal32(9, 4)');
+----
+1.23
Contributor
Author
There was a problem hiding this comment.
I think this should be a different issue, I'll open it later today as I did #23161
| @@ -365,17 +370,27 @@ impl AggregateUDFImpl for Avg { | |||
| Decimal32(_sum_precision, sum_scale), | |||
Contributor
There was a problem hiding this comment.
Small refactoring suggestion: the Decimal32 and Decimal64 branches both follow the same pattern of creating a wider DecimalAverager, dividing by a widened count, and then try_from-ing back to the output native type.
It might be nice to factor that into a helper such as avg_decimal_with_wider_sum. That would encode the widening invariant in one place and help keep the accumulator and group-accumulator paths aligned over time.
Signed-off-by: Adam Gutglick <adamgsal@gmail.com>
AdamGS
force-pushed
the
adamg/fix-avg-decimal-overflow
branch
from
cdd0af7 to
7a695d2
Compare
Signed-off-by: Adam Gutglick <adamgsal@gmail.com>
AdamGS
force-pushed
the
adamg/fix-avg-decimal-overflow
branch
from
3bf7e53 to
f8f0ab8
Compare
AdamGS
commented
Jul 10, 2026
| /// of reach. `Decimal256` input near max precision is the exception: no wider | ||
| /// type exists, so its sum keeps only whatever headroom `Decimal256(76, _)` has | ||
| /// left, as before this budget was introduced. | ||
| const AVG_SUM_HEADROOM_DIGITS: u8 = 4 + 9; |
Contributor
Author
There was a problem hiding this comment.
This seems like a reasonable heuristic to me, while keeping reasonable performance in most cases. The alternative here would be to always use i256 for the intermediate sum which is much slower.
Contributor
|
run benchmarks |
Contributor
|
I think the tpch tests uses decimal and avg |
Contributor
|
(BTW thank you @kosiew for your never ending reviews -- it is very much appreciated) |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing adamg/fix-avg-decimal-overflow (90dd6ff) to 754e113 (merge-base) diff using: tpcds File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing adamg/fix-avg-decimal-overflow (90dd6ff) to 754e113 (merge-base) diff using: tpch File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing adamg/fix-avg-decimal-overflow (90dd6ff) to 754e113 (merge-base) diff using: clickbench_partitioned File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagetpch — base (merge-base)
tpch — branch
File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagetpcds — base (merge-base)
tpcds — branch
File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usageclickbench_partitioned — base (merge-base)
clickbench_partitioned — branch
File an issue against this benchmark runner |
Contributor
|
🚀 |
Contributor
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Rationale for this change
Fixes a bug with
avgthat currently prevents us from running TPC-DS q1. I think that this issues is masked by Parquet because the current implementations infers that columns as a Decimal128.What changes are included in this PR?
Rin sqllogictest, like the bigger decimal types, and fixes some tests that used?. (this can be a separate PR, but its seems very small).DecimalAvgAccumulatornow takes another type to hold its inner sum accumulator, which can be different than the input/output type.AVGimpl building blocks.Are these changes tested?
Additional SLT test that would've overflowed internally, and more focused unit tests for
AvgGroupsAccumulatorAre there any user-facing changes?
None, just more code that doesn't currently work and will work now.