sunchao commented on PR #25428: URL: https://github.com/apache/datafusion/pull/25428#issuecomment-5745999512
Rebased onto main at `925d7f8ff`; the PR now targets `main`, with head `ede48fcdd`. The closed #25172 accounting patch is excluded. Widening retains the original admission grant and makes no extra pool allocation, so this optimization does not depend on that patch. The rebase also preserves #25406's released-cursor handling and partial-output compaction. All three inline suggestions are addressed, and the description now states the containment explicitly: only the five aggregate opt-ins can reach a widened spill-only intermediate merge and its new batch budget. Standalone sorts, mixed in-memory merges, and final replay do not use that budget. Fresh validation on the rebased head is green: - [Regular CI](https://github.com/apache/datafusion/actions/runs/35474632180): all jobs passed, including Clippy and the workspace/CLI suites; all 34 PR checks are green. - [Extended CI](https://github.com/apache/datafusion/actions/runs/35474638916): 12,047 Rust tests passed, 8 ignored, and 522/522 SQL files completed. All four seeded Greedy/Fair concurrent-aggregate cases passed. The unfiltered SQL suite includes `aggregate_memory_spill.slt` with Case G at four partitions, 1 MiB, and default fan-in. The hash-collision and SQLite jobs passed too. I also checked the suggested 8 MiB explanation with temporary instrumentation on the exact historical head behind those numbers (`0ee7a4d06`). Widening was active: **7 → 14 inputs**, read-ahead **2 → 1**, and the unchanged **3,670,016-byte grant**. The intermediate merge rewrote **1,720,320 rows in 210 full 8,192-row batches**, with **zero budget-triggered flushes**. Saving one file without reducing the reported spilled row/byte volume explains the limited I/O benefit. This does not identify the cause of the measured +8.3% timing regression; I have not profiled CPU/read-ahead costs. At 16 MiB there was no intermediate merge: all nine runs fit the final merge. Both diagnostic results were exactly 2,000,000. The description labels the old benchmark and 160-repetition stress results as historical. Local compilation and fresh main/head timings are blocked because the configured registry does not expose main's locked Arrow 60.0.0 dependencies. I left main's dependencies unchanged and used GitHub CI for the fresh compiled validation above; I am not claiming a local all-feature Clippy pass or a rerun of the 160-repetition stress test on this head. -- 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]
