Repository navigation
fix: emit completed groups when a fully sorted partial aggregate runs out of memory - #26576
Merged
jayzhan211 merged 2 commits intoOct 23, 2026
Merged
Conversation
lyne7-sc
pushed a commit
to lyne7-sc/datafusion
that referenced
this pull request
Oct 23, 2026
## Which issue does this PR close? - N/A ## Rationale for this change The semver check previously compared the PR against the current state of `apache/main`, and did so by doing a full `git` checkout, including the entire repository history. This approach has two downsides: 1. It is expensive: cloning the entire repo history takes ~9 sec locally, versus ~0.7 for a shallow checkout. 2. There's a race condition which results in comparing the PR against the wrong base when assessing whether the PR breaks semantic versioning. On (2): when a PR is opened or updated, GitHub creates a test merge commit that merges the PR into main as of that moment (T0). Now suppose that there is a delay between the CI jobs being queued and the semver job actually running, so that when the semver job runs, `HEAD` is now at T1. The previous version of this check would compare the PR with T1, not T0. For example, if a commit between T0 and T1 added a new API, the semver check would claim that the PR _removes_ that API. Fix this by comparing the merge commit with its first parent instead, which is T0 in the notation above. ## What changes are included in this PR? See above. ## What is the testing strategy for this PR? CI / dev tooling change only. Tested locally on six open PRs (apache#26565, apache#26000, apache#26582, apache#25874, apache#26534, apache#26576): fetched each PR's merge ref at depth 2 and ran both steps against `HEAD^1`. The crate lists and semver results match CI, including two PRs that branched 66 and 154 commits behind main. ## Are there any user-facing changes? No.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Rationale for this change
A
Partialaggregate over input sorted on all grouping keys fails withResourcesExhaustedin a shared memory pool, although emitting the completed groups would leave only the active group in memory. This is a regression from 54, see the issue for details.What changes are included in this PR?
When the reservation after an input batch is refused under full group clustering,
ClusteredPartialAggregateStreamnow emits the completed groups, the same rows the success path emits. It returns the error only when no group is complete.What is the testing strategy for this PR?
New unit test
completed_groups_under_memory_pressureinclustered_partial_stream.rs. It runs the stream with a limit that fits the table between batches but not after aggregating a batch, and compares the output with an unlimited run. It fails onmainwithResourcesExhausted. It is a unit test because a SQL memory limit cannot target one stream's table size.Are there any user-facing changes?
These queries now complete instead of failing. No API changes.