Skip to content

fix: Handle potential overflow in internal state for avg(decimal)#22714

Open
AdamGS wants to merge 1 commit into
apache:mainfrom
AdamGS:adamg/fix-avg-decimal-overflow
Open

fix: Handle potential overflow in internal state for avg(decimal)#22714
AdamGS wants to merge 1 commit into
apache:mainfrom
AdamGS:adamg/fix-avg-decimal-overflow

Conversation

@AdamGS
Copy link
Copy Markdown
Contributor

@AdamGS AdamGS commented Jun 2, 2026

Which issue does this PR close?

Rationale for this change

Fixes a bug with avg that 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?

  1. Mark Decimal32/64 as R in sqllogictest, like the bigger decimal types, and fixes some tests that used ?. (this can be a separate PR, but its seems very small).
  2. Adds a test for decimal32 overflow
  3. DecimalAvgAccumulator now takes another type to hold its inner sum accumulator, which can be different than the input/output type.
  4. Decimal32 and Decimal 64 use i64 and i128 (respectively) to prevent an overflow (should i128 use i256 here?).
  5. Adds some unit tests for the AVG impl building blocks.

Are these changes tested?

Additional SLT test that would've overflowed internally, and more focused unit tests for AvgGroupsAccumulator

Are there any user-facing changes?

None, just more code that doesn't currently work and will work now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

functions Changes to functions implementation sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Decimal average can overflow because its inner intermediate sum state overflows its storage size

1 participant