jayzhan211 opened a new pull request, #25538: URL: https://github.com/apache/datafusion/pull/25538
## Which issue does this PR close? - Part of #25537 (unified spill-replay driver for grouped aggregation). Follow-up to #22710. ## Rationale for this change The four grouped aggregation streams that can spill — `FinalHashAggregateStream`, `SingleHashAggregateStream`, `OrderedSingleAggregateStream` and `OrderedFinalAggregateStream` — each define their own spill context (`FinalSpillContext`, `SingleSpillContext`, `OrderedSingleSpillContext`, `OrderedFinalSpillContext`). The four types have the same seven fields and the same three operations: - build the spill sort key and the aggregate configuration used for replay, - sort the table's state batch and write it as one spill file, - merge all runs with `StreamingMergeBuilder` and replay them through `OrderedFinalAggregateStream::new_with_input_and_metrics`. The bodies are copies of each other; the only real differences are constructor-time: | | sort key | replay aggregate | |---|---|---| | Final hash | group columns in natural order | same aggregate | | Single hash | group columns in natural order | `Single → Final`, `group_by.as_final()` | | Ordered final | already-ordered columns first, then the rest | same aggregate | | Ordered single | already-ordered columns first, then the rest | `Single → Final`, `group_by.as_final()` | Both axes are functions of values the constructor already receives (`AggregateMode` and `InputOrderMode`), and the natural order is just the ordered-columns-first formula with no ordered columns. ## What changes are included in this PR? No behaviour change. - Add `aggregates/spill.rs` with a single non-generic `AggregateSpill` (`try_new`, `has_spills`, `spill`, `into_replay_stream`). It is non-generic because the only thing a spill needs from a table is the batch returned by `take_state_batch()`, so the caller passes that batch in. - Delete the four per-stream spill context types and use `AggregateSpill` instead. - Spill request descriptions (`"FinalHashAggregateSpill"` etc.) and memory consumer names are unchanged. The text of four `internal_err!` messages that cannot be reached by users is now shared. Net: 4 stream files −665 lines, +1 new file of ~235 lines. This is the first step towards a single spill-replay driver for these streams (see the linked issue), but it stands on its own. ## What is the testing strategy for this PR? Existing tests: the aggregate unit tests in `aggregates/mod.rs` and `ordered_final_stream.rs` (spill + replay, OOM, drop/cancel, memory accounting), `aggregate_memory_spill.slt`, `ordered_aggregate_spill.slt`, the `memory_limit` integration tests and the aggregate fuzz tests. No new tests since there is no new behaviour. ## Are there any user-facing changes? No. -- 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]
