Fix AvgGroupsAccumulator::size() self-overcount - #25117
Draft
kosiew wants to merge 3 commits into
Draft
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.
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
1 participant
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.