Repository navigation
planner: recognize order on common handle PK (#66645) - #70460
takaidohigasi wants to merge 1 commit into
Conversation
close pingcap#66644 (cherry picked from commit 3032ee1)
ti-chi-bot
Bot
added
release-note-none
|
Hi @takaidohigasi. Thanks for your PR. I'm waiting for a pingcap member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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. |
📝 WalkthroughWalkthroughThe planner now recognizes ordering through eligible CommonHandle indexes by extending index columns with missing handle columns. It excludes unsafe v0 new-collation string handles. New Cascades tests cover secondary and clustered index ordering cases. ChangesCommonHandle ordering
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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.12.2)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 |
|
Local verification results on this branch (macOS arm64, worktree of latest release-8.5): Unit test — passed ✅ $ make failpoint-enable
$ go test --tags=intest ./pkg/planner/core/casetest/rule/ -run TestCommonHandleIndexOrdering -v
=== RUN TestCommonHandleIndexOrdering
=== RUN TestCommonHandleIndexOrdering/off
--- PASS: TestCommonHandleIndexOrdering (1.21s)
--- PASS: TestCommonHandleIndexOrdering/off (1.21s)
PASS
ok github.com/pingcap/tidb/pkg/planner/core/casetest/rule 4.707s
$ make failpoint-disableAll 10 cases in Bazel metadata — clean
A full local |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/planner/core/casetest/rule/rule_common_handle_ordering_test.go (1)
38-301: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a regression case for
hasV0NewCollationStringHandle.The current cases use DDL-created tables with
CommonHandleVersion == 1. Add a focused case withCommonHandleVersion == 0, new collation enabled, a non-binary string primary key, anINpredicate, andORDER BYon the handle. Assert correct row ordering and that the plan does not usePropMatchedNeedMergeSort.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/planner/core/casetest/rule/rule_common_handle_ordering_test.go` around lines 38 - 301, Add a focused regression scenario within TestCommonHandleIndexOrdering using CommonHandleVersion == 0 with new collation enabled, a non-binary string clustered primary key, an IN predicate, and ORDER BY on the primary-key handle. Assert the returned rows are correctly ordered and verify the EXPLAIN plan does not contain PropMatchedNeedMergeSort, reusing the test’s existing setup and assertion helpers.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/planner/core/casetest/rule/rule_common_handle_ordering_test.go`:
- Around line 70-73: Update every positive ordering assertion in the test cases,
including the blocks around explainHas checks for “keep order:true” at the
referenced cases, to also assert that explainHas(rows, "Sort") is false.
Preserve the existing TopN rejection and apply the same Sort check consistently
to all positive ordering cases.
---
Nitpick comments:
In `@pkg/planner/core/casetest/rule/rule_common_handle_ordering_test.go`:
- Around line 38-301: Add a focused regression scenario within
TestCommonHandleIndexOrdering using CommonHandleVersion == 0 with new collation
enabled, a non-binary string clustered primary key, an IN predicate, and ORDER
BY on the primary-key handle. Assert the returned rows are correctly ordered and
verify the EXPLAIN plan does not contain PropMatchedNeedMergeSort, reusing the
test’s existing setup and assertion helpers.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: fbfba9f7-d140-4b6a-b3af-0008cf64de8c
📒 Files selected for processing (3)
pkg/planner/core/casetest/rule/BUILD.bazelpkg/planner/core/casetest/rule/rule_common_handle_ordering_test.gopkg/planner/core/find_best_task.go
| require.True(t, explainHas(rows, "keep order:true"), | ||
| "case 1: expected keep order:true for basic CommonHandle ordering") | ||
| require.False(t, explainHas(rows, "TopN"), | ||
| "case 1: unexpected TopN sort") |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Check for Sort in the positive plan assertions.
The objective requires removal of both TopN and Sort. These assertions only reject TopN, so a regression to Sort passes the test.
Proposed test change
- require.False(t, explainHas(rows, "TopN"),
+ require.False(t, explainHas(rows, "TopN") || explainHas(rows, "Sort"),Apply the same check to every positive ordering case.
Also applies to: 91-94, 111-114, 144-147, 225-228, 257-260, 289-292
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/planner/core/casetest/rule/rule_common_handle_ordering_test.go` around
lines 70 - 73, Update every positive ordering assertion in the test cases,
including the blocks around explainHas checks for “keep order:true” at the
referenced cases, to also assert that explainHas(rows, "Sort") is false.
Preserve the existing TopN rejection and apply the same Sort check consistently
to all positive ordering cases.
|
Follow-up: the full local bazel build also passed ✅ $ bazel build --config=ci //... --//build:with_nogo_flag=true --//build:with_rbe_flag=true
INFO: Found 1257 targets...
INFO: Elapsed time: 3932.397s, Critical Path: 611.59s
INFO: 26684 processes: 1539 internal, 25145 darwin-sandbox.
INFO: Build completed successfully, 26684 total actionsAll local verification is now complete: bazel build (with nogo), |
|
I am glad if we can get ok-to-test on it |
|
@terry1purcell would you please check for this? |
|
@qw4990 sorry for the mention. this is originally merged to master branch, but change for 8.5 is stopped because bazel config conflict did not resolved. would you give us ok-to-test label on this PR if you are OK? |
|
/ok-to-test |
ti-chi-bot
Bot
added
ok-to-test
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## release-8.5 #70460 +/- ##
================================================
Coverage ? 55.1154%
================================================
Files ? 1852
Lines ? 667027
Branches ? 0
================================================
Hits ? 367635
Misses ? 271996
Partials ? 27396
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
test setup failed. tests in //tests/realtikvtest/statisticstest:statisticstest_test, this is not related to this PR change. |
|
/test check-dev2 |
|
all the CI passed. ready! |
|
please take a look when you have time🙏 |
There was a problem hiding this comment.
Pull request overview
Manually backports common-handle PK ordering recognition to release-8.5.
Changes:
- Extends secondary-index order matching with common-handle PK columns.
- Adds ten ordering regression scenarios.
- Registers and shards the new Bazel test.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
pkg/planner/core/find_best_task.go |
Recognizes common-handle PK suffix ordering. |
pkg/planner/core/casetest/rule/rule_common_handle_ordering_test.go |
Tests planner and result ordering. |
pkg/planner/core/casetest/rule/BUILD.bazel |
Registers the new test. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| alreadyInIndex := false | ||
| for _, col := range idxCols { | ||
| if col != nil && col.EqualColumn(handleCol) { | ||
| alreadyInIndex = true | ||
| break | ||
| } | ||
| } |
There was a problem hiding this comment.
Thanks for the observation — the scenario is real, but it is a missed optimization rather than a correctness issue, and it is intentional (pre-existing) behavior faithfully mirrored from master. Keeping this cherry-pick identical to master, so no change here; a follow-up on master would be the right place.
Details:
-
No correctness impact. The matcher's case-1 check requires
idxColLens[colIdx] == types.UnspecifiedLength, so a prefix column can never be treated as providing order. WithKEY idx(d, a1(2))+PRIMARY KEY (a1, a2), the dedup drops the fulla1, matching stops ata1(2), and we returnPropNotMatched— i.e. we fall back to an explicit sort, which is exactly the behavior before this change. This PR never loses an ordering we previously recognized; it only declines to recognize a new one in the prefix-overlap corner. -
The suggested dedup guard alone would not enable the optimization. Even if we kept the full
a1(effective columns(d, a1(2), a1, a2)), the matcher would still stop ata1(2)because there is no rule to skip a prefix column when the same column follows in full form. Supporting this corner needs both the length-aware dedup and a prefix-skip rule in the matcher, plus an argument that prefix ordering is monotonic under the column's collation (sort-key prefixes and collation contractions make this non-trivial — the same class of concern that motivated excluding v0 new-collation string handles here). -
This block is byte-for-byte identical to master (see
matchPropertyinpkg/planner/core/find_best_task.goon master), so a behavior change belongs there first and can then be backported if desired. I'd suggest tracking the prefix-overlap enhancement (and a regression case pinning today's sort-fallback behavior) as a follow-up issue against master rather than diverging in this backport.
There was a problem hiding this comment.
I agree @takaidohigasi. The AI reviews provide additional opportunities for improvement - but they should not delay the cherry pick.
There was a problem hiding this comment.
thanks, please tell me if there is anything I can help you or anything I need to cope with
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: terry1purcell 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 |
[LGTM Timeline notifier]Timeline:
|
ti-chi-bot
Bot
added
cherry-pick-approved
This is a manual cherry-pick of #66645 to release-8.5, replacing the automated cherry-pick #67107 which contains unresolved merge conflict markers in
pkg/planner/core/casetest/rule/BUILD.bazel(causing itsbuild/unit-test/check-dev2CI jobs to fail).What problem does this PR solve?
Issue Number: close #66644
Problem Summary:
Common handle PKs were not recognized as providing order after the index columns, while integer PKs were. This caused unnecessary TopN/Sort operators for
ORDER BY <pk columns>queries on clustered (common handle) tables scanned via a secondary index.What changed and how does it work?
Manual cherry-pick of #66645 (commit 3032ee1) onto release-8.5.
pkg/planner/core/find_best_task.goapplies cleanly (no conflicts).pkg/planner/core/casetest/rule/BUILD.bazel; resolved by addingrule_common_handle_ordering_test.gotosrcsand bumpingshard_countfrom 21 to 22 (master is at 23 because it has an extra test file that does not exist on release-8.5).As in the original PR:
matchPropertynow builds an effective index column list that includes the common handle PK suffix for non-unique secondary indexes, so ORDER BY on PK columns can be satisfied withkeep order:true. The v0 CommonHandleVersion + new collation string handle case is excluded for correctness.Check List
Tests
Local verification on this branch:
make bazel_prepare(no diff beyond the resolved BUILD.bazel)go test --tags=intest ./pkg/planner/core/casetest/rule/ -run TestCommonHandleIndexOrderingpasses (all 10 cases, with failpoints enabled)Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit
Bug Fixes
Tests