Skip to content

refactor: one admission rule for every spill merge - #26984

Draft
jayzhan211 wants to merge 4 commits into
apache:mainfrom
jayzhan211:poc/spill-batch-bound
Draft

jayzhan211 wants to merge 4 commits into
apache:mainfrom
jayzhan211:poc/spill-batch-bound

Conversation

@jayzhan211

@jayzhan211 jayzhan211 commented Nov 1, 2026 •

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Stacked on #26987. Only the last commit, "refactor: one admission rule for every spill merge", is new here; I'll rebase once #26987 merges.

Rationale for this change

After #26987, the spill merge still has two options that only the aggregate sets: with_replay_headroom (#25383) and with_intermediate_merge_sizing (#25428). #26711 asks to remove options like these, and #26264 proposes a third, with_fan_in_headroom, so that a spilled sort also leaves memory for other operators.

This PR replaces all three with one rule that every caller gets: each run a merge reads must leave as much again as the merge takes beyond the workspace it already holds. This is the formula from #26264.

  • The aggregate holds no workspace, so it gets its replay headroom as before.
  • The sort holds sort_spill_reservation_bytes, so only what it takes beyond that is doubled.

What changes are included in this PR?

What is the testing strategy for this PR?

Are there any user-facing changes?

Query results don't change. A spilled sort's merge now leaves headroom beyond its workspace, as #26264 proposes. This applies even when nothing else is using memory, so it can add merge passes. Sorting 3M rows by an integer and a short string on one partition, with sort_spill_reservation_bytes = 4 MB:

Memory limit Rows written to spill files, main This PR
8 MB 8.95M 11.62M
24 MB 3.00M 4.80M

Intermediate merge sizing now applies to sorts as well as aggregates.

@github-actions github-actions Bot added core Core DataFusion crate physical-plan Changes to the physical-plan crate documentation Improvements or additions to documentation sqllogictest SQL Logic Tests (.slt) labels Nov 1, 2026
@jayzhan211
jayzhan211 force-pushed the poc/spill-batch-bound branch from 97b4c1f to ba3c3da Compare November 1, 2026 06:12
@jayzhan211 jayzhan211 changed the title fix: bound spilled sort and aggregate runs by bytes, with one merge admission rule refactor: one admission rule for every spill merge Nov 1, 2026
@jayzhan211 jayzhan211 closed this Nov 1, 2026
@jayzhan211 jayzhan211 reopened this Nov 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core Core DataFusion crate documentation Improvements or additions to documentation physical-plan Changes to the physical-plan crate sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant