Repository navigation
planner, executor: support access path keep order with IN conditions using merge sort (#62694) | tidb-test=pr/2699 - #66746
Merged
ti-chi-bot[bot] merged 4 commits intoMar 11, 2026
Conversation
…ons using merge sort (pingcap#62694) close pingcap#34882, close pingcap#63449
ti-chi-bot
Bot
added
do-not-merge/cherry-pick-not-approved
release-note-none
📝 WalkthroughWalkthroughThis change implements merge-sort optimization for IN-condition queries to enable "keep order" without requiring a global sort. It adds grouped-range metadata to physical plans, propagates these ranges through executor layers with corresponding KV range information, and implements merge-sort across multiple sorted range groups to satisfy ORDER BY requirements efficiently. Changes
Sequence DiagramsequenceDiagram
participant Planner as Planner<br/>(find_best_task)
participant Plan as Physical Plan<br/>(physical_plans)
participant Executor as Executor<br/>(builder/distsql)
participant Reader as Range Reader<br/>(table_reader)
participant KV as KV Layer
Planner->>Planner: Detect IN-condition<br/>with constant prefix cols
Planner->>Planner: GroupRangesByCols<br/>(ranges by prefix)
Planner->>Plan: Set GroupedRanges<br/>& GroupByColIdxs
Plan-->>Executor: Pass grouped metadata
Executor->>Executor: Build grouped KV ranges<br/>per physical table
Executor->>Reader: Create per-group tasks<br/>with needExtraSorting flag
Reader->>Reader: Generate KV requests<br/>per grouped range
Reader->>KV: Send multiple sorted<br/>range requests
KV-->>Reader: Return sorted<br/>results per range
Reader->>Reader: Merge-sort results<br/>from range groups
Reader-->>Executor: Deliver globally<br/>ordered rows
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/executor/table_reader.go (1)
398-449:⚠️ Potential issue | 🔴 CriticalPreserve
e.kvRangesacross both split reads.
Open()can invoke this helper twice afterSplitRangesAcrossInt64Boundary(), but each branch currently replacese.kvRanges. SincekvRangesare later consumed by UnionScan, only the last half of an unsigned-handle scan survives and dirty rows from the first half can be missed.💡 Suggested fix
- e.kvRanges = sortAndGetKVRangesFromReqs(kvReqs) + e.kvRanges = append(e.kvRanges, sortAndGetKVRangesFromReqs(kvReqs)...)Apply the same append change at each assignment site in
buildRespForGroupedRanges().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/executor/table_reader.go` around lines 398 - 449, The code currently replaces e.kvRanges in multiple places inside buildRespForGroupedRanges(), losing earlier KV ranges when this helper is called twice; change each assignment (the three sites that call sortAndGetKVRangesFromReqs and assign to e.kvRanges) to append to the existing slice instead of overwriting it (e.g. e.kvRanges = append(e.kvRanges, sortAndGetKVRangesFromReqs(... )...)), ensuring you handle the nil/empty case safely so previous ranges are preserved for UnionScan; update the assignments in the TiFlash batch/non-batch branches and the final assignment accordingly.pkg/executor/distsql.go (1)
415-421:⚠️ Potential issue | 🟠 MajorClose already-open
SelectResults on mid-loop failure.If
e.SelectResult()fails after one or more earlier iterations succeeded, those earlier results are leaked becausee.resultis still unset whenOpen()returns. Close the accumulatedresultsslice before bubbling the error.Suggested fix
var results []distsql.SelectResult + cleanup := true + defer func() { + if !cleanup { + return + } + for _, r := range results { + terror.Log(r.Close()) + } + }() for _, kvReq := range kvReqs { result, err := e.SelectResult(ctx, e.dctx, kvReq, exec.RetTypes(e), getPhysicalPlanIDs(e.plans), e.ID()) if err != nil { return err } results = append(results, result) } e.result = distsql.NewSortedSelectResults(e.ectx.GetEvalCtx(), results, e.Schema(), e.byItems, e.memTracker) + cleanup = false }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/executor/distsql.go` around lines 415 - 421, In the loop that builds the results slice by calling e.SelectResult (inside the Open method), if a later e.SelectResult call returns an error, iterate the already-appended results and call Close() on each to avoid leaking open SelectResult objects before returning the error; ensure this cleanup runs before returning the error so e.result remains unset while no SelectResult stays open. Reference: e.SelectResult, results slice, e.result, Open().
🧹 Nitpick comments (1)
tests/integrationtest/r/planner/core/grouped_ranges_order_by.result (1)
137-176: Add a directORDER BY ... LIMITregression case.These cases lock in the new merge-sorted order behavior, but the primary user-facing regression here is the limited ordered-read path. A small
EXPLAIN ... ORDER BY ... LIMIT Nassertion would make future regressions in TopN pushdown / early-stop much easier to catch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integrationtest/r/planner/core/grouped_ranges_order_by.result` around lines 137 - 176, Add a compact regression case exercising ORDER BY ... LIMIT to lock in the merge-sorted early-stop behavior: add an EXPLAIN format='brief' and a matching execution for a query such as "select c from t2 where a in (1,3) and b in (1,3) and d < 150 order by c limit 3" (or a similar a/b/c combination used elsewhere in this file) and assert the plan shows the ordered index access (IndexRangeScan / IndexReader / IndexLookUp over index:iabc) with keep order/limit pushdown and that the result returns only the top N rows; place it near the existing queries that reference planner__core__grouped_ranges_order_by.t2.c and index:iabc so regressions in TopN pushdown / early-stop are detected.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/executor/distsql.go`:
- Around line 619-630: The loop over physicalIDs incorrectly reassigns the
shared variable ranges (used as the default input) causing overrides from one
partition to leak into subsequent iterations; fix by introducing a per-iteration
variable (e.g., curRanges) initialized to the original ranges, then if
rangeOverrideForPartitionID[physicalID] exists set curRanges = pRange and pass
curRanges into distsql.CommonHandleRangesToKVRanges and
distsql.IndexRangesToKVRangesWithInterruptSignal (instead of mutating the outer
ranges), ensuring each physicalID uses its own ranges copy.
In `@pkg/executor/table_reader.go`:
- Around line 487-494: sortAndGetKVRangesFromReqs currently mutates
kvReq.KeyRanges in place which can change request semantics before SelectResult
runs; instead, for each kvReq create a copy of its KeyRanges (e.g., allocate a
new slice and copy the elements or use a Clone method if available), call
SortByFunc on that copied KeyRanges, then call AppendSelfTo on the copied ranges
to build the returned slice—do not call SortByFunc directly on kvReq.KeyRanges
so SelectResult and e.kvRanges remain unaffected.
In `@pkg/planner/core/find_best_task.go`:
- Line 740: The current pruning uses compareBool on
lhs.matchPropResult.Matched() and rhs.matchPropResult.Matched(), which collapses
PropMatched and PropMatchedNeedMergeSort into the same bucket and can let a
need-merge-sort path wrongly dominate a direct-order path; replace that boolean
comparison with a rank-based comparison of the full
property.PhysicalPropMatchResult enum (e.g., implement
comparePropMatch(lhs.matchPropResult, rhs.matchPropResult) that maps
PropMatched→2, PropMatchedNeedMergeSort→1, others→0 and returns cmp.Compare of
scores) and use its result in place of matchResult while keeping the
compareGlobalIndex(lhs, rhs) usage unchanged so skyline pruning preserves
direct-order matches.
In `@pkg/planner/core/plan_cache_rebuild.go`:
- Around line 391-397: When rebuilding ranges for the PhysicalIndexJoin path,
ensure you refresh ts.GroupedRanges the same way you do for standalone scans:
after the inner scan's ranges are rebuilt (via x.Ranges.Rebuild()/updateRange()
or inside rebuildRange for the index-join branch), if len(ts.GroupByColIdxs) > 0
call GroupRangesByCols(ts.Ranges, ts.GroupByColIdxs) and assign the result to
ts.GroupedRanges, returning any error; apply the same change to the other
similar block noted (lines ~419-425) so grouped-range metadata is never left
stale for cached index-join merge-sort paths.
---
Outside diff comments:
In `@pkg/executor/distsql.go`:
- Around line 415-421: In the loop that builds the results slice by calling
e.SelectResult (inside the Open method), if a later e.SelectResult call returns
an error, iterate the already-appended results and call Close() on each to avoid
leaking open SelectResult objects before returning the error; ensure this
cleanup runs before returning the error so e.result remains unset while no
SelectResult stays open. Reference: e.SelectResult, results slice, e.result,
Open().
In `@pkg/executor/table_reader.go`:
- Around line 398-449: The code currently replaces e.kvRanges in multiple places
inside buildRespForGroupedRanges(), losing earlier KV ranges when this helper is
called twice; change each assignment (the three sites that call
sortAndGetKVRangesFromReqs and assign to e.kvRanges) to append to the existing
slice instead of overwriting it (e.g. e.kvRanges = append(e.kvRanges,
sortAndGetKVRangesFromReqs(... )...)), ensuring you handle the nil/empty case
safely so previous ranges are preserved for UnionScan; update the assignments in
the TiFlash batch/non-batch branches and the final assignment accordingly.
---
Nitpick comments:
In `@tests/integrationtest/r/planner/core/grouped_ranges_order_by.result`:
- Around line 137-176: Add a compact regression case exercising ORDER BY ...
LIMIT to lock in the merge-sorted early-stop behavior: add an EXPLAIN
format='brief' and a matching execution for a query such as "select c from t2
where a in (1,3) and b in (1,3) and d < 150 order by c limit 3" (or a similar
a/b/c combination used elsewhere in this file) and assert the plan shows the
ordered index access (IndexRangeScan / IndexReader / IndexLookUp over
index:iabc) with keep order/limit pushdown and that the result returns only the
top N rows; place it near the existing queries that reference
planner__core__grouped_ranges_order_by.t2.c and index:iabc so regressions in
TopN pushdown / early-stop are detected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e5521cfc-8c9d-40a2-8b72-53aeb4acc333
📒 Files selected for processing (13)
pkg/executor/builder.gopkg/executor/distsql.gopkg/executor/mem_reader.gopkg/executor/table_reader.gopkg/executor/union_scan.gopkg/planner/core/exhaust_physical_plans.gopkg/planner/core/find_best_task.gopkg/planner/core/physical_plans.gopkg/planner/core/plan_cache_rebuild.gopkg/planner/property/physical_property.gopkg/planner/util/path.gotests/integrationtest/r/planner/core/grouped_ranges_order_by.resulttests/integrationtest/t/planner/core/grouped_ranges_order_by.test
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## release-8.5 #66746 +/- ##
================================================
Coverage ? 55.7301%
================================================
Files ? 1819
Lines ? 658579
Branches ? 0
================================================
Hits ? 367027
Misses ? 265005
Partials ? 26547
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
time-and-fate
changed the title
keep order with IN conditions using merge sort (#62694)keep order with IN conditions using merge sort (#62694) | tidb-test=pr/2699
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/executor/distsql.go (1)
405-423:⚠️ Potential issue | 🟠 MajorClose previously opened
SelectResults on the merge-sort error path.If a later
buildKVReq()orSelectResult()call fails, everyresultalready appended toresultsis leaked becauseopen()returns before assigninge.resultor closing the partial slice.Suggested fix
} else { // Use sortedSelectResults for merge sort kvReqs := make([]*kv.Request, 0, len(kvRanges)) for _, kvRange := range kvRanges { kvReq, err := e.buildKVReq([]kv.KeyRange{kvRange}) if err != nil { return err } kvReqs = append(kvReqs, kvReq) } var results []distsql.SelectResult + defer func() { + if err == nil { + return + } + closeables := make([]Closeable, 0, len(results)) + for _, result := range results { + closeables = append(closeables, result) + } + terror.Log(closeAll(closeables...)) + }() for _, kvReq := range kvReqs { result, err := e.SelectResult(ctx, e.dctx, kvReq, exec.RetTypes(e), getPhysicalPlanIDs(e.plans), e.ID()) if err != nil { return err } results = append(results, result) } e.result = distsql.NewSortedSelectResults(e.ectx.GetEvalCtx(), results, e.Schema(), e.byItems, e.memTracker) }As per coding guidelines, "Keep error handling actionable and contextual; avoid silently swallowing errors in Go code".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/executor/distsql.go` around lines 405 - 423, If SelectResult or buildKVReq fails partway through creating the merge-sort results, previously appended distsql.SelectResult values in the results slice are leaked; update the code in the elt that builds kvReqs/results so that on any error you iterate the accumulated results and call Close() (or the appropriate cleanup method) on each distsql.SelectResult before returning the error, and only assign e.result via distsql.NewSortedSelectResults(e.ectx.GetEvalCtx(), results, e.Schema(), e.byItems, e.memTracker) after all SelectResult calls have succeeded (ensure buildKVReq, SelectResult, results slice, and e.result are referenced to locate the fix).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/executor/distsql.go`:
- Around line 319-345: buildKVRangesForIndexReader currently flattens
groupedRanges into a single []kv.KeyRange which loses the logical (group ×
partition) stream boundaries; change it to preserve and return grouped KV
streams (e.g. [][]kv.KeyRange) instead of flattening, so each entry corresponds
to one logical stream produced from groupedRanges/tableIDs; update callers
(notably open() and any needMergeSort logic) to compute needMergeSort from the
number of logical streams (stream count) rather than the raw KV range count, and
follow the pattern used by IndexLookUpExecutor.buildTableKeyRanges and
GroupRangesByCols to keep group boundaries intact when invoking buildKeyRanges.
---
Outside diff comments:
In `@pkg/executor/distsql.go`:
- Around line 405-423: If SelectResult or buildKVReq fails partway through
creating the merge-sort results, previously appended distsql.SelectResult values
in the results slice are leaked; update the code in the elt that builds
kvReqs/results so that on any error you iterate the accumulated results and call
Close() (or the appropriate cleanup method) on each distsql.SelectResult before
returning the error, and only assign e.result via
distsql.NewSortedSelectResults(e.ectx.GetEvalCtx(), results, e.Schema(),
e.byItems, e.memTracker) after all SelectResult calls have succeeded (ensure
buildKVReq, SelectResult, results slice, and e.result are referenced to locate
the fix).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 7e8acda6-55d6-4431-b6d4-beeb7de22a24
📒 Files selected for processing (1)
pkg/executor/distsql.go
Member
Author
|
/retest |
2 similar comments
Member
Author
|
/retest |
Member
Author
|
/retest |
qw4990
approved these changes
Mar 9, 2026
ti-chi-bot
Bot
added
approved
needs-1-more-lgtm
winoros
approved these changes
Mar 9, 2026
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: qw4990, winoros The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
ti-chi-bot
Bot
added
lgtm
and removed
needs-1-more-lgtm
ti-chi-bot
Bot
added
cherry-pick-approved
Member
Author
|
/retest |
3 similar comments
Member
Author
|
/retest |
Member
Author
|
/retest |
Member
Author
|
/retest |
Member
Author
|
/retest |
9 similar comments
Member
Author
|
/retest |
Member
Author
|
/retest |
Member
Author
|
/retest |
Member
Author
|
/retest |
Member
Author
|
/retest |
Member
Author
|
/retest |
Member
Author
|
/retest |
Member
Author
|
/retest |
Member
Author
|
/retest |
This was referenced Mar 12, 2026
This was referenced Mar 19, 2026
1 of 13 tasks
This was referenced Apr 10, 2026
6 of 12 tasks
This was referenced May 29, 2026
Merged
Closed
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
3 participants
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.
Manual cherry-pick of ##62694
What problem does this PR solve?
Issue Number: close #63449 close #34882
Problem Summary:
What changed and how does it work?
GroupedRangesandGroupByColIdxsare added toAccessPath,PhysicalIndexScan, andPhysicalTableScan. (pkg/planner/core/operator/physicalop/*)matchProperty()(pkg/planner/core/find_best_task.go)matchProperty(), the names and data types of several other fields and the function return value are also modified correspondingly, likecandidatePath.isMatchProp,matchPropForIndexMergeAlternatives(). The checking logic is also updated correspondingly. (pkg/planner/core/find_best_task.go)PropMatchedNeedMergeSort, set upByItems,GroupedRanges, andGroupByColIdxsforPhysicalIndexScanandPhysicalTableScan. (convertToIndexScan(),convertToTableScan())GroupRangesByCols()that builds/rebuilds theGroupedRangesfrom theGroupByColIdxsandranges. (pkg/planner/core/find_best_task.go)PhysicalPropMatchResult(pkg/planner/property/physical_property.go)PropMatchedNeedMergeSort: means the newly introduced enhancement.GroupedRanges. (pkg/planner/core/plan_cache_rebuild.go)Open()of the 3 executors, when there are correlated columns, also useGroupRangesByColsto rebuild grouped ranges after rebuilding ranges.IndexReaderExecutorgroupedRanges [][]*ranger.Range, directly comes fromPhysicalIndexScan.Open()to consider the new case when building kv ranges.IndexLookUpExecutorgroupedRanges [][]*ranger.Range, directly comes fromPhysicalIndexScan.groupedKVRanges []*kvRangesWithPhysicalTblIDas a unified place to store the kv ranges. It replaces the previouspartitionKVRangesandkvRangesfield.buildTableKeyRanges()to consider the new case when building kv ranges.TableReaderExecutorgroupedRangesandgroupByColIdxs, directly come fromPhysicalTableScan.Open()to consider the new case when building kv ranges.buildResp()is updated and renamed tobuildRespForGroupedRanges()as a unified method to handle ranges and grouped ranges.mem*ReaderusedIndexfor the extra sorting (mem reader doesn't support merge sort). (pkg/executor/mem_reader.go)usedIndexis set, it means we need extra sorting. (pkg/executor/mem_reader.go)memIndexLookUpReadergroupedKVRanges []*kvRangesWithPhysicalTblIDas a unified place to store the kv ranges. It replaces the previouspartitionKVRanges,partitionTables, andpartitionModefields. It corresponds to the changes inIndexLookUpExecutor.Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit
New Features
Performance Improvements