sunchao opened a new pull request, #25428: URL: https://github.com/apache/datafusion/pull/25428
## Which issue does this PR close? Follow-up to [the review of #25383](https://github.com/apache/datafusion/pull/25383#pullrequestreview-5236833677), which suggested skipping replay headroom for intermediate multi-level merge passes. **Depends on #25383 and #25172.** This draft is stacked on a temporary upstream branch at #25172's exact head, `b0752560ff47f36ce8896574829082e51f79088c`, so the diff contains only this follow-up. Merge those prerequisites in that order, then rebase and retarget this PR to `main`. ## Rationale for this change An intermediate spill merge writes its output back to disk before aggregation resumes. Reserving equal memory for aggregate replay during that pass unnecessarily limits how many runs it can merge, causing more intermediate files and repeated spill I/O. For example, with 32 equal runs and enough memory for eight buffered inputs, the existing policy admits four inputs per pass. The measured fixture writes ten intermediate files and rewrites every input row twice. Allowing intermediate passes to admit eight inputs reduces this to four files and one rewrite per row. The final merge still needs space for the aggregate consuming its output. The accounting prerequisite matters: widening intermediate merges on #25383 alone makes both existing legacy aggregate replay tests exceed their 8 KiB reservation limit. Legacy aggregation retains a sibling reservation, which the old `FairSpillPool` admission check overlooks. #25172 accounts for that retained memory. The original peak-memory assertions are unchanged. ## What changes are included in this PR? The merge first tries the existing selection with replay headroom. If it admits only some of the remaining runs, it restores the selection and retries without replay headroom, retaining at least one pending run so the larger merge must write back to disk. Configured fan-in limits and the minimum number of inputs needed to make progress still apply. Split requests retain their existing behavior because splitting also establishes output batch-size limits for short or indivisible input batches. Every subsequent selection probes for a final merge with headroom again, preserving the existing minimum-merge fallback for indivisible rows. Callers that do not enable replay headroom retain their existing behavior. ## Are these changes tested? New tests cover larger intermediate merges, retaining a pending run, final-pass read-ahead reduction, configured fan-in, split retries, wide singleton batches, exact sorted output, an actual allocation from the replay allowance, and release of memory and temporary files. The deterministic fixture uses 32 interleaved Int64 runs, 256 rows per run, a 256-row output batch size, and a pool of 32 times the largest input batch's recorded memory. It counts actual intermediate spill writes, excluding the original input spills: | Metric | Before | After | | --- | ---: | ---: | | Intermediate merge passes / files | 10 | 4 | | Rows written by intermediate merges | 16,384 | 8,192 | | Intermediate IPC bytes written, including framing | 143,696 | 71,712 | These are spill-work measurements for this fixture, not elapsed-time or peak-disk-space measurements. The regression test prints the counters with `--nocapture`. Fresh local validation passed on the accounting prerequisite plus this change: - 2,272 physical-plan tests, including the unchanged legacy aggregate peak-memory checks. - 2,281 core/CLI tests and all 520 SQL logic test files. - `cargo fmt --all` and `cargo clippy --all-targets --all-features -- -D warnings`. - Full `./dev/rust_lint.sh`, including private Rust documentation and local Markdown links. The before/after measurements used separate binaries, with the before build at `b0752560`. Local validation used Rust 1.98.1 and the prerequisites' unchanged dependency lockfile. This validates the stacked branch; GitHub CI results are separate. ## Are there any user-facing changes? Spilling aggregations can perform fewer intermediate merges and spill writes. Final replay retains the existing headroom policy and indivisible-row fallback. SQL results, configuration, and public APIs are unchanged. -- 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]
