Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

planner/core: separate aggPrune from aggPushDown #7676

Merged
merged 28 commits into from
Oct 8, 2018
Merged

planner/core: separate aggPrune from aggPushDown #7676

merged 28 commits into from
Oct 8, 2018

Conversation

winoros
Copy link
Member

@winoros winoros commented Sep 12, 2018 •
edited
Loading

What problem does this PR solve?

AggregationPushDown is closed by default since it's not always better. But AggregationElimination is always a good choice.

What is changed and how it works?

So separate it from aggPushDown

Check List

Tests

  • Unit test

Related changes

It's one part from #7531.

Release Note
reopen aggregation pruning optimization.

eurekaka
eurekaka reviewed Sep 12, 2018
View reviewed changes
Copy link
Contributor

@eurekaka eurekaka left a comment

Choose a reason for hiding this comment

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

rest LGTM

plan/optimizer.go Outdated Show resolved Hide resolved
plan/rule_aggregation_elimination.go Outdated Show resolved Hide resolved
@winoros winoros mentioned this pull request Sep 12, 2018
Merged
@winoros
Copy link
Member Author

winoros commented Sep 12, 2018

Struct renaming changes will be pushed after merged #7680 if there's no need to change the code logic.

@eurekaka eurekaka added the sig/planner SIG: Planner label Sep 13, 2018
alivxxx
alivxxx reviewed Sep 13, 2018
View reviewed changes
plan/rule_aggregation_elimination.go Outdated
"github.com/pingcap/tidb/types"
)

type aggregationRecursiveEliminater struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

Eliminator

@zz-jason
Copy link
Member

@winoros PTAL

@zz-jason zz-jason mentioned this pull request Sep 17, 2018
Merged
@winoros
Copy link
Member Author

winoros commented Sep 20, 2018

Comments addressed.
PTAL @zz-jason @eurekaka @lamxTyler

@winoros
Copy link
Member Author

winoros commented Sep 20, 2018

/run-all-tests

alivxxx
alivxxx reviewed Sep 20, 2018
View reviewed changes
Copy link
Contributor

@alivxxx alivxxx left a comment

Choose a reason for hiding this comment

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

LGTM

zz-jason
zz-jason previously approved these changes Sep 20, 2018
View reviewed changes
Copy link
Member

@zz-jason zz-jason left a comment

Choose a reason for hiding this comment

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

LGTM

@zz-jason
Copy link
Member

/run-common-test

@zz-jason zz-jason added the status/LGT2 Indicates that a PR has LGTM 2. label Sep 20, 2018
@winoros
Copy link
Member Author

winoros commented Sep 21, 2018

Seems there's precision error. I'm looking into it.

@winoros winoros dismissed zz-jason’s stale review September 21, 2018 03:40

Waiting for fixing.

@winoros
Copy link
Member Author

winoros commented Sep 21, 2018

Emmm... It seems that i need to reuse expression.aggregation.AggFuncDesc.typeInfer to make the rewriting works properly.

@winoros
Copy link
Member Author

winoros commented Sep 26, 2018

/run-all-tests tidb-test=pr/631

@winoros
Copy link
Member Author

winoros commented Sep 26, 2018

Now it works well.
PTAL @zz-jason @eurekaka @XuHuaiyu

@winoros winoros changed the title plan: separate aggPrune from aggPushDown planner/core: separate aggPrune from aggPushDown Sep 26, 2018
zz-jason
zz-jason reviewed Sep 26, 2018
View reviewed changes
expression/aggregation/descriptor.go Show resolved Hide resolved
planner/core/optimizer.go Outdated
@@ -32,23 +32,25 @@ const (
flagPrunColumns uint64 = 1 << iota
flagEliminateProjection
flagBuildKeyInfo
flagEliminateAgg
Copy link
Member

Choose a reason for hiding this comment

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

how about moving it after flagDecorrelate?

Copy link
Member Author

Choose a reason for hiding this comment

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

Seems reasonable.

planner/core/rule_aggregation_elimination.go Show resolved Hide resolved
winoros
winoros commented Sep 27, 2018
View reviewed changes
planner/core/physical_plan_test.go
@@ -851,7 +851,7 @@ func (s *testPlanSuite) TestDAGPlanBuilderAgg(c *C) {
},
{
sql: "select (select count(1) k from t s where s.a = t.a having k != 0) from t",
best: "LeftHashJoin{TableReader(Table(t))->TableReader(Table(t)->StreamAgg)->StreamAgg->Sel([ne(k, 0)])}(test.t.a,s.a)->Projection->Projection",
best: "MergeLeftOuterJoin{TableReader(Table(t))->TableReader(Table(t))->Projection}(test.t.a,s.a)->Projection->Projection",
Copy link
Member Author

@winoros winoros Sep 27, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

First decorrelate, it becomes select k from t left join (select count(1) k from t s group by a) on s.a=t.a and k != 0.
Then aggregate eliminate, it becomes select k from t left join (select if(isnull(1), 0, 1) k from t s) on s.a=t.a and k != 0. Since if(isnull(1), 0, 1) is always 1. So it simplified as select k from t left join (select 1 k from t s) on s.a=t.a and k != 0.
Finally predicate push down, k != 0 pushed down and converted to 1 != 0 which is always true.
So Sel([ne(k, 0)]) is removed after that aggregate eliminate is operated after decorrelate.

@zz-jason
Copy link
Member

/run-all-tests

@winoros
Copy link
Member Author

winoros commented Sep 27, 2018

/run-common-test tidb-test=pr/631
/run-integration-common-test tidb-test=pr/631

@winoros
Copy link
Member Author

winoros commented Sep 27, 2018

/run-common-test tidb-test=pr/631

@zz-jason
Copy link
Member

LGTM

@zz-jason
Copy link
Member

@eurekaka @XuHuaiyu PTAL

@winoros winoros added the status/LGT1 Indicates that a PR has LGTM 1. label Sep 27, 2018
eurekaka
eurekaka approved these changes Oct 8, 2018
View reviewed changes
Copy link
Contributor

@eurekaka eurekaka left a comment

Choose a reason for hiding this comment

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

LGTM

@eurekaka eurekaka added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Oct 8, 2018
@winoros winoros merged commit 78303cb into pingcap:master Oct 8, 2018
@winoros winoros deleted the extract-agg-prune branch October 8, 2018 11:51
XuHuaiyu added a commit to XuHuaiyu/tidb that referenced this pull request Oct 11, 2018
@XuHuaiyu
cherry-pick from pingcap#7676
a758f75
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zz-jason zz-jason zz-jason left review comments

@alivxxx alivxxx alivxxx left review comments

@eurekaka eurekaka eurekaka approved these changes

Assignees
No one assigned
Labels
release-note sig/planner SIG: Planner 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.

None yet
4 participants
@winoros @zz-jason @eurekaka @alivxxx