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.

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

Planner:eliminate redundant projection #11920

Merged
merged 12 commits into from Sep 2, 2019

Conversation

@jingyugao
Copy link
Contributor

commented Aug 28, 2019

What problem does this PR solve?

If projection plan have a projection child.Then eliminate the child.fix #11863

What is changed and how it works?

Mater:

Projection_4	10000.00	root	plus(1, t.num)
└─Projection_5	10000.00	root	plus(1, test.t.a)
  └─TableReader_7	10000.00	root	data:TableScan_6
    └─TableScan_6	10000.00	cop	table:t, range:[-inf,+inf], keep order:false, stats:pseudo

This pr.

Projection_4	4.00	root	plus(1, plus(1, test.t.a))
└─TableReader_6	4.00	root	data:TableScan_5
  └─TableScan_5	4.00	cop	table:t, range:[-inf,+inf], keep order:false, stats:pseudo

Check List

Tests

  • Unit test:Yes
  • Integration tes:Maybe

Code changes

Simple change

Side effects

Not konwn
Related changes

Maybe not.
Release note

  • Write release note for bug-fix or new feature.
jingyugao added 3 commits Aug 28, 2019
Planner:eliminate redundant projection
Signed-off-by: jingyugao <1121087373@qq.com>
add test cases
Signed-off-by: jingyugao <1121087373@qq.com>
generate explaintest
Signed-off-by: jingyugao <1121087373@qq.com>
rm non-related change
Signed-off-by: jingyugao <1121087373@qq.com>
@codecov

This comment has been minimized.

Copy link

commented Aug 28, 2019

Codecov Report

Merging #11920 into master will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master     #11920   +/-   ##
===========================================
  Coverage   81.2687%   81.2687%           
===========================================
  Files           444        444           
  Lines         95194      95194           
===========================================
  Hits          77363      77363           
  Misses        12341      12341           
  Partials       5490       5490
if child, ok := p.Children()[0].(*LogicalProjection); isProj && ok {
if len(proj.Exprs) == len(child.Exprs) {
for i := range proj.Exprs {
proj.Exprs[i] = replaceColumnOfExpr(proj.Exprs[i], child)

This comment has been minimized.

Copy link
@lamxTyler

lamxTyler Aug 29, 2019

Member

Can we merge it with replaceExprColumns? It should be a more general optimization.

This comment has been minimized.

Copy link
@jingyugao

jingyugao Aug 29, 2019

Author Contributor

I am not sure if all operators could do this optimization.
So I just add some special judgemen.
I just knowprojection->join/apply,projection->sort may do this optimization.

This comment has been minimized.

Copy link
@jingyugao

jingyugao Aug 29, 2019

Author Contributor

It seems all projection except the final one can be eliminate?

planner/core/logical_plan_test.go Outdated Show resolved Hide resolved
planner/core/logical_plan_test.go Outdated Show resolved Hide resolved
planner/core/rule_eliminate_projection.go Outdated Show resolved Hide resolved
planner/core/rule_eliminate_projection.go Outdated Show resolved Hide resolved
fix review comment
Signed-off-by: jingyugao <1121087373@qq.com>

fixed

eliminater even exprs not equal
Signed-off-by: jingyugao <1121087373@qq.com>
@winoros
Copy link
Member

left a comment

Could you please add a SQL to reproduce what you said to @francis0407 ?
HashCode() just encode the UniqueID. So compare hashcode should be same with Equal().

@jingyugao

This comment has been minimized.

Copy link
Contributor Author

commented Aug 29, 2019

Could you please add a SQL to reproduce what you said to @francis0407 ?
HashCode() just encode the UniqueID. So compare hashcode should be same with Equal().
proj.Schema().ColumnIndex(v) will compare table name and column name so I use hashcode.
According to your comment, I replace hashcode with Equal() , it is ok and more readable.

fix review comment
Signed-off-by: jingyugao <1121087373@qq.com>
@winoros

This comment has been minimized.

Copy link
Member

commented Aug 29, 2019

@jingyugao ColumnIndex is compared by UniqueID too.

}
p.Children()[0] = child.Children()[0]
}
}

