planner: fix `PointGetPlan.PrunePartitions` function works with non-binary collate #62002

Defined2014 · 2025-06-25T09:30:42Z

What problem does this PR solve?

Issue Number: close #61965, close #59827

Problem Summary:

What changed and how does it work?

Check List

Tests

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

tiprow · 2025-06-25T09:31:05Z

Hi @Defined2014. 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.

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.

Copilot

Pull Request Overview

This PR refactors partition pruning in PointGetPlan to correctly handle non-binary collations and exports the range detachment function for broader use.

Export and rename detachCondAndBuildRange to DetachCondAndBuildRange in pkg/util/ranger.
Update PointGetPlan.PrunePartitions to detect non-binary collations, rebuild a single-point range via the new API, and use its low values for partition pruning.
Refactor getNameValuePairs control flow from if/else to switch for improved clarity.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
pkg/util/ranger/detacher.go	Renamed and exported the `DetachCondAndBuildRange` function and updated callers.
pkg/planner/core/point_get_plan.go	Replaced the old `needsPartitionPruning` with non-binary-collation logic using `ranger.DetachCondAndBuildRange`; refactored `getNameValuePairs`.

Comments suppressed due to low confidence (2)

pkg/planner/core/point_get_plan.go:389

[nitpick] Consider renaming containsNonBinaryCollate to hasNonBinaryCollate to align with common boolean variable naming conventions and improve readability.

		containsNonBinaryCollate := false

pkg/planner/core/point_get_plan.go:397

Add unit or integration tests covering the non-binary collation branch in PrunePartitions, verifying that DetachCondAndBuildRange produces exactly one point range and the subsequent partition pruning behaves correctly.

		// If a non-binary collation is used, the values in `p.IndexValues` are sort keys and cannot be used for partition pruning.

pkg/planner/core/point_get_plan.go

codecov · 2025-06-25T09:53:40Z

Codecov Report

Attention: Patch coverage is 80.00000% with 4 lines in your changes missing coverage. Please review.

Project coverage is 73.5680%. Comparing base (a7465f4) to head (dd2c350).
Report is 16 commits behind head on master.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #62002        +/-   ##
================================================
+ Coverage   72.9452%   73.5680%   +0.6228%     
================================================
  Files          1735       1737         +2     
  Lines        482252     490630      +8378     
================================================
+ Hits         351780     360947      +9167     
+ Misses       108880     108009       -871     
- Partials      21592      21674        +82

Flag	Coverage Δ
integration	`42.5936% <80.0000%> (?)`
unit	`72.4942% <80.0000%> (+0.2929%)`	⬆️

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

