Skip to content

planner, expression: support predicate pushdown for CTE#28204

Closed
wjhuang2016 wants to merge 5 commits intopingcap:masterfrom
wjhuang2016:ppd_cte
Closed

planner, expression: support predicate pushdown for CTE#28204
wjhuang2016 wants to merge 5 commits intopingcap:masterfrom
wjhuang2016:ppd_cte

Conversation

@wjhuang2016
Copy link
Member

Signed-off-by: wjhuang2016 huangwenjun1997@gmail.com

What problem does this PR solve?

Issue Number: close #28163

Problem Summary:

What is changed and how it works?

Proposal: xxx

What's Changed:

How it Works:

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No code

Side effects

  • Performance regression: Consumes more CPU
  • Performance regression: Consumes more Memory
  • Breaking backward compatibility

Documentation

  • Affects user behaviors
  • Contains syntax changes
  • Contains variable changes
  • Contains experimental features
  • Changes MySQL compatibility

Release note

'None'

1
Signed-off-by: wjhuang2016 <huangwenjun1997@gmail.com>
@ti-chi-bot
Copy link
Member

[REVIEW NOTIFICATION]

This pull request has not been approved.

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Details

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

@ti-chi-bot ti-chi-bot added release-note-none Denotes a PR that doesn't merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 18, 2021
@wjhuang2016 wjhuang2016 added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 18, 2021
}

// ExtractColumnSetSimple extracts the different values of `UniqueId` for columns in expressions.
func ExtractColumnSetSimple(expr Expression) *intsets.Sparse {
Copy link
Member

Choose a reason for hiding this comment

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

You can change the �ExtractColumnSet's signature to ExtractColumnSet(exprs ...Expression).
Then you can easily call it by ExtractColumnSet(a), ExtractColumnSet(a, b), Extract(exprSlice...).

This way we don't need two functions.

Signed-off-by: wjhuang2016 <huangwenjun1997@gmail.com>
}
var p LogicalPlan
lp := LogicalCTE{cteAsName: tn.Name, cte: cte.cteClass, seedStat: cte.seedStat}.Init(b.ctx, b.getSelectOffset())
prevSchema := cte.seedLP.Schema().Clone()
Copy link
Member

Choose a reason for hiding this comment

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

Why we clone it here?

if p.cte.seedPartPhysicalPlan == nil {
// Build push-downed predicates.
if len(p.cte.pushDownPredicates) > 0 && p.cte.recursivePartLogicalPlan == nil {
selectedPushDownPredicates := make([]expression.Expression, 0)
Copy link
Member

Choose a reason for hiding this comment

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

Why allocate zero space?

selectedPushDownPredicates := make([]expression.Expression, 0)
minColSet := expression.IntersectColumnSet(p.cte.pushDownPredicates)
for _, cond := range p.cte.pushDownPredicates {
cnfItems := expression.SplitCNFItems(cond)
Copy link
Member

Choose a reason for hiding this comment

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

The pushDownPredicates should already be in CNF form. Thus each of its items should be a DNF case. Why do we call SplitCNFItems here?


var err error
if p.cte.seedPartPhysicalPlan == nil {
// Build push-downed predicates.
Copy link
Member

Choose a reason for hiding this comment

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

We can add more comments to illustrate the codes here.

@ti-chi-bot ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 26, 2021
@ti-chi-bot ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 6, 2022
Signed-off-by: wjhuang2016 <huangwenjun1997@gmail.com>
@wjhuang2016 wjhuang2016 deleted the ppd_cte branch November 17, 2022 11:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. release-note-none Denotes a PR that doesn't merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add Join Predicate Pushdown in CTE to improve the performance and reduce OOM in TiDB

3 participants