kosiew commented on code in PR #25188:
URL: https://github.com/apache/datafusion/pull/25188#discussion_r4101267167
##########
datafusion/physical-plan/src/aggregates/aggregate_hash_table/common.rs:
##########
@@ -396,9 +396,16 @@ impl<AggrMode> AggregateHashTable<AggrMode> {
let batch = RecordBatch::try_new(state_schema, output)?;
debug_assert!(batch.num_rows() > 0);
- // `emit(EmitTo::All)` resets accumulator state. Explicitly shrink the
- // key/index buffers too so the memory reservation can be released
- // before the batch is sorted for spilling.
+ // State emission should reset accumulators, but spill recovery must
+ // release every emitted allocation even for an accumulator that
retains
+ // capacity. Rebuild the accumulator set before returning the state
batch.
+ state.accumulators = state
Review Comment:
[f432ac1](https://github.com/apache/datafusion/pull/25188/commits/f432ac10e961fbd42dbb277a21d1315051dcf7a0)
- removed. The corrected empty `GroupValues` accounting is accommodated by the
700 B grouping-sets spill budget.
--
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]
