expression, planner: allow pushdown count distinct when enumerate physical plans by tisonkun · Pull Request #22867 · pingcap/tidb

tisonkun · 2021-02-22T08:42:56Z

What problem does this PR solve?

Issue Number: related to pingcap/tiflash#1428

co pingcap/tiflash#1457

Problem Summary:

support push down count distinct aggregation to tiflash in mpp mode.

What is changed and how it works?

Related changes

N/A

Check List

Tests

Unit test
Integration test

Side effects

N/A

Release note

Support DISTINCT AGG pushdown to TiFlash

sre-bot · 2021-02-22T08:43:07Z

No release note, Please follow https://github.com/pingcap/community/blob/master/contributors/release-note-checker.md

CLAassistant · 2021-02-22T08:43:25Z

All committers have signed the CLA.

zhouqiang-cl · 2021-02-22T10:25:39Z

/run-check-release-note

sre-bot · 2021-02-22T10:25:49Z

No release note, Please follow https://github.com/pingcap/community/blob/master/contributors/release-note-checker.md

zhouqiang-cl · 2021-02-22T10:31:09Z

/run-check-release-note

zhouqiang-cl · 2021-02-22T10:33:45Z

/run-check-release-note

sre-bot · 2021-02-22T10:33:57Z

No release note, Please follow https://github.com/pingcap/community/blob/master/contributors/release-note-checker.md

zhouqiang-cl · 2021-02-22T12:04:06Z

/run-check-release-note

ti-srebot · 2021-02-22T12:04:18Z

No release note, Please follow https://github.com/pingcap/community/blob/master/contributors/release-note-checker.md

ti-srebot · 2021-02-23T07:04:56Z

No release note, Please follow https://github.com/pingcap/community/blob/master/contributors/release-note-checker.md

ti-srebot · 2021-02-23T08:19:22Z

No release note, Please follow https://github.com/pingcap/community/blob/master/contributors/release-note-checker.md

tisonkun · 2021-02-23T08:21:24Z

/run-check-release-note

tisonkun · 2021-02-23T08:53:48Z

ref pingcap/tiflash#1428

tisonkun · 2021-02-24T03:19:12Z

@hanfei1991 please take a look. I'm unsure if we should make changes in PhysicalHashAgg.attach2TaskForMpp in order to only allow Mpp1Phase to go, or others are already rejected by current logic.

So far, the planner will choose Mpp1Phase physical plan. Tests will be added later.

hanfei1991 · 2021-03-10T04:48:41Z

add a test in planner: select count(distinct **), sum(distinct **) from .... . To prove that the pushdown check really works well

Signed-off-by: tison <wander4096@gmail.com>

tisonkun · 2021-03-10T06:55:48Z

add a test in planner: select count(distinct **), sum(distinct **) from .... . To prove that the pushdown check really works well

add failing logic as b834726

tisonkun · 2021-03-10T07:49:45Z

@hanfei1991 @windtalker PTAL

tisonkun · 2021-03-10T11:59:25Z

@fzhedu please take a look.

fzhedu · 2021-03-11T08:36:03Z

planner/core/exhaust_physical_plans.go