This comment has been minimized.

Copy link
@eurekaka

eurekaka Aug 29, 2019

Contributor

Can we fix this issue by removing the constraint of canProjectionBeEliminatedLoose and enhancing replace map[string]*expression.Column to replace map[string]expression.Expression? we need to update resolveExprAndReplace though.

This comment has been minimized.

Copy link
@jingyugao

jingyugao Aug 29, 2019

Author Contributor

Some operator will use replace map[string]*expression.Column to replace the schema.
I am not sure how to handle it.

@jingyugao

This comment has been minimized.

Copy link
Contributor Author

commented Aug 29, 2019

@jingyugao ColumnIndex is compared by UniqueID too.

Yes,you are right. I have modify it

@jingyugao

This comment has been minimized.

Copy link
Contributor Author

commented Aug 29, 2019

@jingyugao ColumnIndex is compared by UniqueID too.

Yes,you are right. I have modify it

That error arises because I forget to remove some dirty code.

@eurekaka
Copy link
Contributor

left a comment

LGTM

@francis0407

This comment has been minimized.

Copy link
Contributor

commented Aug 30, 2019

/run-all-tests

jingyugao added 2 commits Aug 31, 2019
@jingyugao

This comment has been minimized.

Copy link
Contributor Author

commented Aug 31, 2019

/run-all-tests

@jingyugao

This comment has been minimized.

Copy link
Contributor Author

commented Aug 31, 2019

@wjhuang2016 @eurekaka @winoros @francis0407 PTAL
I update the branch and fix bug that prune sleep function by mistake.

@francis0407
Copy link
Contributor

left a comment

Good Job! cc @zz-jason

planner/core/rule_column_pruning.go Outdated Show resolved Hide resolved
@jingyugao

This comment has been minimized.

Copy link
Contributor Author

commented Aug 31, 2019

/run-all-tests

@jingyugao jingyugao force-pushed the jingyugao:issue#11863 branch from f0104a1 to a962f54 Aug 31, 2019

@jingyugao

This comment has been minimized.

Copy link
Contributor Author

commented Aug 31, 2019

/run-all-tests

@zz-jason
Copy link
Member

left a comment

LGTM

@@ -137,6 +137,14 @@ func (pe *projectionEliminater) eliminate(p LogicalPlan, replace map[string]*exp
}
}
p.replaceExprColumns(replace)
if isProj {
if child, ok := p.Children()[0].(*LogicalProjection); ok && !exprsHasSideEffects(child.Exprs) {

This comment has been minimized.

Copy link
@zz-jason

zz-jason Sep 2, 2019

Member

This optimization seems a little ad-hoc. I think a general way is what @eurekaka pointed out. But never mind, the current optimizer is not well suited for these kinds of optimizations.

In the cascades planner, we can push project operator down, merge two adjacent project operators when we find this pattern.

@sre-bot

This comment has been minimized.

Copy link

commented Sep 2, 2019

/run-all-tests

@sre-bot sre-bot merged commit 7372dcd into pingcap:master Sep 2, 2019

13 checks passed

idc-jenkins-ci-tidb/build Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/build_check_race Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/check_dev Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/check_dev_2 Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/common-test job succeeded
Details
idc-jenkins-ci-tidb/integration-common-test Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/integration-compatibility-test Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/integration-ddl-test Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/mybatis-test job succeeded
Details
idc-jenkins-ci-tidb/sqllogic-test-1 Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/sqllogic-test-2 Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/unit-test Jenkins job succeeded.
Details
license/cla Contributor License Agreement is signed.
Details
jingyugao added a commit to jingyugao/tidb that referenced this pull request Sep 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.