executor, planner: support indexMerge limit embeded with non partition table #42811

Defined2014 · 2023-04-04T09:02:57Z

What problem does this PR solve?

Issue Number: ref #41028

Problem Summary:

What is changed and how it works?

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No code

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 · 2023-04-04T09:02:59Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

guo-shaoge
winoros

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

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

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

Defined2014 · 2023-04-11T06:47:00Z

/retest

Defined2014 · 2023-04-11T07:14:54Z

/retest

guo-shaoge · 2023-04-12T08:00:24Z

executor/index_merge_reader.go

+
+// pruneTableWorkerTaskIdxRows prune idxRows and only keep columns that will be used in byItems.
+func (w *indexMergeProcessWorker) pruneTableWorkerTaskIdxRows(task *indexMergeTableTask) {
+	if plan, ok := w.indexMerge.partialPlans[task.partialPlanID][0].(*plannercore.PhysicalTableScan); ok {


Can the bottom of the partial plan is IndexScan?

The IndexScan no need prune retChk. Columns required by byItems are always first.

guo-shaoge · 2023-04-12T08:01:40Z

executor/index_merge_reader.go

+	if plan, ok := w.indexMerge.partialPlans[task.partialPlanID][0].(*plannercore.PhysicalTableScan); ok {
+		prune := make([]int, 0, len(w.indexMerge.byItems))
+		for _, item := range plan.ByItems {
+			c, _ := item.Expr.(*expression.Column)


How about order by an expression like col + 1?

It's not support yet. The planner will not generate a such plan.

order by col+1 will be optimized to order by col.
order by f(col) or order by f(col1, col2) are clearly that they cannot use the order property of the index.

executor/index_merge_reader.go

winoros · 2023-04-13T17:01:11Z

executor/index_merge_reader.go

+	if plan, ok := w.indexMerge.partialPlans[task.partialPlanID][0].(*plannercore.PhysicalTableScan); ok {
+		prune := make([]int, 0, len(w.indexMerge.byItems))
+		for _, item := range plan.ByItems {
+			c, _ := item.Expr.(*expression.Column)


The item.Expr.(*expression.Column).Index is the same thing as the i in for i, col := range plan.Schema().Columns.
You can use the Index directly.

Oh, the ByItems are shared between partial scans. We cannot use the Index. But i think we can use Schema.ColumnIndex or Schema.Contains to find the column by unique id, no need to use ID?

Updated.
It confused me about ID and UniqueID in expression.Column. I found we use ID to compare with model.ColumnInfo, and use UniqueID to compare with expression.Column. @winoros

Yes, ID is the meta data of model.ColumnInfo, created by DDL. So ID is a table-level meta data.
UniqueID is generated during planning. It's unique at the SQL level.
We often use UniqueID to check the equivalence of the expression.Column. Use ID to know which ColumnInfo it represents.

Defined2014 · 2023-04-14T05:31:51Z

/retest

Defined2014 · 2023-04-14T05:53:54Z

/retest

Defined2014 · 2023-04-14T06:05:42Z

/merge

ti-chi-bot · 2023-04-14T06:05:45Z

This pull request has been accepted and is ready to merge.

Commit hash: ea995a9

Defined2014 · 2023-04-14T06:15:01Z

/retest

Defined2014 · 2023-04-14T06:41:29Z

/retest

ti-chi-bot · 2023-04-14T18:17:07Z

@Defined2014: 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
idc-jenkins-ci-tidb/unit-test	`ea995a9`	link	true	`/test unit-test`

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/test-infra repository. I understand the commands that are listed here.

Defined2014 added the needs-ok-to-test Indicates a PR created by contributors and need ORG member send '/ok-to-test' to start testing. label Apr 4, 2023

Defined2014 force-pushed the indexMerge branch from 37fb252 to c8780bf Compare April 7, 2023 07:45

ti-chi-bot removed do-not-merge/needs-linked-issue needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Apr 7, 2023

Defined2014 force-pushed the indexMerge branch from c8780bf to 64701a2 Compare April 10, 2023 13:44

ti-chi-bot added the needs-cherry-pick-release-6.5 Should cherry pick this PR to release-6.5 branch. label Apr 10, 2023

Defined2014 removed needs-cherry-pick-release-6.5 Should cherry pick this PR to release-6.5 branch. needs-ok-to-test Indicates a PR created by contributors and need ORG member send '/ok-to-test' to start testing. labels Apr 11, 2023

Defined2014 force-pushed the indexMerge branch from 64701a2 to 8274b52 Compare April 11, 2023 03:27

Defined2014 changed the title ~~[WIP] executor, planner: support mergeSort for indexMerge~~ executor, planner: support mergeSort for indexMerge Apr 11, 2023

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

Defined2014 changed the title ~~executor, planner: support mergeSort for indexMerge~~ executor, planner: support push down limit into indexMerge for non partition table Apr 11, 2023

Defined2014 changed the title ~~executor, planner: support push down limit into indexMerge for non partition table~~ executor, planner: support indexMerge limit embeded with non partition table Apr 11, 2023

Defined2014 requested review from winoros, tiancaiamao, XuHuaiyu and guo-shaoge April 11, 2023 06:15

Defined2014 requested a review from gengliqi April 11, 2023 07:48

pingcap deleted a comment from ti-chi-bot Apr 11, 2023

Defined2014 force-pushed the indexMerge branch from 97afff3 to ae7c379 Compare April 12, 2023 08:08

guo-shaoge reviewed Apr 12, 2023

View reviewed changes

executor/index_merge_reader.go Outdated Show resolved Hide resolved

Defined2014 closed this Apr 12, 2023

Defined2014 reopened this Apr 12, 2023

guo-shaoge approved these changes Apr 13, 2023

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Apr 13, 2023

winoros reviewed Apr 13, 2023

View reviewed changes

ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 14, 2023

Defined2014 added 8 commits April 14, 2023 12:07

executor, planner: support mergeSort for indexMerge

e79a854

support tableWorker

360c43c

tiny update

0f5ebde

bug fix

8b44734

tiny update

321eb63

fix ci

7856856

tiny update

7583488

update

ea995a9

Defined2014 force-pushed the indexMerge branch from 93a945e to ea995a9 Compare April 14, 2023 04:09

ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 14, 2023

winoros approved these changes Apr 14, 2023

View reviewed changes

ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Apr 14, 2023

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Apr 14, 2023

ti-chi-bot merged commit 2d0564c into pingcap:master Apr 14, 2023

Defined2014 deleted the indexMerge branch April 14, 2023 07:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

executor, planner: support indexMerge limit embeded with non partition table #42811

executor, planner: support indexMerge limit embeded with non partition table #42811

Defined2014 commented Apr 4, 2023 •

edited by winoros

Loading

ti-chi-bot commented Apr 4, 2023 •

edited

Loading

Defined2014 commented Apr 11, 2023

Defined2014 commented Apr 11, 2023

guo-shaoge Apr 12, 2023

Defined2014 Apr 12, 2023 •

edited

Loading

guo-shaoge Apr 12, 2023

Defined2014 Apr 12, 2023 •

edited

Loading

winoros Apr 12, 2023

winoros Apr 13, 2023

winoros Apr 13, 2023

Defined2014 Apr 14, 2023 •

edited

Loading

winoros Apr 14, 2023

Defined2014 commented Apr 14, 2023

Defined2014 commented Apr 14, 2023

Defined2014 commented Apr 14, 2023

ti-chi-bot commented Apr 14, 2023

Defined2014 commented Apr 14, 2023

Defined2014 commented Apr 14, 2023

ti-chi-bot commented Apr 14, 2023

executor, planner: support indexMerge limit embeded with non partition table #42811

executor, planner: support indexMerge limit embeded with non partition table #42811

Conversation

Defined2014 commented Apr 4, 2023 • edited by winoros Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Apr 4, 2023 • edited Loading

Defined2014 commented Apr 11, 2023

Defined2014 commented Apr 11, 2023

guo-shaoge Apr 12, 2023

Choose a reason for hiding this comment

Defined2014 Apr 12, 2023 • edited Loading

Choose a reason for hiding this comment

guo-shaoge Apr 12, 2023

Choose a reason for hiding this comment

Defined2014 Apr 12, 2023 • edited Loading

Choose a reason for hiding this comment

winoros Apr 12, 2023

Choose a reason for hiding this comment

winoros Apr 13, 2023

Choose a reason for hiding this comment

winoros Apr 13, 2023

Choose a reason for hiding this comment

Defined2014 Apr 14, 2023 • edited Loading

Choose a reason for hiding this comment

winoros Apr 14, 2023

Choose a reason for hiding this comment

Defined2014 commented Apr 14, 2023

Defined2014 commented Apr 14, 2023

Defined2014 commented Apr 14, 2023

ti-chi-bot commented Apr 14, 2023

Defined2014 commented Apr 14, 2023

Defined2014 commented Apr 14, 2023

ti-chi-bot commented Apr 14, 2023

Defined2014 commented Apr 4, 2023 •

edited by winoros

Loading

ti-chi-bot commented Apr 4, 2023 •

edited

Loading

Defined2014 Apr 12, 2023 •

edited

Loading

Defined2014 Apr 12, 2023 •

edited

Loading

Defined2014 Apr 14, 2023 •

edited

Loading