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, executor: add post-process after physical plan optimization and move buildProjBelowAgg from executor to planner #8828

Merged
merged 5 commits into from Jan 3, 2019

Conversation

@qw4990
Copy link
Contributor

qw4990 commented Dec 26, 2018

What problem does this PR solve?

Add a post process function after DoOptimize() in package "planner/core". (#8713)
In this post process, we can:

  1. eliminate physical projection eliminatePhysicalProjection;
  2. build projection operator below aggregation operator;

So after this code change, the plan in explain xx is equals to explain analyze xx, and the plan generation is back to the "planner" package.

Hope this can make the code more readable and maintainable.

What is changed and how it works?

  1. add a postOptimize() after DoOptimize() at planner/core/optimizer.go;
  2. move buildProjBelowAgg() from executor/builder.go to planner/core/optimizer.go;
  3. update some unit tests;

Check List

Tests

  • Unit test
  • Integration test

This change is Reviewable

@CLAassistant

This comment has been minimized.

Copy link

CLAassistant commented Dec 26, 2018

CLA assistant check
All committers have signed the CLA.

Show resolved Hide resolved planner/core/optimizer.go Outdated
@XuHuaiyu

This comment has been minimized.

Copy link
Contributor

XuHuaiyu commented Dec 26, 2018

/run-all-tests

@qw4990 qw4990 changed the title planner, executor: add post-process after physical plan optimazation … planner, executor: add post-process after physical plan optimazation and move buildProjBelowAgg from executor to planner Dec 26, 2018

Show resolved Hide resolved planner/core/optimizer.go Outdated
Show resolved Hide resolved planner/core/optimizer.go Outdated
Show resolved Hide resolved planner/core/optimizer.go Outdated
Show resolved Hide resolved planner/core/optimizer.go Outdated
Show resolved Hide resolved planner/core/optimizer.go Outdated
Show resolved Hide resolved planner/core/optimizer.go Outdated

@XuHuaiyu XuHuaiyu changed the title planner, executor: add post-process after physical plan optimazation and move buildProjBelowAgg from executor to planner planner, executor: add post-process after physical plan optimization and move buildProjBelowAgg from executor to planner Dec 28, 2018

@qw4990 qw4990 force-pushed the qw4990:phy_post_proc branch from 2635771 to 2b51b39 Dec 28, 2018

qw4990 added a commit to qw4990/tidb that referenced this pull request Dec 28, 2018

planner, executor: add post-process after physical plan optimazation …
…and move buildProjBelowAgg from executor to planner (pingcap#8828)
Show resolved Hide resolved planner/core/optimizer.go Outdated

@qw4990 qw4990 force-pushed the qw4990:phy_post_proc branch from f65d3e5 to 52eca3a Dec 28, 2018

qw4990 added a commit to qw4990/tidb that referenced this pull request Dec 28, 2018

planner, executor: add post-process after physical plan optimazation …
…and move buildProjBelowAgg from executor to planner (pingcap#8828)

qw4990 added a commit to qw4990/tidb that referenced this pull request Dec 28, 2018

planner, executor: add post-process after physical plan optimazation …
…and move buildProjBelowAgg from executor to planner (pingcap#8828)

qw4990 added some commits Dec 25, 2018

planner, executor: add post-process after physical plan optimazation …
…and move buildProjBelowAgg from executor to planner
planner, executor: add post-process after physical plan optimazation …
…and move buildProjBelowAgg from executor to planner (#8828)
planner, executor: add post-process after physical plan optimazation …
…and move buildProjBelowAgg from executor to planner (#8828)
planner, executor: add post-process after physical plan optimazation …
…and move buildProjBelowAgg from executor to planner (#8828)

@qw4990 qw4990 force-pushed the qw4990:phy_post_proc branch from 52eca3a to 9fedc6f Jan 2, 2019

@XuHuaiyu
Copy link
Contributor

XuHuaiyu left a comment

LGTM

@XuHuaiyu XuHuaiyu added the status/LGT1 label Jan 3, 2019

@XuHuaiyu XuHuaiyu requested review from winoros and zz-jason Jan 3, 2019

@zz-jason
Copy link
Member

zz-jason left a comment

LGTM

@zz-jason zz-jason added status/LGT2 and removed status/LGT1 labels Jan 3, 2019

@zz-jason

This comment has been minimized.

Copy link
Member

zz-jason commented Jan 3, 2019

/run-all-tests

@qw4990 qw4990 merged commit 243120c into pingcap:master Jan 3, 2019

12 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
idc-jenkins-ci-tidb/build 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

@qw4990 qw4990 added this to Done in Planner via automation Jan 3, 2019

Show resolved Hide resolved planner/core/cbo_test.go

// If the mode is FinalMode, we do not need to wrap cast upon the args,
// since the types of the args are already the expected.
if len(aggFuncs) > 0 && aggFuncs[0].Mode != aggregation.FinalMode {

This comment has been minimized.

@eurekaka

eurekaka Jan 3, 2019

Contributor

if the mode is Partial2Mode, we don't need the cast either? @XuHuaiyu @qw4990

}

child := aggPlan.Children()[0]
prop := aggPlan.GetChildReqProps(0).Clone()

This comment has been minimized.

@eurekaka

eurekaka Jan 3, 2019

Contributor

prop would not be used anymore since we have finished physical optimization, so we can just set it nil?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment