Planner: Run full partition pruning if convertToPointGet converted keys to SortKey #59918

mjonss · 2025-03-05T08:54:35Z

What problem does this PR solve?

Issue Number: close #59827

Problem Summary:
When PointGetPlan is created by convertToPointGet, the IndexValues comes from the Range optimizer, and if any of the LIST/RANGE COLUMNS has a non-binary collation, they conditions values are converted to SortKey, which does not work with the LIST/RANGE COLUMNS partitioning pruning code, since they will unconditionally convert it again to SortKey, which may fail finding a matching partition.

NOTE: This only affects PointGet, since BatchPointGet is not supported yet.

What changed and how does it work?

If the IndexValues are already converted, run the normal partition pruning code on the conditions, to find the matching partition.

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.

Partition Pruning could fail and choose 'TableDual', i.e. no matching partition, in case the optimizer could convert the query to PointGet (if not using the FastPlan) and any LIST COLUMNS or RANGE COLUMNS had non-binary collation.

…tKey

ti-chi-bot · 2025-03-05T08:54:37Z

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 · 2025-03-05T08:54:55Z

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

mjonss · 2025-03-05T08:55:27Z

/test all

tiprow · 2025-03-05T08:55:49Z

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

In response to this:

/test all

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.

…y collation

mjonss · 2025-03-06T13:09:19Z

/retest

tiprow · 2025-03-06T13:09:43Z

@mjonss: 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.

codecov · 2025-03-06T13:28:54Z

Codecov Report

Attention: Patch coverage is 63.30275% with 40 lines in your changes missing coverage. Please review.

Project coverage is 73.2799%. Comparing base (d392685) to head (7f64780).
Report is 97 commits behind head on master.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #59918        +/-   ##
================================================
+ Coverage   72.9654%   73.2799%   +0.3145%     
================================================
  Files          1699       1703         +4     
  Lines        469477     476770      +7293     
================================================
+ Hits         342556     349377      +6821     
- Misses       105828     106093       +265     
- Partials      21093      21300       +207

Flag	Coverage Δ
integration	`42.7406% <30.2752%> (?)`
unit	`72.6821% <62.3853%> (+0.2289%)`	⬆️

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

Components	Coverage Δ
dumpling	`∅ <ø> (∅)`
parser	`∅ <ø> (∅)`
br	`46.1044% <ø> (-0.3110%)`	⬇️

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

mjonss · 2025-03-06T17:43:49Z

KEY partitioning does not seem to be affected, since it is using val.ToHashKey() which is using the value's collation, meaning that if it already is converted to binary SortKey, it will get the same hash/key partition as if it is a non-converted values with the original collation, which will be converted in val.ToHashKey()

pkg/planner/core/point_get_plan.go

qw4990 · 2025-03-11T12:55:01Z

pkg/planner/core/point_get_plan.go

+	// TODO: Is there a simpler way, or existing function for this?!?
+	tblCols := make([]*expression.Column, 0, len(indexInfo.Columns))
+	var partNameSlice types.NameSlice
+	for _, tblCol := range tblInfo.Columns {
+		found := false
+		for _, idxCol := range indexCols {
+			if idxCol.ID == tblCol.ID {
+				tblCols = append(tblCols, idxCol)
+				found = true
+				break
+			}
+		}
+		partNameSlice = append(partNameSlice, &types.FieldName{
+			ColName:     tblCol.Name,
+			TblName:     tblInfo.Name,
+			DBName:      ast.NewCIStr(dbName),
+			OrigTblName: tblInfo.Name,
+			OrigColName: tblCol.Name,
+		})
+		if !found {
+			tblCols = append(tblCols, &expression.Column{
+				ID:       tblCol.ID,
+				OrigName: tblCol.Name.O,
+				RetType:  tblCol.FieldType.Clone(),
+			})
+		}
+	}


Is this operations common for partitioning tables? If Yes, could we wrap this operation as a separate function in partition_prune.go?

I tried using ColumnInfos2ColumnsAndNames() but there were test failures. So I'm keeping this code for now.

This reverts commit dba86e5.

ti-chi-bot · 2025-03-12T00:41:22Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Defined2014, qw4990

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~~ [Defined2014,qw4990]
~~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 · 2025-03-12T00:41:24Z

[LGTM Timeline notifier]

Timeline:

2025-03-10 09:52:41.19161533 +0000 UTC m=+176716.797144257: ☑️ agreed by Defined2014.
2025-03-12 00:41:23.919209587 +0000 UTC m=+316439.524738509: ☑️ agreed by qw4990.

qw4990 · 2025-03-12T00:46:50Z

/test unit-test

tiprow · 2025-03-12T00:47:13Z

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

In response to this:

/test unit-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.

tiprow · 2025-03-12T00:53:33Z

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

Test name	Commit	Details	Required	Rerun command
fast_test_tiprow	`7f64780`	link	true	`/test fast_test_tiprow`

Full PR test history. Your PR dashboard.

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.

ti-chi-bot · 2025-03-19T04:40:18Z

In response to a cherrypick label: new pull request created to branch release-9.0-beta.1: #60161.

