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/core: use constant propagate before predicates push down #21061

Merged
merged 24 commits into from
Dec 8, 2020
Merged

planner/core: use constant propagate before predicates push down #21061

merged 24 commits into from
Dec 8, 2020

Conversation

ou-bing
Copy link
Contributor

@ou-bing ou-bing commented Nov 15, 2020 •
edited
Loading

What problem does this PR solve?

Issue Number: close #20139 #14481

Problem Summary:

What is changed and how it works?

use constant propagate before range partition pruning

Check List

Tests

  • Integration test

Release note

  • use constant propagate before range partition pruning

@ou-bing ou-bing requested a review from a team as a code owner November 15, 2020 10:08
@ou-bing ou-bing requested review from SunRunAway and removed request for a team November 15, 2020 10:08
@ichn-hu ichn-hu mentioned this pull request Nov 15, 2020
Closed
@XuHuaiyu
Copy link
Contributor

Please add some test case for this commit

@ou-bing
Copy link
Contributor Author

ou-bing commented Nov 16, 2020

Please add some test case for this commit

I still have some problems unsolved，And then I'll add the test cases😂.

@XuHuaiyu XuHuaiyu added sig/planner SIG: Planner type/bugfix This PR fixes a bug. labels Nov 16, 2020
@ou-bing ou-bing requested a review from a team as a code owner November 17, 2020 15:22
@ou-bing ou-bing changed the title planner/core: use constant propagate before range partition pruning planner/core: use constant propagate before predicates push down Nov 19, 2020
@ti-srebot
Copy link
Contributor

@ou-bing merge failed.

@XuHuaiyu
Copy link
Contributor

XuHuaiyu commented Dec 3, 2020

/merge

@ti-srebot
Copy link
Contributor

Your auto merge job has been accepted, waiting for:

  • 21411
  • 21469
  • 21165
  • 21411
  • 20836
  • 21409
  • 19820
  • 21469

@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

@ou-bing merge failed.

@XuHuaiyu
Copy link
Contributor

XuHuaiyu commented Dec 4, 2020

@winoros Is the test failed because of this commit?

tiancaiamao
tiancaiamao reviewed Dec 4, 2020
View reviewed changes
Copy link
Contributor

@tiancaiamao tiancaiamao left a comment

Choose a reason for hiding this comment

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

@winoros Is the test failed because of this commit?

MySQL test fails because of this change, some test cases may need to update.
I'll take a look @XuHuaiyu

planner/core/rule_predicate_push_down.go
@@ -99,6 +99,7 @@ func (p *LogicalUnionScan) PredicatePushDown(predicates []expression.Expression)

// PredicatePushDown implements LogicalPlan PredicatePushDown interface.
func (ds *DataSource) PredicatePushDown(predicates []expression.Expression) ([]expression.Expression, LogicalPlan) {
predicates = expression.PropagateConstant(ds.ctx, predicates)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we would do expression.PropagateConstant many times (much more than necessary).

@tiancaiamao
Copy link
Contributor

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

@tiancaiamao
Copy link
Contributor

/run-integration-common-test tidb-test=pr/1121

@XuHuaiyu
Copy link
Contributor

XuHuaiyu commented Dec 7, 2020

/merge

@ti-srebot
Copy link
Contributor

Your auto merge job has been accepted, waiting for:

  • 21409
  • 21469

@ti-srebot
Copy link
Contributor

/run-all-tests

@XuHuaiyu
Copy link
Contributor

XuHuaiyu commented Dec 8, 2020

/merge

@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

@ou-bing merge failed.

@XuHuaiyu
Copy link
Contributor

XuHuaiyu commented Dec 8, 2020

/run-integration-common-test tidb-test=pr/1121
/run-common-test tidb-test=pr/1121

@XuHuaiyu
Copy link
Contributor

XuHuaiyu commented Dec 8, 2020

/run-integration-common-test tidb-test=pr/1121

@XuHuaiyu XuHuaiyu merged commit 03f385d into pingcap:master Dec 8, 2020
@wshwsh12 wshwsh12 mentioned this pull request Dec 9, 2020
Closed
@you06
Copy link
Contributor

you06 commented Dec 17, 2020

Seems this PR fixed the issue for point get with impossible condition still locking row #21688, this issue still exist in release-4.0.
@XuHuaiyu Do we need to cherry pick this PR to release-4.0?

@XuHuaiyu
Copy link
Contributor

@you06 I think we can cherrypick this.
@qw4990 What do you think?

@you06 you06 mentioned this pull request Dec 17, 2020
Open
@wshwsh12 wshwsh12 mentioned this pull request Dec 18, 2020
Closed
@XuHuaiyu XuHuaiyu mentioned this pull request Dec 21, 2020
Closed
@time-and-fate time-and-fate mentioned this pull request Mar 4, 2022
Merged
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Reminiscent Reminiscent Reminiscent left review comments

@lzmhhh123 lzmhhh123 lzmhhh123 left review comments

@winoros winoros winoros left review comments

@tiancaiamao tiancaiamao tiancaiamao left review comments

@ti-srebot ti-srebot ti-srebot approved these changes

@SunRunAway SunRunAway Awaiting requested review from SunRunAway

Assignees
No one assigned
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. type/bugfix This PR fixes a bug.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

constant propagate is not done before range partition pruning
8 participants
@ou-bing @XuHuaiyu @lzmhhh123 @ti-srebot @tiancaiamao @you06 @winoros @Reminiscent