planner: enforce projection push down #25450

fzhedu · 2021-06-15T12:23:40Z

What problem does this PR solve?

Issue Number: close #xxx

Problem Summary: if enforced MPP, we push down projection to tiflash.

What is changed and how it works?

Proposal: xxx

What's Changed:

How it Works:

Related changes

Need to cherry-pick to the release branch

Check List

Tests

Integration test

Release note

No release note

fzhedu · 2021-06-16T04:37:24Z

executor/tiflash_test.go

@@ -380,7 +380,7 @@ func (s *tiflashTestSuite) TestTiFlashPartitionTableReader(c *C) {

 	tk.MustExec("SET tidb_enforce_mpp=1")
 	tk.MustExec("set @@session.tidb_isolation_read_engines='tiflash'")
-	for i := 0; i < 100; i++ {
+	for i := 0; i < 10; i++ {


before this PR, this test do not run in MPP. after this PR, this test timeout, so reduce the running times.

fzhedu · 2021-06-16T07:26:23Z

/run-all-tests

hanfei1991 · 2021-06-16T07:38:38Z

planner/core/exhaust_physical_plans.go

-	return []PhysicalPlan{proj}, true, nil
+	newProps := []*property.PhysicalProperty{newProp}
+	// generate a mpp task candidate if enforced mpp
+	if newProp.TaskTp != property.MppTaskType && p.SCtx().GetSessionVars().IsMPPEnforced() {


better to check "canPushDownToCop(TiFlash)"

It can avoid some plan id changes

done, by the way, should we consider whether expressions can push down?

fzhedu · 2021-06-18T10:02:35Z

planner/core/testdata/enforce_mpp_suite_in.json

-      "explain format='verbose' select count(*) from t where a=1",
-      "explain format='verbose' select /*+ read_from_storage(tikv[t]) */ count(*) from t where a=1",
-      "explain format='verbose' select /*+ read_from_storage(tiflash[t]) */ count(*) from t where a=1",
+      "explain format = 'brief' select count(*) from t where a=1",


@LittleFall can I change this? because it binds the id of each operator, which will be changed in future due to other PRs.

I set format='verbose' to check that the task cost is reduced by enforce_mpp. So I suggest keeping verbose in the test suite TestEnforceMPP, and use format='brief' to check other tests.

LittleFall

lgtm

LittleFall · 2021-06-18T13:37:02Z

planner/core/testdata/enforce_mpp_suite_in.json

-      "explain format='verbose' select count(*) from t where a=1",
-      "explain format='verbose' select /*+ read_from_storage(tikv[t]) */ count(*) from t where a=1",
-      "explain format='verbose' select /*+ read_from_storage(tiflash[t]) */ count(*) from t where a=1",
+      "explain format = 'brief' select count(*) from t where a=1",


I set format='verbose' to check that the task cost is reduced by enforce_mpp. So I suggest keeping verbose in the test suite TestEnforceMPP, and use format='brief' to check other tests.

ti-chi-bot · 2021-06-18T13:37:14Z

@LittleFall: Thanks for your review. The bot only counts approvals from reviewers and higher roles in list, but you're still welcome to leave your comments.

In response to this:

lgtm

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.

lzmhhh123 · 2021-06-21T10:04:34Z

planner/core/exhaust_physical_plans.go

-	return []PhysicalPlan{proj}, true, nil
+	newProps := []*property.PhysicalProperty{newProp}
+	// generate a mpp task candidate if enforced mpp
+	if newProp.TaskTp != property.MppTaskType && p.SCtx().GetSessionVars().IsMPPEnforced() && p.canPushToCop(kv.TiFlash) &&


It's a very hack code here. Can we find the root cause why properties don't contain MppTaskType? These generated plans are dangerous with force MPP type.

if projection is the top operator, we should add a option of MPPTaskType. it is not dangenous, because in the physical phase, if it cannot satisfy MPPTaskType, it only generates root tasks.

ti-chi-bot · 2021-06-22T09:47:24Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

LittleFall
hanfei1991

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.

fzhedu · 2021-06-22T10:45:38Z

/run-all-tests

fzhedu · 2021-06-22T11:41:44Z

/run-check_dev_2

fzhedu · 2021-06-22T11:46:44Z

/merge

ti-chi-bot · 2021-06-22T11:46:48Z

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

Commit hash: 3dbd838

fzhedu · 2021-06-22T12:09:11Z

/run-check_dev_2

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

ti-srebot · 2021-06-24T07:41:19Z

cherry pick to release-5.0 in PR #25740

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

ti-srebot · 2021-06-24T07:41:56Z

cherry pick to release-5.1 in PR #25741

fzhedu requested a review from a team as a code owner June 15, 2021 12:23

fzhedu requested review from hanfei1991 and removed request for a team June 15, 2021 12:23

ti-chi-bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jun 15, 2021

fzhedu requested a review from LittleFall June 15, 2021 12:29

fzhedu force-pushed the enforceProjPushDown branch from 74861b8 to b61feb1 Compare June 16, 2021 04:26

fzhedu requested a review from a team as a code owner June 16, 2021 04:26

fzhedu requested review from lzmhhh123 and removed request for a team June 16, 2021 04:26

github-actions bot added the sig/execution SIG execution label Jun 16, 2021

fzhedu requested a review from qw4990 June 16, 2021 04:36

fzhedu commented Jun 16, 2021

View reviewed changes

hanfei1991 reviewed Jun 16, 2021

View reviewed changes

fzhedu force-pushed the enforceProjPushDown branch from 207787e to c287b6d Compare June 18, 2021 09:38

fzhedu commented Jun 18, 2021

View reviewed changes

LittleFall approved these changes Jun 18, 2021

View reviewed changes

ti-chi-bot removed the sig/execution SIG execution label Jun 18, 2021

LittleFall approved these changes Jun 18, 2021

View reviewed changes

ti-chi-bot added status/LGT1 Indicates that a PR has LGTM 1. sig/execution SIG execution and removed sig/execution SIG execution labels Jun 18, 2021

qw4990 removed their request for review June 21, 2021 03:30

lzmhhh123 reviewed Jun 21, 2021

View reviewed changes

fzhedu added 4 commits June 22, 2021 17:41

enforce projection push down

d3979b2

update test to avoid timeout

d68476c

add canPushToCop check

b860c35

add expression check

951036c

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 Jun 22, 2021

Merge branch 'master' into enforceProjPushDown

3dbd838

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Jun 22, 2021

Merge branch 'master' into enforceProjPushDown

8325c2a

Merge branch 'master' into enforceProjPushDown

76d69d6

ti-chi-bot merged commit 9ca449b into pingcap:master Jun 22, 2021

fzhedu added needs-cherry-pick-release-5.0 needs-cherry-pick-release-5.1 labels Jun 24, 2021

ti-srebot mentioned this pull request Jun 24, 2021

planner: enforce projection push down (#25450) #25740

Closed

ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Jun 24, 2021

cherry pick pingcap#25450 to release-5.0

5d97c5a

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

ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Jun 24, 2021

cherry pick pingcap#25450 to release-5.1

a40b953

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

ti-srebot mentioned this pull request Jun 24, 2021

planner: enforce projection push down (#25450) #25741

Merged

This was referenced Jul 13, 2021

planner: let projection can push down to TiFlash when mpp is not enforced. #26207

Closed

Let top-level projection can push down to TiFlash when mpp is not enforced. #26242

Closed

LittleFall pushed a commit to LittleFall/tidb that referenced this pull request Jul 20, 2021

planner: enforce projection push down (pingcap#25450)

68cdb73

LittleFall mentioned this pull request Jul 20, 2021

DNM: planner: cherry-pick enforce-mpp to v5.0 #26378

Closed

10 tasks

LittleFall pushed a commit to LittleFall/tidb that referenced this pull request Jul 20, 2021

planner: enforce projection push down (pingcap#25450)

6c66105

This was referenced Jul 20, 2021

planner: cherry-pick enforce-mpp to v5.0 #26382

Merged

planner: report warning when mpp join can't be selected whether it is MPPTask. #26487

Merged

ti-chi-bot pushed a commit that referenced this pull request Jul 22, 2021

planner: enforce projection push down (#25450) (#25741)

9094b44

time-and-fate mentioned this pull request Mar 4, 2022

planner: fix stream agg pushed down to mpp plan unexpectedly #32752

Merged

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

planner: enforce projection push down #25450

planner: enforce projection push down #25450

fzhedu commented Jun 15, 2021

fzhedu Jun 16, 2021

fzhedu commented Jun 16, 2021

hanfei1991 Jun 16, 2021

hanfei1991 Jun 16, 2021

fzhedu Jun 18, 2021

fzhedu Jun 18, 2021

LittleFall Jun 18, 2021

LittleFall left a comment

LittleFall Jun 18, 2021

ti-chi-bot commented Jun 18, 2021

lzmhhh123 Jun 21, 2021

fzhedu Jun 22, 2021

ti-chi-bot commented Jun 22, 2021

fzhedu commented Jun 22, 2021

fzhedu commented Jun 22, 2021

fzhedu commented Jun 22, 2021

ti-chi-bot commented Jun 22, 2021

fzhedu commented Jun 22, 2021

ti-srebot commented Jun 24, 2021

ti-srebot commented Jun 24, 2021

planner: enforce projection push down #25450

planner: enforce projection push down #25450

Conversation

fzhedu commented Jun 15, 2021

What problem does this PR solve?

What is changed and how it works?

Related changes

Check List

Release note

Choose a reason for hiding this comment

fzhedu commented Jun 16, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LittleFall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ti-chi-bot commented Jun 18, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ti-chi-bot commented Jun 22, 2021

fzhedu commented Jun 22, 2021

fzhedu commented Jun 22, 2021

fzhedu commented Jun 22, 2021

ti-chi-bot commented Jun 22, 2021

fzhedu commented Jun 22, 2021

ti-srebot commented Jun 24, 2021

ti-srebot commented Jun 24, 2021