…ys to SortKey (pingcap#59918) close pingcap#59827

ti-chi-bot · 2025-03-19T06:21:30Z

In response to a cherrypick label: new pull request created to branch release-8.5: #60162.

…ys to SortKey (#59918) (#60161) close #59827

…ys to SortKey (#59918) (#60162) close #59827

Run full partition pruning if convertToPointGet converted keys to Sor…

3b69e1b

…tKey

mjonss added 3 commits March 6, 2025 10:20

Only rune the full pruner if IndexValues are converted from non binar…

83c5029

…y collation

Added more safety and simplified code

aa12130

Fixed EvalType() and cleanups

17ff66a

ti-chi-bot bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 6, 2025

Added integration test for BatchPointGet

471900f

More tests and better error handling

0b4f9e3

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 Mar 6, 2025

Formatting

ee033bf

mjonss marked this pull request as ready for review March 6, 2025 18:08

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 Mar 6, 2025

mjonss added 2 commits March 7, 2025 10:19

Added RangeColumns test

f5a8dff

Better error handling

57ff0f2

ti-chi-bot bot removed the do-not-merge/needs-triage-completed label Mar 7, 2025

more error handling

52b726a

ti-chi-bot bot added needs-1-more-lgtm Indicates a PR needs 1 more LGTM. release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Mar 10, 2025

mjonss requested a review from qw4990 March 10, 2025 13:13

qw4990 reviewed Mar 11, 2025

View reviewed changes

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

qw4990 reviewed Mar 11, 2025

View reviewed changes

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

qw4990 reviewed Mar 11, 2025

View reviewed changes

mjonss added 2 commits March 11, 2025 14:21

Removed intest.assert, and using ColumnInfos2ColumnsAndNames.

dba86e5

Revert "Removed intest.assert, and using ColumnInfos2ColumnsAndNames."

7f64780

This reverts commit dba86e5.

qw4990 approved these changes Mar 12, 2025

View reviewed changes

ti-chi-bot bot added approved lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Mar 12, 2025

ti-chi-bot bot merged commit 89cab6a into pingcap:master Mar 12, 2025
23 of 24 checks passed

mjonss deleted the partition-point-get-prune-59827 branch March 12, 2025 08:03

ti-chi-bot bot added the needs-cherry-pick-release-9.0-beta.1 label Mar 19, 2025

ti-chi-bot mentioned this pull request Mar 19, 2025

Planner: Run full partition pruning if convertToPointGet converted keys to SortKey (#59918) #60161

Merged

13 tasks

ti-chi-bot bot added the needs-cherry-pick-release-8.5 Should cherry pick this PR to release-8.5 branch. label Mar 19, 2025

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

Planner: Run full partition pruning if convertToPointGet converted ke…

334f32a

…ys to SortKey (pingcap#59918) close pingcap#59827

ti-chi-bot mentioned this pull request Mar 19, 2025

Planner: Run full partition pruning if convertToPointGet converted keys to SortKey (#59918) #60162

Merged

13 tasks

ti-chi-bot bot removed the needs-cherry-pick-release-9.0-beta.1 label Mar 19, 2025

ti-chi-bot bot pushed a commit that referenced this pull request Mar 19, 2025

Planner: Run full partition pruning if convertToPointGet converted ke…

dd701af

…ys to SortKey (#59918) (#60161) close #59827

ti-chi-bot bot pushed a commit that referenced this pull request Apr 14, 2025

Planner: Run full partition pruning if convertToPointGet converted ke…

5902fb6

…ys to SortKey (#59918) (#60162) close #59827

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Planner: Run full partition pruning if convertToPointGet converted keys to SortKey #59918

Planner: Run full partition pruning if convertToPointGet converted keys to SortKey #59918

mjonss commented Mar 5, 2025 •

edited

Loading

ti-chi-bot bot commented Mar 5, 2025

tiprow bot commented Mar 5, 2025

mjonss commented Mar 5, 2025

tiprow bot commented Mar 5, 2025

mjonss commented Mar 6, 2025

tiprow bot commented Mar 6, 2025

codecov bot commented Mar 6, 2025 •

edited

Loading

mjonss commented Mar 6, 2025

qw4990 Mar 11, 2025

mjonss Mar 11, 2025

ti-chi-bot bot commented Mar 12, 2025

ti-chi-bot bot commented Mar 12, 2025

qw4990 commented Mar 12, 2025

tiprow bot commented Mar 12, 2025

tiprow bot commented Mar 12, 2025

ti-chi-bot commented Mar 19, 2025

ti-chi-bot commented Mar 19, 2025

Planner: Run full partition pruning if convertToPointGet converted keys to SortKey #59918

Planner: Run full partition pruning if convertToPointGet converted keys to SortKey #59918

Conversation

mjonss commented Mar 5, 2025 • edited Loading

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

ti-chi-bot bot commented Mar 5, 2025

tiprow bot commented Mar 5, 2025

mjonss commented Mar 5, 2025

tiprow bot commented Mar 5, 2025

mjonss commented Mar 6, 2025

tiprow bot commented Mar 6, 2025

codecov bot commented Mar 6, 2025 • edited Loading

Codecov Report

mjonss commented Mar 6, 2025

qw4990 Mar 11, 2025

Choose a reason for hiding this comment

mjonss Mar 11, 2025

Choose a reason for hiding this comment

ti-chi-bot bot commented Mar 12, 2025

ti-chi-bot bot commented Mar 12, 2025

[LGTM Timeline notifier]

qw4990 commented Mar 12, 2025

tiprow bot commented Mar 12, 2025

tiprow bot commented Mar 12, 2025

ti-chi-bot commented Mar 19, 2025

ti-chi-bot commented Mar 19, 2025

mjonss commented Mar 5, 2025 •

edited

Loading

codecov bot commented Mar 6, 2025 •

edited

Loading