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

expression: fix constItem() and column-pruning in order by clause #11839

Merged
merged 7 commits into from
Aug 28, 2019

Conversation

wjhuang2016
Copy link
Member

What problem does this PR solve?

#10064 Change the semantics of constItem().
A const item can be eval() when build a plan, and it shouldn't contain correlated column.

Column-pruning in order by clause, however, need to identify a "const item" in execute stage. And IsMutableEffectsExpr should be enough.

What is changed and how it works?

Change back constItem() and use IsMutableEffectsExpr. And export isMutableEffectsExpr as IsMutableEffectsExpr

Check List

Tests

Code changes

  • Has exported function/method change

Side effects

Related changes

  • Need to cherry-pick to the release branch

@codecov
Copy link

codecov bot commented Aug 23, 2019

Codecov Report

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

@@             Coverage Diff             @@
##             master     #11839   +/-   ##
===========================================
  Coverage   81.2937%   81.2937%           
===========================================
  Files           438        438           
  Lines         94578      94578           
===========================================
  Hits          76886      76886           
  Misses        12217      12217           
  Partials       5475       5475

@wjhuang2016
Copy link
Member Author

@lysu @zz-jason PTAL

@qw4990
Copy link
Contributor

qw4990 commented Aug 23, 2019

/run-common-test

Copy link
Contributor

@qw4990 qw4990 left a comment

Choose a reason for hiding this comment

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

LGTM

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.

Can you add an explain test for this PR? thanks

@wjhuang2016
Copy link
Member Author

Can you add an explain test for this PR? thanks

Done

@wjhuang2016
Copy link
Member Author

/run-all-tests

executor/sort_test.go Outdated Show resolved Hide resolved
executor/explain_test.go Outdated Show resolved Hide resolved
@eurekaka eurekaka added contribution This PR is from a community contributor. and removed status/all tests passed labels Aug 28, 2019
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/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. labels Aug 28, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Aug 28, 2019

/run-all-tests

@sre-bot sre-bot merged commit c599333 into pingcap:master Aug 28, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Aug 28, 2019

cherry pick to release-2.1 failed

@sre-bot
Copy link
Contributor

sre-bot commented Apr 7, 2020

It seems that, not for sure, we failed to cherry-pick this commit to release-2.1. Please comment '/run-cherry-picker' to try to trigger the cherry-picker if we did fail to cherry-pick this commit before. @wjhuang2016 PTAL.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/expression contribution This PR is from a community contributor. 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