kosiew opened a new pull request, #25117:
URL: https://github.com/apache/datafusion/pull/25117

   ## Which issue does this PR close?
   
   * Part of #23393
   
   ## Rationale 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 inline `Vec` 
descriptors for `counts` and `sums`, 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:
   
   * Removes the `size_of::<Vec<_>>()` charges for the inline `counts` and 
`sums` vector descriptors from `AvgGroupsAccumulator::size()`.
   * Keeps accounting for the heap capacities of `counts` and `sums`.
   * Keeps accounting for retained null-tracking state through 
`null_state.size()`.
   * Adds coverage verifying that the sum buffer is accounted for using the 
actual sum native type, including the `Decimal128` input / `Decimal256` sum 
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_storage`
   * `avg_groups_size_uses_sum_native_type`
   
   The first verifies that an empty accumulator reports zero retained state and 
that, after group state is allocated, `size()` equals the capacities of 
`counts` and `sums` plus `null_state.size()`.
   
   The second verifies that memory accounting uses the sum accumulator's native 
type by checking a `Decimal128` AVG whose sum state uses `Decimal256`.
   
   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 `AvgGroupsAccumulator` 
so that it follows the `GroupsAccumulator::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.
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to