feat/spill: implement spill support for merge_order - #24656
Merged
mergify[bot] merged 39 commits intoMay 29, 2026
Conversation
- reuse spill buffers and avoid redundant allocations - track cached spill memory incrementally - use heap-based spill reader merging - keep sort_spill BVT coverage and spill-threshold wiring Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
aunjgr
changed the title
aunjgr
changed the title
- append spill batches to active runs when ordering allows - rotate active run by spill append target policy - finalize active run before final spill merge - add policy and appendability unit tests Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- remove unused shouldSpill helper - drop tautological outOrder nil check in spill merge path Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Delete now-unused in-memory and spill selector helpers, inline the small remaining reader-choice logic at call sites, and drop tests tied to the removed wrappers. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Remove single-use batch append, order-column evaluation, and memory-usage accessor wrappers, leaving the in-memory and spill paths direct and compact. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Remove the spillInputBatch dispatch layer and its spillBatchAsRun fallback by inlining the current-batch spill branch directly into Call and spillCachedRuns. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add targeted mergeorder tests for spill append paths, merge/read helpers, and edge branches so PR patch coverage crosses the CI gate. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Update DropDatabase relation-collection tests to expect SetSnapshotTS during deferred snapshot restoration. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Resolve DropDatabase test merge conflict by keeping SetSnapshotTS gomock.Any() expectations in the merged test updates. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
heni02
approved these changes
May 29, 2026
ouyuanning
approved these changes
May 29, 2026
XuPeng-SH
approved these changes
May 29, 2026
XuPeng-SH
left a comment
XuPeng-SH
left a comment
Contributor
There was a problem hiding this comment.
Reviewed from multiple angles on the latest revision. I focused on correctness around merge_order spill run lifecycle, append boundary checks, final merge/drain behavior, and the sort_spill_mem plan/pipeline plumbing. I did not find a blocking issue.
Contributor
Merge Queue Status
This pull request spent 24 minutes 54 seconds in the queue, with no time running CI. Waiting for
All conditions
ReasonThe merge conditions cannot be satisfied due to failing checks HintYou may have to fix your CI before adding the pull request to the queue again. |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
Merge Queue Status
This pull request spent 55 seconds in the queue, including 5 seconds running CI. Required conditions to merge
|
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.
What type of PR is this?
Which issue(s) this PR fixes:
issue #23353 #22374
What this PR does / why we need it:
This PR adds spill support to
merge_order(ORDER BY final merge) for the first time, enabling external sort when data exceeds memory.Key points:
pkg/sql/colexec/mergeorder/spill.gobacked byfileservice.MutableFileService.sort_spill_memfrom session variable -> plan -> pipeline so spill threshold is configurable and propagated.test/distributed/cases/qexec/sort_spill.sql) that proves spill happens viaEXPLAIN ANALYZE/SpillSize=.tail <= next-firstunder existing comparator semantics, rotate on boundary violation or policy target.Special notes for your reviewer:
This is the initial spill implementation for
merge_order; optimization changes are included in the same PR.Additional documentation e.g., usage docs, design docs:
None.