Skip to content

fix: leave memory for aggregate spill replay - #25383

Open
sunchao wants to merge 1 commit into
mainfrom
dev/chao/codex/pr25172-replay-headroom
Open

sunchao wants to merge 1 commit into
mainfrom
dev/chao/codex/pr25172-replay-headroom

Conversation

@sunchao

@sunchao sunchao commented Sep 16, 2026

Copy link
Copy Markdown
Member

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:

  • 2,263 physical-plan tests and 2,281 core/CLI tests passed.
  • The existing permanent-pressure regression passed with its original success expectation.
  • All 520 SQL logic test files passed.
  • cargo fmt --all and strict all-target/all-feature Clippy passed.
  • Full ./dev/rust_lint.sh passed, including private Rust documentation and local Markdown links.

Local validation uses Rust 1.98.1 and upstream revision 22651d24 with 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.

@github-actions github-actions Bot added the physical-plan Changes to the physical-plan crate label Sep 16, 2026
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 91.94313% with 34 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.02%. Comparing base (22651d2) to head (1ff4126).
⚠️ Report is 20 commits behind head on main.

Files with missing lines Patch % Lines
...usion/physical-plan/src/sorts/multi_level_merge.rs 92.18% 9 Missing and 11 partials ⚠️
datafusion/physical-plan/src/aggregates/mod.rs 90.90% 6 Missing and 8 partials ⚠️
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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

physical-plan Changes to the physical-plan crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants