planner, executor: support index merge's order prop push down at the normal way #43881

winoros · 2023-05-16T12:39:17Z

What problem does this PR solve?

Issue Number: ref #41028, close #43577, close #45387

Problem Summary:

Previously, we used a hack to support the order prop push-down for index merge.

What is changed and how it works?

We put the order prop checking at the regular physical property checking. Making it correct for the optimizer to be notified that the order prop is pushed down.

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

…normal way

ti-chi-bot · 2023-05-16T12:39:19Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

Defined2014

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.

planner/core/find_best_task.go

time-and-fate · 2023-05-19T12:40:35Z

planner/core/find_best_task.go

+		// Add sort items for index scan for merge-sort operation between partitions.
+		byItems := make([]*util.ByItems, 0, len(prop.SortItems))
+		for _, si := range prop.SortItems {
+			byItems = append(byItems, &util.ByItems{
+				Expr: si.Col,
+				Desc: si.Desc,
+			})
+		}
+		ts.ByItems = byItems


Looks like we can pass []*util.ByItems to this function to reduce the repeated code here and in convertToPartialIndexScan.

PTAL again. The byItems for each partial plan are the same.
Do we need to build new byItems for each of them? It looks like sharing the same one is enough.

planner/core/find_best_task.go

time-and-fate · 2023-05-24T09:22:59Z

executor/index_merge_reader.go