-		// If AllowDistinctAggPushDown is set to true, we should not consider RootTask.
-		if !la.ctx.GetSessionVars().AllowDistinctAggPushDown {
+		// TODO: remove after the cost estimation of distinct pushdown is implemented.
+		if !la.ctx.GetSessionVars().AllowDistinctAggPushDown && !canPushDownToMPP {


this change is wrong.
If AllowDistinctAggPushDown is set to false, we should only consider RootTask.

also need a test to ensure this.

IIRC @hanfei1991 stands that we don't share the same config with AllowDistinctAggPushDown which is introduced by #15500 and focuses on rewrite distinct in agg into group by.

Yeah, the "AllowDistinctAggPushDown" doesn't decide whether to push mpp query.

after disscussed with @zanmato1984 and @hanfei1991 , I approve this change, even through it induces some unexpections.

planner/core/testdata/integration_serial_suite_out.json

expression/aggregation/agg_to_pb_test.go

fzhedu

/LGTM

ti-chi-bot · 2021-03-11T10:29:22Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

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

Details

Reviewer can indicate their review by writing /lgtm in a comment.
Reviewer can cancel approval by writing /lgtm cancel in a comment.

hanfei1991 · 2021-03-11T10:29:59Z

/merge

ti-chi-bot · 2021-03-11T10:30:02Z

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

Details

Commit hash: 4b5cd54

ti-chi-bot · 2021-03-11T11:07:12Z

@tisonkun: Your PR was out of date, I have automatically updated it for you.

At the same time I will also trigger all tests for you:

/run-all-tests

If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

Details

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.

tisonkun requested a review from a team as a code owner February 22, 2021 08:42

tisonkun requested review from winoros and removed request for a team February 22, 2021 08:42

ti-srebot added the first-time-contributor Indicates that the PR was contributed by an external member and is a first-time contributor. label Feb 22, 2021

tisonkun marked this pull request as draft February 22, 2021 12:34

tisonkun force-pushed the pushdown-distinct-aggregation branch from 9dd6010 to c314ede Compare February 23, 2021 07:04

ti-chi-bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Feb 23, 2021

ti-chi-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 23, 2021

tisonkun force-pushed the pushdown-distinct-aggregation branch 2 times, most recently from e68fe4e to 47047c5 Compare February 23, 2021 08:22

tisonkun force-pushed the pushdown-distinct-aggregation branch from 47047c5 to 8464347 Compare March 4, 2021 02:41

tisonkun mentioned this pull request Mar 4, 2021

add has_distinct property in expression.Expr pingcap/tipb#215

Merged

tisonkun force-pushed the pushdown-distinct-aggregation branch from 8464347 to 5aab25c Compare March 5, 2021 02:24

tisonkun changed the title ~~WIP: allow pushdown count distinct when enumerate physical plans~~ allow pushdown count distinct when enumerate physical plans Mar 5, 2021

tisonkun marked this pull request as ready for review March 5, 2021 02:35

tisonkun requested a review from a team as a code owner March 5, 2021 02:35

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Mar 10, 2021

add failing test

b834726

Signed-off-by: tison <wander4096@gmail.com>

tisonkun force-pushed the pushdown-distinct-aggregation branch from 970658b to b834726 Compare March 10, 2021 06:50

Merge branch 'master' into pushdown-distinct-aggregation

98700e0

Merge branch 'master' into pushdown-distinct-aggregation

82b40b9

tisonkun added 2 commits March 10, 2021 16:14

update it output sql

0ed9cbe

Merge branch 'master' into pushdown-distinct-aggregation

f9fcabf

tisonkun added 2 commits March 11, 2021 10:57

Merge branch 'master' into pushdown-distinct-aggregation

21c1f6a

Merge branch 'master' into pushdown-distinct-aggregation

e4c6693

fzhedu reviewed Mar 11, 2021

View reviewed changes

planner/core/testdata/integration_serial_suite_out.json Show resolved Hide resolved

fzhedu reviewed Mar 11, 2021

View reviewed changes

expression/aggregation/agg_to_pb_test.go Show resolved Hide resolved

fzhedu approved these changes Mar 11, 2021

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 Mar 11, 2021

Merge branch 'master' into pushdown-distinct-aggregation

4b5cd54

hanfei1991 self-assigned this Mar 11, 2021

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

Merge branch 'master' into pushdown-distinct-aggregation

60fc9d1

ti-chi-bot merged commit 362883c into pingcap:master Mar 11, 2021

tisonkun mentioned this pull request Mar 12, 2021

[MPP] Support count distinct pingcap/tiflash#1428

Closed

tisonkun deleted the pushdown-distinct-aggregation branch April 17, 2021 09:51

Conversation

tisonkun commented Feb 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What problem does this PR solve?

What is changed and how it works?

Related changes

Check List

Release note

Uh oh!

sre-bot commented Feb 22, 2021

Uh oh!

CLAassistant commented Feb 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zhouqiang-cl commented Feb 22, 2021

Uh oh!

sre-bot commented Feb 22, 2021

Uh oh!

zhouqiang-cl commented Feb 22, 2021

Uh oh!

zhouqiang-cl commented Feb 22, 2021

Uh oh!

sre-bot commented Feb 22, 2021

Uh oh!

zhouqiang-cl commented Feb 22, 2021

Uh oh!

ti-srebot commented Feb 22, 2021

Uh oh!

ti-srebot commented Feb 23, 2021

Uh oh!

ti-srebot commented Feb 23, 2021

Uh oh!

tisonkun commented Feb 23, 2021

Uh oh!

tisonkun commented Feb 23, 2021

Uh oh!

tisonkun commented Feb 24, 2021

Uh oh!

hanfei1991 commented Mar 10, 2021

Uh oh!

tisonkun commented Mar 10, 2021

Uh oh!

tisonkun commented Mar 10, 2021

Uh oh!

tisonkun commented Mar 10, 2021

Uh oh!

fzhedu Mar 11, 2021 • 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!

fzhedu Mar 11, 2021

Choose a reason for hiding this comment

Uh oh!

tisonkun Mar 11, 2021 • 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!

hanfei1991 Mar 11, 2021

Choose a reason for hiding this comment

Uh oh!

fzhedu Mar 11, 2021 • 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!

Uh oh!

Uh oh!

fzhedu left a comment

Choose a reason for hiding this comment

Uh oh!

ti-chi-bot commented Mar 11, 2021

Uh oh!

hanfei1991 commented Mar 11, 2021

Uh oh!

ti-chi-bot commented Mar 11, 2021

Uh oh!

ti-chi-bot commented Mar 11, 2021

Uh oh!

Reviewers

Assignees

Labels

tisonkun commented Feb 22, 2021 •

edited

Loading

CLAassistant commented Feb 22, 2021 •

edited

Loading

fzhedu Mar 11, 2021 •

edited

Loading

tisonkun Mar 11, 2021 •

edited

Loading

fzhedu Mar 11, 2021 •

edited

Loading