planner: add the prefix index as candidate for topn optimization #65533

elsa0520 · 2026-01-12T09:49:26Z

What problem does this PR solve?

Issue Number: Fix #63280

Problem Summary:

What changed and how does it work?

The planner part 1 of the partial order by enhancement

The new physical property
- PartialOrderInfo: save the order by columns info
The exhaust physical plan of TopN will add a new path with this new physical property
- the scope will be limited, and it will only affect the available query patterns when the OptPartialOrderedIndexForTopN is turned on.
The new match property function "matchPartialOrderProperty" is used to check if prefix index can be used for TopN query
- The restrictions on match properties with new physical properties will be relaxed.
- Allows prefix index to match ORDER BY columns.
- The index can completely match the prefix of the order by column.

Check List

Tests

I will add more test after finish the part2 of this enhancement

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No need to test
- I checked and no code files have been changed.

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Documentation

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

None

ti-chi-bot · 2026-01-12T09:49:28Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

tiprow · 2026-01-12T09:49:48Z

Hi @elsa0520. Thanks for your PR.

PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test all.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

hawkingrei · 2026-01-13T09:02:37Z

/ok-to-test

pkg/planner/property/physical_property.go

pkg/planner/core/find_best_task.go

winoros · 2026-01-13T15:50:43Z

pkg/planner/property/physical_property.go

+type PartialOrderInfo struct {
+	// SortItems are the ORDER BY columns from TopN
+	SortItems []*SortItem
+}


type PartialOrderInfo []*SortItem

In the future, it maybe need to add a new field named "limit number" inside of struct PartialOrderInfo.
Because we need to know which is order by column , also we need to know What number of the limit N we can short cut.
So I keep the struct instead of use []*sortItem directly

pkg/planner/property/physical_property.go

pkg/planner/core/find_best_task.go

winoros · 2026-01-13T15:54:44Z

pkg/planner/core/operator/physicalop/physical_topn.go

+			}
+		}
+	}
+


Also, the case where there's a projection?

And, I think that we can not support the partial order with txn's dirty writes in the first implementation?

Yep, projection is possible. Is it expected that we bail out for Projection?

The dirty write already fixed

Is it expected that we bail out for Projection?

There can indeed be projection between top and ds. However, the presence of projection doesn't necessarily guarantee the use of a prefix index.

For example, consider the query select fn(a) as a1 from t order by fn(a) limit 10.

In this case, whether an order-preserving index can be used depends entirely on the scalar function of fn(a). Our current code doesn't actually have this capability.

In other words, this query can't even use the index a, let alone the prefix (a).

what if select a+b, a, b from t order by a,b limit 10?

I already tried this case. The topn is under the projection because we have a rule named "PushDownTopN(topN)"

pkg/planner/core/operator/physicalop/physical_topn.go

qw4990 · 2026-01-15T06:50:43Z

pkg/planner/core/find_best_task.go

-				continue
-			}
+			// Check if candidate path need to consider partial order optimization
+			considerPartialOrderIndex := ds.SCtx().GetSessionVars().OptPartialOrderedIndexForTopN && prop.PartialOrderInfo != nil


Should we make the judge here more precise? For example, just call matchPartialOrderProperty to check whether the index can match the partial order property, matchPartialOrderIndex := ds.SCtx().GetSessionVars().OptPartialOrderedIndexForTopN && matchPartialOrderProperty(path, prop) != nil. Then we can avoid putting some unnecessary indexes into our index candidates.

Otherwise, if we enable OptPartialOrderedIndexForTopN and there is a PartialOrderInfo, then all indexes are going to be put into our candidates, the if-branch below seems to stop working.

And the partial order skyline pruning at the begin of here ~

qw4990

Others LGTM, please address these comments before merging it.

ti-chi-bot · 2026-01-15T06:55:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: qw4990

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~pkg/planner/OWNERS~~ [qw4990]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ti-chi-bot · 2026-01-15T06:55:14Z

[LGTM Timeline notifier]