@@ -348,10 +349,9 @@ func (e *IndexMergeReaderExecutor) startPartialIndexWorker(ctx context.Context,
 					memTracker:         e.memTracker,
 					partitionTableMode: e.partitionTableMode,
 					prunedPartitions:   e.prunedPartitions,
-					byItems:            e.byItems,


Looks like we don't need to change here.
Because we will set this in PhysicalIndexMergeReader.ByItems (Init()) then IndexMergeReaderExecutor.byItems (buildNoRangeIndexMergeReader), so finally they are the same.

Yeah, I want to do the remained part in the later pull.

util/util.go

Defined2014

UnionScan with IndexMerge also have problems both for normal table and parititon table. We should also fixed it in this PR.

mysql> create table t(a int, b int, c int, key idx(a, c), key idx1(b, c));
Query OK, 0 rows affected (0.02 sec)

mysql> begin;
Query OK, 0 rows affected (0.00 sec)

mysql> insert into t(a, b, c) values (6,6,6),(3,3,3),(9,9,9),(4,4,4),(5,5,5),(7,7,7),(8,8,8);
Query OK, 7 rows affected (0.00 sec)
Records: 7  Duplicates: 0  Warnings: 0

mysql> select * from t where a = 6 or b = 3 order by c limit 10;
+------+------+------+
| a    | b    | c    |
+------+------+------+
|    6 |    6 |    6 |
|    3 |    3 |    3 |
+------+------+------+
2 rows in set (0.01 sec)

Like #45140, and sort result in indexMerge.MemTableReader

winoros · 2023-07-05T08:28:41Z

This pr can be closed since #45140 is published.

winoros · 2023-07-07T06:57:03Z

The wrong pull is linked. This pull should be reopened

codecov · 2023-07-19T11:01:24Z

Codecov Report

Merging #43881 (ebfc8f4) into master (a53fd2d) will increase coverage by 0.0141%.
The diff coverage is 75.1020%.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #43881        +/-   ##
================================================
+ Coverage   73.1995%   73.2136%   +0.0141%     
================================================
  Files          1265       1268         +3     
  Lines        390142     390811       +669     
================================================
+ Hits         285582     286127       +545     
- Misses        86243      86336        +93     
- Partials      18317      18348        +31

Flag	Coverage Δ
integration	`78.1388% <ø> (?)`
unit	`73.2051% <75.1020%> (+0.0056%)`	⬆️

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

Components	Coverage Δ
dumpling	`54.0444% <ø> (ø)`
parser	`85.0359% <ø> (ø)`
br	`51.9906% <ø> (ø)`

Defined2014

reset is good since 25dd692d12c04b81fed5514df4d1227ebbebd1a4

executor/test/indexmergereadtest/index_merge_reader_test.go

planner/core/find_best_task.go

time-and-fate · 2023-07-20T14:40:04Z

planner/core/task.go

@@ -935,6 +935,54 @@ func (p *PhysicalLimit) sinkIntoIndexLookUp(t task) bool {
 	return true
 }

+func (p *PhysicalLimit) sinkIntoIndexMerge(t task) bool {


I notice that this function is quite similar to sinkIntoIndexLookUp. Can we merge them into one function?

There's some type assertion in the codes. It might introduce more code complexity for the pull. We can left it for next pulls. Such as add a new common interface for the two struct.

time-and-fate · 2023-07-20T14:43:00Z

planner/core/find_best_task.go

+		// Add sort items for index scan for merge-sort operation between partitions.
+		byItems := make([]*util.ByItems, 0, len(prop.SortItems))
+		for _, si := range prop.SortItems {
+			byItems = append(byItems, &util.ByItems{
+				Expr: si.Col,
+				Desc: si.Desc,
+			})
+		}
+		ts.ByItems = byItems


PTAL again. The byItems for each partial plan are the same.
Do we need to build new byItems for each of them? It looks like sharing the same one is enough.

planner/core/find_best_task.go

executor/index_merge_reader.go

Co-authored-by: Hangjie Mo <mohangjie1995@gmail.com> Co-authored-by: Zhou Kunqin <25057648+time-and-fate@users.noreply.github.com>

ti-chi-bot · 2023-07-31T14:23:54Z

@winoros: 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
pull-br-integration-test	`08c86e0`	link	true	`/test pull-br-integration-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.

ti-chi-bot · 2023-08-01T01:20:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Defined2014, time-and-fate

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,time-and-fate]

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 · 2023-08-01T01:20:40Z

[LGTM Timeline notifier]

Timeline:

2023-07-05 02:43:11.849061025 +0000 UTC m=+171823.782694443: ✖️🔁 reset by Defined2014.
2023-07-31 14:56:37.516490081 +0000 UTC m=+110881.458838598: ☑️ agreed by time-and-fate.
2023-08-01 01:20:38.816828896 +0000 UTC m=+148322.759177428: ☑️ agreed by Defined2014.

planner, executor: support index merge's order prop push down at the …

18b5cee

…normal way

ti-chi-bot bot added release-note-none Denotes a PR that doesn't merit a release note. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels May 16, 2023

fix test

5f8dc44

winoros force-pushed the support-indexmerge-ordered-scan branch from 9bf9fb5 to 5f8dc44 Compare May 16, 2023 13:41

Defined2014 reviewed May 17, 2023

View reviewed changes

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

Defined2014 reviewed May 17, 2023

View reviewed changes

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

time-and-fate reviewed May 19, 2023

View reviewed changes

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

winoros added 2 commits May 23, 2023 21:28

add-generic-shallow-clone

a246875

address comments

08c86e0

Defined2014 approved these changes May 24, 2023

View reviewed changes

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

time-and-fate reviewed May 24, 2023

View reviewed changes

Defined2014 requested changes Jul 5, 2023

View reviewed changes

Defined2014 closed this Jul 6, 2023

winoros reopened this Jul 7, 2023

ti-chi-bot bot deleted a comment from ti-chi-bot Jul 7, 2023

Defined2014 mentioned this pull request Jul 17, 2023

Support keepOrder for memIndexMerge #45387

Closed

Merge branch 'master' into support-indexmerge-ordered-scan

fd75137

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

ti-chi-bot bot deleted a comment from ti-chi-bot Jul 18, 2023

winoros added 2 commits July 18, 2023 21:50

add some changes to align with master

c1da5cd

update tests

880da73

winoros force-pushed the support-indexmerge-ordered-scan branch from d3f011a to 5829795 Compare July 18, 2023 18:13

winoros mentioned this pull request Jul 18, 2023

[WIP] executor: let fetchLoopUnionWithOrderBy support without limit #44309

Closed

12 tasks

winoros force-pushed the support-indexmerge-ordered-scan branch from ab0c632 to 25dd692 Compare July 19, 2023 11:00

Defined2014 reviewed Jul 19, 2023

View reviewed changes

executor/test/indexmergereadtest/index_merge_reader_test.go Outdated Show resolved Hide resolved

executor/test/indexmergereadtest/index_merge_reader_test.go Show resolved Hide resolved

add the case that only order or only limit is pushed

2c3c27c

winoros force-pushed the support-indexmerge-ordered-scan branch from 25dd692 to 2c3c27c Compare July 19, 2023 17:45

Defined2014 reviewed Jul 20, 2023

View reviewed changes

executor/test/indexmergereadtest/index_merge_reader_test.go Outdated Show resolved Hide resolved

time-and-fate reviewed Jul 20, 2023

View reviewed changes

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

winoros added 2 commits July 25, 2023 20:43

Merge branch 'master' into support-indexmerge-ordered-scan

9b86d70

address comments

6013468

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

time-and-fate reviewed Jul 26, 2023

View reviewed changes

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

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

executor/index_merge_reader.go Outdated Show resolved Hide resolved

Defined2014 reviewed Jul 28, 2023

View reviewed changes

executor/index_merge_reader.go Show resolved Hide resolved

executor/index_merge_reader.go Show resolved Hide resolved

winoros and others added 5 commits July 31, 2023 15:18

Apply suggestions from code review

586d581

Co-authored-by: Hangjie Mo <mohangjie1995@gmail.com> Co-authored-by: Zhou Kunqin <25057648+time-and-fate@users.noreply.github.com>

address comments

7f0d0ae

fix the problem of dirty read

9930a4f

add test for dirty read

b97ed79

Merge branch 'master' into support-indexmerge-ordered-scan

7cb3795

time-and-fate approved these changes Jul 31, 2023

View reviewed changes

ti-chi-bot bot added needs-1-more-lgtm Indicates a PR needs 1 more LGTM. approved labels Jul 31, 2023

Merge branch 'master' into support-indexmerge-ordered-scan

ebfc8f4

Defined2014 approved these changes Aug 1, 2023

View reviewed changes

ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Aug 1, 2023

ti-chi-bot bot merged commit 0e068ed into pingcap:master Aug 1, 2023
6 of 9 checks passed

winoros deleted the support-indexmerge-ordered-scan branch August 1, 2023 13:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

planner, executor: support index merge's order prop push down at the normal way #43881

planner, executor: support index merge's order prop push down at the normal way #43881

winoros commented May 16, 2023 •

edited

Loading

ti-chi-bot bot commented May 16, 2023 •

edited

Loading

time-and-fate May 19, 2023

time-and-fate Jul 20, 2023

time-and-fate May 24, 2023

winoros May 29, 2023

Defined2014 left a comment •

edited

Loading

winoros commented Jul 5, 2023

winoros commented Jul 7, 2023

codecov bot commented Jul 19, 2023 •

edited

Loading

Defined2014 left a comment

time-and-fate Jul 20, 2023

winoros Jul 24, 2023

time-and-fate Jul 20, 2023

ti-chi-bot bot commented Jul 31, 2023 •

edited

Loading

ti-chi-bot bot commented Aug 1, 2023

ti-chi-bot bot commented Aug 1, 2023

planner, executor: support index merge's order prop push down at the normal way #43881

planner, executor: support index merge's order prop push down at the normal way #43881

Conversation

winoros commented May 16, 2023 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot bot commented May 16, 2023 • edited Loading

time-and-fate May 19, 2023

Choose a reason for hiding this comment

time-and-fate Jul 20, 2023

Choose a reason for hiding this comment

time-and-fate May 24, 2023

Choose a reason for hiding this comment

winoros May 29, 2023

Choose a reason for hiding this comment

Defined2014 left a comment • edited Loading

Choose a reason for hiding this comment

winoros commented Jul 5, 2023

winoros commented Jul 7, 2023

codecov bot commented Jul 19, 2023 • edited Loading

Codecov Report

Defined2014 left a comment

Choose a reason for hiding this comment

time-and-fate Jul 20, 2023

Choose a reason for hiding this comment

winoros Jul 24, 2023

Choose a reason for hiding this comment

time-and-fate Jul 20, 2023

Choose a reason for hiding this comment

ti-chi-bot bot commented Jul 31, 2023 • edited Loading

ti-chi-bot bot commented Aug 1, 2023

ti-chi-bot bot commented Aug 1, 2023

[LGTM Timeline notifier]

winoros commented May 16, 2023 •

edited

Loading

ti-chi-bot bot commented May 16, 2023 •

edited

Loading

Defined2014 left a comment •

edited

Loading

codecov bot commented Jul 19, 2023 •

edited

Loading

ti-chi-bot bot commented Jul 31, 2023 •

edited

Loading