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: Solve the problem that the parent query condition is pushed incorrectly when the setvar function is evaluated in the subquery.（#11622） #11624

Merged
merged 6 commits into from
Sep 5, 2019
Merged

planner: Solve the problem that the parent query condition is pushed incorrectly when the setvar function is evaluated in the subquery.（#11622） #11624

merged 6 commits into from
Sep 5, 2019

Conversation

lovewin99
Copy link
Contributor

What problem does this PR solve?

fix issue: #11622 when the setvar function is evaluated in the subquery, the parent query condition is pushed down wrongly.

Check List

Tests

  • Integration test

@sre-bot
Copy link
Contributor

sre-bot commented Aug 5, 2019

Hi contributor, thanks for your PR.

This patch needs to be approved by someone of admins. They should reply with "/ok-to-test" to accept this PR for running test automatically.

@sre-bot sre-bot added the contribution This PR is from a community contributor. label Aug 5, 2019
@zz-jason zz-jason added sig/planner SIG: Planner type/bugfix This PR fixes a bug. labels Aug 7, 2019
qw4990
qw4990 reviewed Aug 14, 2019
View reviewed changes
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.

I tested this case on my computer with MySQL 8.0, and it returns an empty set.

@lovewin99
Copy link
Contributor Author

I tested this case on my computer with MySQL 8.0, and it returns an empty set.

I'm so sorry for sql written incorrectly. I have modified sql in issue. PTAL @qw4990

eurekaka
eurekaka reviewed Aug 16, 2019
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.

@lovewin99 Please help fix the failed CI.

@codecov
Copy link

codecov bot commented Aug 16, 2019 •
edited
Loading

Codecov Report

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

@@             Coverage Diff             @@
##             master     #11624   +/-   ##
===========================================
  Coverage   81.4629%   81.4629%           
===========================================
  Files           447        447           
  Lines         96191      96191           
===========================================
  Hits          78360      78360           
  Misses        12286      12286           
  Partials       5545       5545

@lovewin99
Copy link
Contributor Author

/run-all-tests

@lovewin99
Copy link
Contributor Author

@eurekaka All tests passed

eurekaka
eurekaka reviewed Aug 16, 2019
View reviewed changes
planner/core/rule_predicate_push_down.go Outdated Show resolved Hide resolved
expression/chunk_executor.go
@@ -53,6 +53,27 @@ func HasGetSetVarFunc(expr Expression) bool {
return false
}

// HasAssignSetVarFunc checks whether an expression contains SetVar function and assign a value
func HasAssignSetVarFunc(expr Expression) bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

This check looks pretty ad-hoc, I prefer to push no condition down to Projection's child completely if p.Exprs contains SetVar or GetVar, because there seems like many hidden corner cases which would return wrong results. @zz-jason How do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If p.Exprs contains setvar and getvar and does not push down all the conditions, then many normal pushdowns will be ignored, which can have a large impact on performance in many cases. I think this kind of check just a subset of reject all pushdowns, which helps narrow down the problem without expanding the problem. @eurekaka @zz-jason How do you think?

eurekaka
eurekaka reviewed Aug 23, 2019
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 the status/LGT1 Indicates that a PR has LGTM 1. label Aug 23, 2019
@lovewin99
Copy link
Contributor Author

PTAL @qw4990

@eurekaka eurekaka requested a review from qw4990 September 3, 2019 07:14
@eurekaka
Copy link
Contributor

eurekaka commented Sep 3, 2019

@lovewin99 Please help resolve the conflict, thanks.

qw4990
qw4990 approved these changes Sep 5, 2019
View reviewed changes
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

@qw4990 qw4990 added status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Sep 5, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Sep 5, 2019

/run-all-tests

@sre-bot sre-bot merged commit 5199e21 into pingcap:master Sep 5, 2019
@lovewin99 lovewin99 deleted the fix-issue branch September 5, 2019 16:18
@cyliu0
Copy link
Contributor

cyliu0 commented Nov 6, 2019

@lovewin99 We may need to cherry pick this one to release-3.1, release-3.0, release-2.1

lovewin99 added a commit to lovewin99/tidb that referenced this pull request Nov 7, 2019
@lovewin99
planner: Solve the problem that the parent query condition is pushed …
0b1ac97 
…incorrectly when the setvar function is evaluated in the subquery.(pingcap#11624)
@lovewin99 lovewin99 mentioned this pull request Nov 7, 2019
Merged
lovewin99 added a commit to lovewin99/tidb that referenced this pull request Nov 7, 2019
@lovewin99
planner: Solve the problem that the parent query condition is pushed …
87f9780 
…incorrectly when the setvar function is evaluated in the subquery.(pingcap#11624)
@lovewin99 lovewin99 mentioned this pull request Nov 7, 2019
Merged
lovewin99 added a commit to lovewin99/tidb that referenced this pull request Nov 7, 2019
@lovewin99
planner: Solve the problem that the parent query condition is pushed …
776a0c3 
…incorrectly when the setvar function is evaluated in the subquery.(pingcap#11624)
@lovewin99 lovewin99 mentioned this pull request Nov 7, 2019
Merged
ngaut pushed a commit that referenced this pull request Nov 13, 2019
@lovewin99 @ngaut
planner: Solve the problem that the parent query condition is pushed …
75b546a 
…incorrectly when the setvar function is evaluated in the subquery.(#11624) (#13235)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eurekaka eurekaka eurekaka left review comments

@qw4990 qw4990 qw4990 approved these changes

Assignees

@eurekaka eurekaka

Labels
contribution This PR is from a community contributor. 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. type/bugfix This PR fixes a bug.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@lovewin99 @sre-bot @eurekaka @cyliu0 @qw4990 @zz-jason