Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #25383 +/- ##
==========================================
+ Coverage 81.92% 82.02% +0.09%
==========================================
Files 1135 1136 +1
Lines 427772 439539 +11767
Branches 427772 439539 +11767
==========================================
+ Hits 350456 360522 +10066
- Misses 56373 58242 +1869
+ Partials 20943 20775 -168 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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?
Prerequisite for #25172, which will enforce one memory allowance across sibling reservations.
Rationale for this change
Aggregate spill replay merges sorted files while rebuilding aggregate state. Both components share a memory consumer. If the merge reserves the whole allowance, the aggregate cannot process the rows it receives.
What changes are included in this PR?
A private merge option asks the actual memory pool to admit merge buffers plus an equal amount of replay headroom, then releases the spare capacity before starting the merge. All supported aggregate replay paths enable it. Ordinary sort callers retain their existing behavior.
The merge can split oversized batches. If a row cannot shrink, only the minimum merge may retry without spare headroom, and the real pool must still admit it. Decoded-size inspection and actual batch-row limits prevent unnecessary spill rewrites and under-reserved output buffers. Legacy replay also releases unused capacity in its newly created grouping state.
This uses one admission policy for all pools and adds no public memory-pool API.
What is the testing strategy for this PR?
Regression coverage checks all aggregate replay implementations, competing consumers, growing ARRAY_AGG state, indivisible rows, short and odd-sized batches, decoded string views, full disk quotas, exact output, and memory/disk cleanup.
Validated replay alone, with the existing FairSpillPool implementation:
cargo fmt --alland strict all-target/all-feature Clippy passed../dev/rust_lint.shpassed, including private Rust documentation and local Markdown links.Local validation uses Rust 1.98.1 and upstream revision
22651d24with its unchanged dependency lockfile; newer main's dependency versions are unavailable in the local registry. Merge compatibility with current main is checked separately, and GitHub CI validates its merged revision.Are there any user-facing changes?
Aggregate replay leaves memory available for processing merged rows. Temporary headroom reservations can increase recorded reservation peaks without allocating additional data buffers. The headroom is released before replay, so later concurrent allocations can still cause resource exhaustion.
The dependent accounting PR #25172 must merge after this prerequisite.