Components	Coverage Δ
dumpling	`52.7804% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`46.3331% <ø> (-0.3390%)`	⬇️

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

Defined2014 · 2025-06-25T11:51:10Z

/retest

tiprow · 2025-06-25T11:51:32Z

@Defined2014: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

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.

pkg/planner/core/point_get_plan.go

Defined2014 · 2025-06-25T12:32:53Z

/retest

tiprow · 2025-06-25T12:33:15Z

@Defined2014: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

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.

AilinKid

rest LGTM

pkg/planner/core/point_get_plan.go

mjonss · 2025-06-26T21:10:39Z

pkg/planner/core/point_get_plan.go

-					idx := -1
-					p.PartitionIdx = &idx
-					return true, nil
+				if len(r.Ranges) != 1 || !r.Ranges[0].IsPoint(sctx.GetRangerCtx()) {


Please add a test where len(r.Ranges) == 0. Like RANGE partition without MAXVALUE and a condition for value greater than last partition.
I would assume it should return true, nil in such case and use table DUAL.

This function isn't related to partition tables — it only processes the AccessConditions and returns the ranger. So even it's not meet any exists partition, the len(r.Ranges) also should equals to 1.

The pruning stage is handled separately here.

The root cause of these problems is that the planner uses a different function from this one, and the results of the two are not always consistent. To ensure correctness, we should use the same function here.

mjonss · 2025-06-26T21:15:10Z

pkg/planner/core/point_get_plan.go

+		for _, col := range p.IdxCols {
+			if !collate.IsBinCollation(col.GetType(evalCtx).GetCollate()) {
+				// If a non-binary collation is used, the values in `p.IndexValues` are sort keys and cannot be used for partition pruning.
+				r, err := ranger.DetachCondAndBuildRangeForPartition(sctx.GetRangerCtx(), p.AccessConditions, p.IdxCols, p.IdxColLens, sctx.GetSessionVars().RangeMaxSize)


Why are we looping on p.IdxCols and doing this for the full conditions, and all columns (p.IdxCols)?
Should we also check if the column is part of the partitioning expression and then check if binary collation?

I added a comments here, I think it's not easy to do this right now. And this part will not affect the performance very much.

However, we currently don't have a safe and reliable way to do that. col is of type expression.Column, while the IDs from pt.GetPartitionColumnIDs() are derived from t.Cols() which type is table.Column. It's not always correctly to compare them.

mjonss · 2025-06-26T21:22:21Z

tests/integrationtest/r/planner/core/casetest/partition/partition_pruner.result

+`col_95` char(181) COLLATE gbk_bin NOT NULL DEFAULT 'SaMKHTyg+nlID-X3Y',
+PRIMARY KEY (`col_95`) /*T![clustered_index] CLUSTERED */
+) ENGINE=InnoDB DEFAULT CHARSET=gbk COLLATE=gbk_bin
+PARTITION BY RANGE COLUMNS(`col_95`)


Do we need to test other partitioning schemes as well? Like RANGE COLUMNS, LIST [COLUMNS] and KEY?

I don't think this is related to partition types. And also I think the test cases have already been added in PR #59918.

Defined2014 · 2025-06-27T01:44:26Z

/retest

tiprow · 2025-06-27T01:44:47Z

@Defined2014: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

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.

Defined2014 · 2025-06-27T03:50:09Z

/retest

tiprow · 2025-06-27T03:50:32Z

@Defined2014: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

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 · 2025-06-27T09:47:12Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: AilinKid, hawkingrei

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [AilinKid,hawkingrei]
~~pkg/planner/OWNERS~~ [AilinKid,hawkingrei]

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 · 2025-06-27T09:47:16Z

[LGTM Timeline notifier]

Timeline:

2025-06-26 07:48:39.954342007 +0000 UTC m=+949172.677520984: ☑️ agreed by hawkingrei.
2025-06-27 09:47:15.123250116 +0000 UTC m=+1042687.846429095: ☑️ agreed by AilinKid.

Defined2014 · 2025-06-28T07:09:05Z

/unhold

Defined2014 · 2025-06-30T06:09:59Z

/cherry-pick-release-8.5

Defined2014 · 2025-06-30T06:11:01Z

/cherry-pick release-8.5

Signed-off-by: ti-chi-bot <[email protected]>

ti-chi-bot · 2025-06-30T06:11:47Z

@Defined2014: new pull request created to branch release-8.5: #62082.
But this PR has conflicts, please resolve them!

In response to this:

/cherry-pick release-8.5

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 ti-community-infra/tichi repository.

…inary collate (#62002) (#62082) close #59827, close #61965

planner: fix PointGetPlan.PrunePartitions works with non-binary collate

9900472

ti-chi-bot bot added the release-note-none Denotes a PR that doesn't merit a release note. label Jun 25, 2025

Defined2014 requested a review from Copilot June 25, 2025 09:30

ti-chi-bot bot added sig/planner SIG: Planner size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 25, 2025

Copilot AI reviewed Jun 25, 2025

View reviewed changes

pkg/planner/core/point_get_plan.go Outdated Show resolved Hide resolved

update

9049af5

add test cases

8cad6da

Defined2014 added the needs-cherry-pick-release-8.5 Should cherry pick this PR to release-8.5 branch. label Jun 25, 2025

hawkingrei reviewed Jun 25, 2025

View reviewed changes

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

hawkingrei reviewed Jun 25, 2025

View reviewed changes

pkg/planner/core/point_get_plan.go Outdated Show resolved Hide resolved

follow comments

ba386f6

Defined2014 requested review from hawkingrei, mjonss and qw4990 June 26, 2025 02:24

hawkingrei approved these changes Jun 26, 2025

View reviewed changes

ti-chi-bot bot added approved needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Jun 26, 2025

AilinKid reviewed Jun 26, 2025

View reviewed changes

pkg/planner/core/point_get_plan.go Outdated Show resolved Hide resolved

pkg/planner/core/point_get_plan.go Outdated Show resolved Hide resolved

pkg/planner/core/point_get_plan.go Outdated Show resolved Hide resolved

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

mjonss reviewed Jun 26, 2025

View reviewed changes

pkg/planner/core/point_get_plan.go Outdated Show resolved Hide resolved

Defined2014 added 3 commits June 26, 2025 16:34

follow comments

285a003

improve code

8c95709

update

fa51b55

mjonss reviewed Jun 26, 2025

View reviewed changes

pkg/planner/core/point_get_plan.go Outdated Show resolved Hide resolved

Defined2014 added 2 commits June 26, 2025 21:49

follow comments

9feab38

follow comments

8ef216a

mjonss reviewed Jun 26, 2025

View reviewed changes

update

d94c1f7

Defined2014 added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 27, 2025

update comments

190f60b

AilinKid approved these changes Jun 27, 2025

View reviewed changes

ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Jun 27, 2025

update

dd2c350

ti-chi-bot bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 28, 2025

ti-chi-bot bot merged commit ce538d6 into pingcap:master Jun 28, 2025
22 checks passed

ti-chi-bot pushed a commit to ti-chi-bot/tidb that referenced this pull request Jun 30, 2025

This is an automated cherry-pick of pingcap#62002

ce72017

Signed-off-by: ti-chi-bot <[email protected]>

ti-chi-bot mentioned this pull request Jun 30, 2025

planner: fix PointGetPlan.PrunePartitions function works with non-binary collate (#62002) #62082

Merged

13 tasks

ti-chi-bot bot pushed a commit that referenced this pull request Jul 7, 2025

planner: fix PointGetPlan.PrunePartitions function works with non-b…

325f410

…inary collate (#62002) (#62082) close #59827, close #61965

planner: fix PointGetPlan.PrunePartitions function works with non-binary collate #62002

planner: fix PointGetPlan.PrunePartitions function works with non-binary collate #62002

Uh oh!

Conversation

Defined2014 commented Jun 25, 2025 • 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!

tiprow bot commented Jun 25, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

codecov bot commented Jun 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Defined2014 commented Jun 25, 2025

Uh oh!

tiprow bot commented Jun 25, 2025

Uh oh!

Uh oh!

Uh oh!

Defined2014 commented Jun 25, 2025

Uh oh!

tiprow bot commented Jun 25, 2025

Uh oh!

AilinKid left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mjonss Jun 26, 2025 • 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!

Defined2014 Jun 27, 2025 • 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!

Defined2014 Jun 27, 2025

Choose a reason for hiding this comment

Uh oh!

mjonss Jun 26, 2025

Choose a reason for hiding this comment

Uh oh!

Defined2014 Jun 27, 2025 • 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!

mjonss Jun 26, 2025

Choose a reason for hiding this comment

Uh oh!

Defined2014 Jun 27, 2025 • 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!

Defined2014 commented Jun 27, 2025

Uh oh!

tiprow bot commented Jun 27, 2025

Uh oh!

Defined2014 commented Jun 27, 2025

Uh oh!

tiprow bot commented Jun 27, 2025

Uh oh!

ti-chi-bot bot commented Jun 27, 2025

Uh oh!

ti-chi-bot bot commented Jun 27, 2025

[LGTM Timeline notifier]

Uh oh!

planner: fix `PointGetPlan.PrunePartitions` function works with non-binary collate #62002

planner: fix `PointGetPlan.PrunePartitions` function works with non-binary collate #62002

Defined2014 commented Jun 25, 2025 •

edited

Loading

codecov bot commented Jun 25, 2025 •

edited

Loading

mjonss Jun 26, 2025 •

edited

Loading

Defined2014 Jun 27, 2025 •

edited

Loading

Defined2014 Jun 27, 2025 •

edited

Loading

Defined2014 Jun 27, 2025 •

edited

Loading