Fix AvgGroupsAccumulator::size() self-overcount - #25117
Merged
Merged
Conversation
- Fixed AvgGroupsAccumulator::size() → no inline Vec descriptor charges. - Added empty + grown-state size test.
… Decimal256 conversion - Added a new test case in `average.rs` that validates Decimal128 inputs are correctly widened to Decimal256 when computing sums using native i256 arithmetic. - The test verifies correct behavior for typical values, large values, and edge cases where the sum exceeds the Decimal128 range. - Includes assertions that ensure the resulting Decimal256 value matches the expected precision and accuracy. - Ensures the Decimal library’s mixed‑precision handling (Decimal128 → Decimal256) works reliably with the native i256 size. - Extends the existing test suite, improving confidence in the library’s widened decimal operations.
… with Decimal128(30, 4) result - Updated `avg_groups_size_uses_sum_native_type` to compute **real AVG types** instead of approximating types. - **Input type** changed to `Decimal128(26, 0)` for precise group size representation. - **Intermediate `sum` type** upgraded to `Decimal256(76, 0)` to avoid overflow during aggregation. - **Returned average type** now uses `Decimal128(30, 4)`, providing sufficient precision and scale for accurate average calculations. - Adjustments ensure that the function respects the native decimal behavior required for downstream analytics and reporting.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #25117 +/- ##
==========================================
- Coverage 81.80% 81.80% -0.01%
==========================================
Files 1130 1130
Lines 417754 417788 +34
Branches 417754 417788 +34
==========================================
+ Hits 341754 341778 +24
- Misses 55875 55877 +2
- Partials 20125 20133 +8 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…rom_ref to avoid unnecessary clone
kosiew
marked this pull request as ready for review
September 10, 2026 09:11
kumarUjjawal
approved these changes
Sep 10, 2026
kumarUjjawal
left a comment
kumarUjjawal
left a comment
Contributor
There was a problem hiding this comment.
Thank you @kosiew
LGTM!
Contributor
Author
|
🚀 |
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
3 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?
sizefunctions #23393Rationale for this change
GroupsAccumulator::size()is expected to report retained state owned by the accumulator without counting the accumulator object itself.AvgGroupsAccumulator::size()was also charging for the inlineVecdescriptors forcountsandsums, even though those descriptors are part of the accumulator object rather than separately retained heap state. This caused AVG group accumulators to over-report their memory usage.What changes are included in this PR?
This PR:
size_of::<Vec<_>>()charges for the inlinecountsandsumsvector descriptors fromAvgGroupsAccumulator::size().countsandsums.null_state.size().Decimal128input /Decimal256sum case.Are these changes tested?
Yes. This PR adds the following focused unit tests in
datafusion/functions-aggregate/src/average.rs:avg_groups_size_excludes_accumulator_storageavg_groups_size_uses_sum_native_typeThe first verifies that an empty accumulator reports zero retained state and that, after group state is allocated,
size()equals the capacities ofcountsandsumsplusnull_state.size().The second verifies that memory accounting uses the sum accumulator's native type by checking a
Decimal128AVG whose sum state usesDecimal256.No test execution results are shown in the patch.
Are there any user-facing changes?
No direct user-facing API or query-result changes are included.
This changes the memory-size accounting reported by
AvgGroupsAccumulatorso that it follows theGroupsAccumulator::size()contract and no longer includes inline vector descriptors.LLM-generated code disclosure
This PR includes LLM-generated code and comments. All LLM-generated content has been manually reviewed.