Skip to content

Support countdistinct pushdown#1457

Merged
tisonkun merged 18 commits into
pingcap:masterfrom
tisonkun:pushdown-distinct-aggregation
Mar 10, 2021
Merged

Support countdistinct pushdown#1457
tisonkun merged 18 commits into
pingcap:masterfrom
tisonkun:pushdown-distinct-aggregation

Conversation

@tisonkun
Copy link
Copy Markdown
Contributor

@tisonkun tisonkun commented Feb 23, 2021 •
edited
Loading

What problem does this PR solve?

Issue Number: ref #1428

co pingcap/tidb#22867

Problem Summary:

Support count distinct pushdown.

What is changed and how it works?

Proposal: Pushing down DISTINCT AGG in MPP

What's Changed:

Update contrib/tipb to respect Expr.has_distinct. When the flag is true, append agg_func_name with Distinct suffix.

Also replace countdistinct with implementation in settings, which is the same logic.

Workaround -Null Combinator to bypass count distinct with null value. An elegant impl could follow with ClickHouse/ClickHouse#11661

Related changes

  • PR to update pingcap/docs/pingcap/docs-cn:
  • Need to cherry-pick to the release branch

Check List

Tests

  • Integration test
  • Manual test (add detailed scripts or steps below)

Side effects

N/A

Release note

  • Support push down count distinct in MPP mode.

@ti-srebot 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 23, 2021
@tisonkun tisonkun changed the title append -Distinct if tipb::Expr has_distinct present [WIP] append -Distinct if tipb::Expr has_distinct present Feb 23, 2021
@tisonkun tisonkun changed the title [WIP] append -Distinct if tipb::Expr has_distinct present Support countdistinct pushdown Feb 24, 2021
@tisonkun
Copy link
Copy Markdown
Contributor Author

tisonkun commented Feb 25, 2021

/run-all-tests

@tisonkun
Copy link
Copy Markdown
Contributor Author

tisonkun commented Feb 25, 2021

/rebuild

@tisonkun
Copy link
Copy Markdown
Contributor Author

tisonkun commented Feb 26, 2021

/run-all-tests

@tisonkun
Copy link
Copy Markdown
Contributor Author

tisonkun commented Feb 26, 2021

/run-all-tests

@tisonkun
Copy link
Copy Markdown
Contributor Author

tisonkun commented Feb 26, 2021

/rebuild

@tisonkun
Copy link
Copy Markdown
Contributor Author

tisonkun commented Feb 26, 2021

/run-all-tests

@tisonkun tisonkun requested a review from hanfei1991 February 26, 2021 10:22
@tisonkun tisonkun mentioned this pull request Mar 9, 2021
Merged
@tisonkun
Copy link
Copy Markdown
Contributor Author

tisonkun commented Mar 9, 2021

@hanfei1991 @windtalker PTAL

@hanfei1991
Copy link
Copy Markdown
Member

hanfei1991 commented Mar 9, 2021

A little question: What does the kvproto change for?

@tisonkun
Copy link
Copy Markdown
Contributor Author

tisonkun commented Mar 9, 2021

A little question: What does the kvproto change for?

We don't have effective change for kvproto, maybe polluted casually, reverting...

@hanfei1991
Copy link
Copy Markdown
Member

hanfei1991 commented Mar 9, 2021

/LGTM

@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Mar 9, 2021
@tisonkun
Copy link
Copy Markdown
Contributor Author

tisonkun commented Mar 9, 2021

/run-all-tests

@tisonkun
Copy link
Copy Markdown
Contributor Author

tisonkun commented Mar 9, 2021

/run-all-tests

windtalker
windtalker reviewed Mar 10, 2021
View reviewed changes
Comment thread dbms/src/Flash/Coprocessor/DAGUtils.cpp Outdated
Comment thread dbms/src/Flash/Coprocessor/DAGExpressionAnalyzer.cpp Outdated
@tisonkun tisonkun self-assigned this Mar 10, 2021
windtalker
windtalker reviewed Mar 10, 2021
View reviewed changes
Comment thread dbms/src/Flash/Coprocessor/DAGUtils.cpp Outdated
@tisonkun
Copy link
Copy Markdown
Contributor Author

tisonkun commented Mar 10, 2021

/run-all-tests

@tisonkun
Copy link
Copy Markdown
Contributor Author

tisonkun commented Mar 10, 2021

@windtalker please take a look.

windtalker
windtalker approved these changes Mar 10, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@windtalker windtalker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ti-srebot ti-srebot removed the status/LGT1 Indicates that a PR has LGTM 1. label Mar 10, 2021
@ti-srebot ti-srebot added the status/LGT2 Indicates that a PR has LGTM 2. label Mar 10, 2021
@tisonkun tisonkun merged commit b1be972 into pingcap:master Mar 10, 2021
@tisonkun
Copy link
Copy Markdown
Contributor Author

tisonkun commented Mar 10, 2021

Thanks for your reviews!

@tisonkun tisonkun mentioned this pull request Mar 12, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@windtalker windtalker windtalker approved these changes

@ti-srebot ti-srebot ti-srebot approved these changes

@hanfei1991 hanfei1991 Awaiting requested review from hanfei1991

Assignees

No one assigned

Labels

first-time-contributor Indicates that the PR was contributed by an external member and is a first-time contributor. status/LGT2 Indicates that a PR has LGTM 2.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@tisonkun @hanfei1991 @windtalker @ti-srebot