Timeline:

2026-01-15 06:55:13.973429008 +0000 UTC m=+44941.587385864: ☑️ agreed by qw4990.

fixdb · 2026-01-15T07:59:47Z

pkg/planner/core/operator/physicalop/physical_topn.go

+			}
+		}
+	}
+


Yep, projection is possible. Is it expected that we bail out for Projection?

fixdb · 2026-01-15T08:02:19Z

pkg/planner/core/find_best_task.go

+type PartialOrderMatchResult struct {
+	// Matched indicates whether the index can provide partial order
+	Matched bool
+	// PrefixColId is the last and only one prefix column ID of index


Can you give an example of this field? The comment is hard to understand.

fixdb · 2026-01-15T08:19:29Z

pkg/planner/core/find_best_task.go

+			if !(len(path.AccessConds) > 0 || !prop.IsSortItemEmpty() || path.Forced || path.IsSingleScan || !considerPartialOrderIndex) {
+				continue
+			}
+


This is counter intuitive, hard to follow.
Should be this:

if len(path.AccessConds) > 0 || !prop.IsSortItemEmpty() || path.Forced || path.IsSingleScan || !considerPartialOrderIndex { currentCandidate = getIndexCandidate(ds, path, prop) } else { continue }

This is counter intuitive, hard to follow. Should be this:

if len(path.AccessConds) > 0 || !prop.IsSortItemEmpty() || path.Forced || path.IsSingleScan || !considerPartialOrderIndex { currentCandidate = getIndexCandidate(ds, path, prop) } else { continue }

Agree with you change... but the origin logic is like that .... I can change it

fixdb · 2026-01-15T09:01:11Z

pkg/planner/core/find_best_task.go

 			// We will use index to generate physical plan if any of the following conditions is satisfied:
 			// 1. This path's access cond is not nil.
 			// 2. We have a non-empty prop to match.
 			// 3. This index is forced to choose.
 			// 4. The needed columns are all covered by index columns(and handleCol).
+			// 5. Has PartialOrderInfo physical property to be considered for partial order optimization (new condition).
+			if !(len(path.AccessConds) > 0 || !prop.IsSortItemEmpty() || path.Forced || path.IsSingleScan || !considerPartialOrderIndex) {


Wait, is this wrong? Should !considerPartialOrderIndex be considerPartialOrderIndex?

This logic is so hard to understand, let me rephrase it in a way that's human-readable.

fixdb · 2026-01-15T09:03:25Z

pkg/planner/property/physical_property.go

+	// partialOrderInfo: sortItems: [a, b]
+	// The partialOrderInfo property will pass through to the datasource and try to matchPartialOrderProperty such as:
+	// index: (a, b(10) )
+	PartialOrderInfo *PartialOrderInfo


Should we extend PhysicalProperty.HashCode() to incorporate PartialOrderInfo (columns + order direction) so task caching is correct?

codecov · 2026-01-15T10:14:22Z

Codecov Report

❌ Patch coverage is 67.91444% with 60 lines in your changes missing coverage. Please review.
✅ Project coverage is 76.2034%. Comparing base (6b3f20a) to head (87ba3d2).
⚠️ Report is 59 commits behind head on master.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #65533        +/-   ##
================================================
- Coverage   78.4730%   76.2034%   -2.2697%     
================================================
  Files          1938       1922        -16     
  Lines        534229     546361     +12132     
================================================
- Hits         419226     416346      -2880     
- Misses       113483     129970     +16487     
+ Partials       1520         45      -1475

Flag	Coverage Δ
integration	`41.5933% <23.5294%> (-6.5789%)`	⬇️
unit	`76.1346% <67.9144%> (-0.3490%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
dumpling	`56.7974% <ø> (+0.1783%)`	⬆️
parser	`∅ <ø> (∅)`
br	`42.4684% <ø> (-22.4202%)`	⬇️

🚀 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.

elsa0520 · 2026-01-15T13:26:54Z

/test mysql-test

tiprow · 2026-01-15T13:27:21Z

@elsa0520: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test fast_test_tiprow

/test tidb_parser_test

Use /test all to run all jobs.

Details

In response to this:

/test mysql-test

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

ti-chi-bot · 2026-01-15T13:27:31Z

[FORMAT CHECKER NOTIFICATION]

Notice: To remove the do-not-merge/needs-tests-checked label, please finished the tests then check the finished items in description.

For example:

Tests

Unit test

Integration test

Manual test (add detailed scripts or steps below)

No code

_{📖 For more info, you can check the "Contribute Code" section in the development guide.}

ti-chi-bot · 2026-01-15T13:41:08Z

@elsa0520: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-integration-realcluster-test-next-gen	`87ba3d2`	link	true	`/test pull-integration-realcluster-test-next-gen`
pull-integration-ddl-test	`87ba3d2`	link	true	`/test pull-integration-ddl-test`
idc-jenkins-ci-tidb/unit-test	`87ba3d2`	link	true	`/test unit-test`

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

planner: add the partial order index as candidate for topn optimization

5ae7f99

change the match partial order property part

7fd3e22

elsa0520 marked this pull request as ready for review January 13, 2026 08:54

ti-chi-bot bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 13, 2026

ti-chi-bot bot added ok-to-test Indicates a PR is ready to be tested. do-not-merge/needs-tests-checked and removed do-not-merge/needs-tests-checked do-not-merge/needs-linked-issue labels Jan 13, 2026

qw4990 reviewed Jan 13, 2026

View reviewed changes

pkg/planner/property/physical_property.go Show resolved Hide resolved

qw4990 reviewed Jan 13, 2026

View reviewed changes

pkg/planner/core/find_best_task.go Show resolved Hide resolved

winoros reviewed Jan 13, 2026

View reviewed changes

elsa0520 added 2 commits January 14, 2026 16:13

add comment and delete unused code

401ecf4

add constraint about the dirty write

1c3d917

elsa0520 changed the title ~~planner: add the partial order index as candidate for topn optimization~~ planner: add the prefix index as candidate for topn optimization Jan 14, 2026

qw4990 reviewed Jan 15, 2026

View reviewed changes

pkg/planner/core/operator/physicalop/physical_topn.go Show resolved Hide resolved

qw4990 reviewed Jan 15, 2026

View reviewed changes

qw4990 approved these changes Jan 15, 2026

View reviewed changes

ti-chi-bot bot added approved needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Jan 15, 2026

fixdb reviewed Jan 15, 2026

View reviewed changes

elsa0520 added 2 commits January 15, 2026 16:32

fix build

b5e6669

fix

1e30a85

fixdb reviewed Jan 15, 2026

View reviewed changes

elsa0520 added 2 commits January 15, 2026 17:30

fix format

43a0e70

fix lint and unreadable logic

43baddc

elsa0520 added 2 commits January 15, 2026 19:48

fix lint

0800fd7

hash code and support projection

87ba3d2

ti-chi-bot bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 15, 2026

planner: add the prefix index as candidate for topn optimization #65533

Are you sure you want to change the base?

planner: add the prefix index as candidate for topn optimization #65533

Conversation

elsa0520 commented Jan 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

Uh oh!

ti-chi-bot bot commented Jan 12, 2026

Uh oh!

tiprow bot commented Jan 12, 2026

Uh oh!

hawkingrei commented Jan 13, 2026

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

qw4990 Jan 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qw4990 left a comment

Choose a reason for hiding this comment

Uh oh!

ti-chi-bot bot commented Jan 15, 2026

Uh oh!

ti-chi-bot bot commented Jan 15, 2026

[LGTM Timeline notifier]

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Jan 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

elsa0520 commented Jan 12, 2026 •

edited

Loading

qw4990 Jan 15, 2026 •

edited

Loading

codecov bot commented Jan 15, 2026 •

edited

Loading

ti-chi-bot bot commented Jan 15, 2026 •

edited

Loading