Skip to content

fix: Address avg return type overflow for decimals - #24371

Merged
kosiew merged 2 commits into
apache:mainfrom
AdamGS:adamg/fix-24369
Aug 29, 2026
Merged

fix: Address avg return type overflow for decimals#24371
kosiew merged 2 commits into
apache:mainfrom
AdamGS:adamg/fix-24369

Conversation

@AdamGS

@AdamGS AdamGS commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Rationale for this change

AVG(Decimal(p, s)) derives a result with precision and scale increased by four. For example, AVG(Decimal32(9, 0)) returned Decimal32(9, 4), although the rescaled average can require 13 digits.

This caused valid averages near the input precision limit to fail with an overflow even though Decimal64(13, 4) can represent the result.

What changes are included in this PR?

  • Preserve the input decimal variant when the derived result fits, and widen it when necessary, while still clamping Decimal256 at MAX.
  • Allow decimal AVG accumulators to use different native types for the input, intermediate sum, and output.
  • Update SQL type assertions and affected logical-plan expectations.

Are these changes tested?

Regression tests and adjusted SLT-based tests.

Are there any user-facing changes?

Yes. The Arrow result type of AVG(decimal) may now be wider when the derived precision does not fit the input variant.

@github-actions github-actions Bot added sqllogictest SQL Logic Tests (.slt) functions Changes to functions implementation labels Aug 14, 2026
@codecov-commenter

codecov-commenter commented Aug 14, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 80.79470% with 29 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.51%. Comparing base (dd77ffc) to head (5550865).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
datafusion/functions-aggregate/src/average.rs 80.82% 3 Missing and 25 partials ⚠️
...egate-common/src/aggregate/avg_distinct/decimal.rs 80.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #24371      +/-   ##
==========================================
- Coverage   81.51%   81.51%   -0.01%     
==========================================
  Files        1123     1123              
  Lines      405139   405235      +96     
  Branches   405139   405235      +96     
==========================================
+ Hits       330242   330308      +66     
- Misses      55568    55578      +10     
- Partials    19329    19349      +20     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@AdamGS
AdamGS marked this pull request as ready for review August 24, 2026 18:51
@AdamGS AdamGS changed the title Address avg return type overflow for decimals fix: Address avg return type overflow for decimals Aug 26, 2026

@kosiew kosiew left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AdamGS, thanks for working on this. The changes look good overall. I left one small suggestion to add end-to-end coverage for the grouped AVG path.

32768.5 Decimal32(9, 4) 32768.5
32768.5 Decimal64(13, 4) 32768.5

# TPC-DS `store_returns.sr_return_amt` is Decimal32(7, 2). Its average needs

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we add a small GROUP BY SQLLogicTest using Decimal32(7, 2) and assert that the result type is Decimal64(11, 6)? The unit tests already exercise the grouped state conversion, but an end-to-end grouped query would also verify that the planner selects the groups accumulator and that it produces the widened Arrow output type.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done!

AdamGS added 2 commits August 29, 2026 11:07
Signed-off-by: Adam Gutglick <adamgsal@gmail.com>
Signed-off-by: Adam Gutglick <adamgsal@gmail.com>
@AdamGS

AdamGS commented Aug 29, 2026

Copy link
Copy Markdown
Contributor Author

@kosiew thank you for the quick review!

@kosiew
kosiew added this pull request to the merge queue Aug 29, 2026
@kosiew

kosiew commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

🚀
@AdamGS
Thank you for your contribution.

Merged via the queue into apache:main with commit 9ef3d47 Aug 29, 2026
38 checks passed
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.

avg(decimal) might lose precision and cause an overflow over return type

3 participants