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: projection don't new unnecessary column #13406

Merged
merged 11 commits into from
Nov 20, 2019

Conversation

winoros
Copy link
Member

@winoros winoros commented Nov 12, 2019

What problem does this PR solve?

Since name is splitted from the Column.
The project don't need to new column when the expr is just a column.
This makes it possible to merge the two project eliminating process which TiDB current has.

What is changed and how it works?

If the expression is column. We use it directly, instead of new one.

Check List

Tests

  • Unit test

@winoros winoros added the sig/planner SIG: Planner label Nov 12, 2019
Copy link
Contributor

@lzmhhh123 lzmhhh123 left a comment

Choose a reason for hiding this comment

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

It seems many explain tests need to change too.

@winoros winoros requested a review from a team as a code owner November 13, 2019 07:52
@ghost ghost requested review from wshwsh12 and XuHuaiyu and removed request for a team November 13, 2019 07:52
@winoros winoros requested review from alivxxx and lzmhhh123 and removed request for XuHuaiyu and wshwsh12 November 13, 2019 07:54
@codecov
Copy link

codecov bot commented Nov 13, 2019

Codecov Report

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

@@             Coverage Diff             @@
##             master     #13406   +/-   ##
===========================================
  Coverage   80.3292%   80.3292%           
===========================================
  Files           472        472           
  Lines        117311     117311           
===========================================
  Hits          94235      94235           
  Misses        15837      15837           
  Partials       7239       7239

Copy link
Member

@francis0407 francis0407 left a comment

Choose a reason for hiding this comment

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

Please fix the conflicts, and the rest LGTM.

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

@francis0407
Copy link
Member

/run-unit-test

@@ -84,10 +73,7 @@ func exprHasSetVarOrSleep(expr expression.Expression) bool {
// If any expression has SetVar function or Sleep function, we do not prune it.
func (p *LogicalProjection) PruneColumns(parentUsedCols []*expression.Column) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems PruneColumns don't need to return error.

Copy link
Member Author

Choose a reason for hiding this comment

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

There's a error check in Aggregation's PruneColumns. In line 113.

lzmhhh123
lzmhhh123 previously approved these changes Nov 18, 2019
Copy link
Contributor

@lzmhhh123 lzmhhh123 left a comment

Choose a reason for hiding this comment

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

LGTM

@lzmhhh123 lzmhhh123 added status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. labels Nov 18, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Nov 18, 2019

@winoros merge failed.

@winoros
Copy link
Member Author

winoros commented Nov 18, 2019

/run-all-tests

@winoros
Copy link
Member Author

winoros commented Nov 18, 2019

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

@winoros
Copy link
Member Author

winoros commented Nov 18, 2019

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

@winoros
Copy link
Member Author

winoros commented Nov 19, 2019

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

1 similar comment
@winoros
Copy link
Member Author

winoros commented Nov 19, 2019

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

@winoros
Copy link
Member Author

winoros commented Nov 20, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sig/planner SIG: Planner status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants