You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "andygrove (via GitHub)" <gi...@apache.org> on 2026/09/27 17:13:27 UTC
[PR] [branch-55] fix: leave memory for aggregate spill replay (#25383) [datafusion]
andygrove opened a new pull request, #25814:
URL: https://github.com/apache/datafusion/pull/25814
## Which issue does this PR close?
- Part of #25758
- Related to #25423 and #25804
## Rationale for this change
On `branch-55`, a final aggregate that has spilled can fail with `ResourcesExhausted` while replaying its spill files. The spill merge reserves as much memory as the pool will give it, and the `OrderedFinalAggregateStream` that consumes the merged rows cannot spill, so when the merge takes the whole allowance the replay's first resize fails. #25423 is the same failure in `aggregate_memory_spill.slt`.
DataFusion Comet, which uses 55.1.0, hits this under memory pressure (apache/datafusion-comet#6254). A `GROUP BY` over 2M distinct 128-character strings fails every time with a 24 MiB per-task budget.
## What changes are included in this PR?
This PR backports #25383 from @sunchao to the `branch-55` line. When the merge feeds an aggregate replay, it only takes spill files that would still fit if it reserved twice their buffers, and releases the extra before the replay starts.
This PR is stacked on #25805, the backport of #24740, which #25383 builds on. The first commit is the one from #25805, and only the second is new. I will rebase once #25805 merges.
On top of #25805, `multi_level_merge.rs` and `streaming_merge.rs` match `main` at #25383 exactly. Two changes were needed for `branch-55`:
- `ordered_single_stream.rs` (#24259) does not exist on `branch-55`, so its one-line change is dropped. There, an ordered `Single` aggregate uses `GroupedHashAggregateStream`, which this PR covers, so the `ordered_single` case of `migrated_aggregate_spill_merge_leaves_memory_for_replay` expects `StreamType::GroupedHash`.
- `test_order_is_retained_when_spilling` keeps `branch-55`'s `new_spill_ctx` and drops the accumulator phase time assertion, which needs #24423. It uses the same 1024-byte limit as #25383.
## Are these changes tested?
Yes. The backport includes the tests from #25383. On this branch I ran:
- `cargo test -p datafusion-physical-plan --lib`
- the `aggregate_memory_spill`, `ordered_aggregate_spill`, `nested_loop_join_spill` and `sort_merge_join_spill` sqllogictests
- `cargo fmt --all -- --check` and `cargo clippy -p datafusion-physical-plan -p datafusion-execution --all-targets --all-features -- -D warnings`
I also built Comet against this branch. Its reproduction of apache/datafusion-comet#6254 fails on 55.1.0 and passes here, with the spill merge fan-in left unlimited.
## Are there any user-facing changes?
Aggregations that spill leave memory for processing the merged rows, so they fail less often under memory pressure. This can mean smaller merges and more merge passes. No public API changes.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] [branch-55] fix: leave memory for aggregate spill replay (#25383) [datafusion]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove closed pull request #25814: [branch-55] fix: leave memory for aggregate spill replay (#25383)
URL: https://github.com/apache/datafusion/pull/25814
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] [branch-55] fix: leave memory for aggregate spill replay (#25383) [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #25814:
URL: https://github.com/apache/datafusion/pull/25814#issuecomment-5858162986
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/25814?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `91.49701%` with `71 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 81.23%. Comparing base ([`9f5714b`](https://app.codecov.io/gh/apache/datafusion/commit/9f5714b0ce725b6af2d27cef4a1dc2aaa4e76cc1?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`f47fe56`](https://app.codecov.io/gh/apache/datafusion/commit/f47fe5616543df9d88363ddb164864d15a63f0a1?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/datafusion/pull/25814?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...ion/execution/src/memory\_pool/merge\_memory\_pool.rs](https://app.codecov.io/gh/apache/datafusion/pull/25814?src=pr&el=tree&filepath=datafusion%2Fexecution%2Fsrc%2Fmemory_pool%2Fmerge_memory_pool.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9leGVjdXRpb24vc3JjL21lbW9yeV9wb29sL21lcmdlX21lbW9yeV9wb29sLnJz) | 90.00% | [11 Missing and 21 partials :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/25814?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...usion/physical-plan/src/sorts/multi\_level\_merge.rs](https://app.codecov.io/gh/apache/datafusion/pull/25814?src=pr&el=tree&filepath=datafusion%2Fphysical-plan%2Fsrc%2Fsorts%2Fmulti_level_merge.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9waHlzaWNhbC1wbGFuL3NyYy9zb3J0cy9tdWx0aV9sZXZlbF9tZXJnZS5ycw==) | 92.41% | [9 Missing and 18 partials :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/25814?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [datafusion/physical-plan/src/aggregates/mod.rs](https://app.codecov.io/gh/apache/datafusion/pull/25814?src=pr&el=tree&filepath=datafusion%2Fphysical-plan%2Fsrc%2Faggregates%2Fmod.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9waHlzaWNhbC1wbGFuL3NyYy9hZ2dyZWdhdGVzL21vZC5ycw==) | 88.88% | [5 Missing and 7 partials :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/25814?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## branch-55 #25814 +/- ##
=============================================
+ Coverage 81.21% 81.23% +0.02%
=============================================
Files 1110 1111 +1
Lines 388499 389274 +775
Branches 388499 389274 +775
=============================================
+ Hits 315513 316226 +713
- Misses 54434 54452 +18
- Partials 18552 18596 +44
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/25814?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org