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

plan: merge continuous selections and delete surely true expressions #24214

Merged
merged 21 commits into from
May 10, 2021
Merged

plan: merge continuous selections and delete surely true expressions #24214

merged 21 commits into from
May 10, 2021

Conversation

fzhedu
Copy link
Contributor

@fzhedu fzhedu commented Apr 22, 2021 •
edited
Loading

What problem does this PR solve?

Issue Number: close #24213, pingcap/tiflash#1731

Problem Summary:

What is changed and how it works?

Proposal: xxx

What's Changed:

some selections may become continuous after some optimizations like eliminating projections. Continuous selections are not supported in TiFlash. From the performance perspective, it is better to merge continuous selections to reduce compution.

on the other hand, we delete surely true expressions during pushing down expressions.

How it Works:
do the post optimization to merge continuous selections, and delete surely true expressions.

Related changes

  • Need to cherry-pick to the release branch

Check List

Tests

  • Integration test

Side effects

  • Performance regression
    • Consumes more CPU

Release note

  • No release note

@fzhedu fzhedu requested a review from a team as a code owner April 22, 2021 13:48
@fzhedu fzhedu requested review from qw4990 and removed request for a team April 22, 2021 13:48
@ti-chi-bot ti-chi-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 22, 2021
@fzhedu fzhedu added sig/planner SIG: Planner type/bugfix This PR fixes a bug. labels Apr 22, 2021
fzhedu
fzhedu commented Apr 22, 2021
View reviewed changes
planner/core/testdata/integration_serial_suite_out.json Outdated
" └─HashJoin 9990.00 cop[tiflash] inner join, equal:[eq(test.ts.col_varchar_64, test.ts.col_varchar_key)]",
" ├─ExchangeReceiver(Build) 7992.00 cop[tiflash] ",
" │ └─ExchangeSender 7992.00 cop[tiflash] ExchangeType: Broadcast",
" │ └─Selection 7992.00 cop[tiflash] 1, not(isnull(test.ts.col_varchar_key))",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

otherwise, there are two continuous selections.

@fzhedu
Copy link
Contributor Author

fzhedu commented Apr 22, 2021

/run-check_dev_2

@fzhedu
Copy link
Contributor Author

fzhedu commented Apr 22, 2021

/run-unit-test

@zhouqiang-cl
Copy link
Contributor

/run-monitor-test

@fzhedu fzhedu requested a review from a team as a code owner April 23, 2021 04:35
@dianqihanwangzi
Copy link
Contributor

/run-monitor-test

@sre-bot
Copy link
Contributor

sre-bot commented Apr 23, 2021

Visit the grafana server at: http://172.16.5.5:32348, it will last for 5 hours

@fzhedu fzhedu changed the title plan: merge continuous selections plan: merge continuous selections and delete surely true expressions Apr 25, 2021
winoros
winoros reviewed Apr 28, 2021
View reviewed changes
planner/core/optimizer.go
@@ -156,9 +157,34 @@ func DoOptimize(ctx context.Context, sctx sessionctx.Context, flag uint64, logic
return finalPlan, cost, nil
}

// mergeContinuousSelections merge continuous selections which may occur after changing plans.
func mergeContinuousSelections(p PhysicalPlan) {
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 need this in the physical phase?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Taking the issue as an example, a projection is eleminated in the postOptimize, then two selections become continuous, so we should merge at the last of postOptimize.

@winoros
Copy link
Member

winoros commented Apr 28, 2021

I'm a little curious about the selection(true) in the mpp plan. It's very rare in my mind. How would it be generated?

The continuous selections should be pruned in predicate push down

@fzhedu
Copy link
Contributor Author

fzhedu commented Apr 30, 2021

I'm a little curious about the selection(true) in the mpp plan. It's very rare in my mind. How would it be generated?

The continuous selections should be pruned in predicate push down

For the case in the issue, the selection(true) comes from pushing down predicates in projection. Adding selection(true) also exist in pointGet. Related cases are in the changed files.

@ti-chi-bot ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 30, 2021
@fzhedu fzhedu requested a review from winoros April 30, 2021 08:45
winoros
winoros reviewed May 6, 2021
View reviewed changes
planner/core/rule_predicate_push_down.go
@@ -73,6 +79,8 @@ func splitSetGetVarFunc(filters []expression.Expression) ([]expression.Expressio

// PredicatePushDown implements LogicalPlan PredicatePushDown interface.
func (p *LogicalSelection) PredicatePushDown(predicates []expression.Expression) ([]expression.Expression, LogicalPlan) {
predicates = DeleteTrueExprs(p, predicates)
Copy link
Member

Choose a reason for hiding this comment

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

It's a little redundant to remove it every time by checking the predicates passed down.
We could only do it for selection and join since they are generated the ones to be passed to their children

Copy link
Contributor Author

Choose a reason for hiding this comment

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

en, we delete that in func (p *LogicalUnionScan) PredicatePushDown, but that in func (ds *DataSource) PredicatePushDown( is needed, because PropagateConstant may introduce some surely true expressions.

@ti-chi-bot ti-chi-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 6, 2021
@purelind
Copy link
Contributor

/run-common-test

@purelind
Copy link
Contributor

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

@fzhedu
Copy link
Contributor Author

fzhedu commented May 10, 2021

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

@fzhedu
Copy link
Contributor Author

fzhedu commented May 10, 2021

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

@fzhedu
Copy link
Contributor Author

fzhedu commented May 10, 2021

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

@fzhedu
Copy link
Contributor Author

fzhedu commented May 10, 2021

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

@fzhedu
Copy link
Contributor Author

fzhedu commented May 10, 2021

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

@fzhedu
Copy link
Contributor Author

fzhedu commented May 10, 2021

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

@fzhedu
Copy link
Contributor Author

fzhedu commented May 10, 2021

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

@fzhedu
Copy link
Contributor Author

fzhedu commented May 10, 2021

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

@fzhedu
Copy link
Contributor Author

fzhedu commented May 10, 2021

/merge

@ti-chi-bot
Copy link
Member

This pull request has been accepted and is ready to merge.

Commit hash: c3003a5

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label May 10, 2021
@fzhedu
Copy link
Contributor Author

fzhedu commented May 10, 2021

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

@fzhedu
Copy link
Contributor Author

fzhedu commented May 10, 2021

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

@fzhedu
Copy link
Contributor Author

fzhedu commented May 10, 2021

/run-integration-ddl-test

@fzhedu
Copy link
Contributor Author

fzhedu commented May 10, 2021

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

@fzhedu
Copy link
Contributor Author

fzhedu commented May 10, 2021

/run-integration-common-test --tidb-test=pr/1187
/run-common-test --tidb-test=pr/1187
/run-integration-ddl-test

@ti-chi-bot ti-chi-bot merged commit f135c53 into pingcap:master May 10, 2021
ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request May 10, 2021
@fzhedu @ti-srebot
cherry pick pingcap#24214 to release-5.0
fe7ca42 
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot ti-srebot mentioned this pull request May 10, 2021
Merged
@ti-srebot
Copy link
Contributor

cherry pick to release-5.0 in PR #24533

ti-chi-bot pushed a commit that referenced this pull request May 12, 2021
@ti-srebot
plan: merge continuous selections and delete surely true expressions (#…
41c0f17 
…24214) (#24533)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@winoros winoros winoros approved these changes

@rebelice rebelice rebelice approved these changes

@qw4990 qw4990 Awaiting requested review from qw4990

@eurekaka eurekaka Awaiting requested review from eurekaka

@hanfei1991 hanfei1991 Awaiting requested review from hanfei1991

Assignees
No one assigned
Labels
component/expression needs-cherry-pick-release-5.0 sig/execution SIG execution sig/planner SIG: Planner size/L Denotes a PR that changes 100-499 lines, ignoring generated files. 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.

DB::TiFlashException: Duplicated selection in DAG request
10 participants
@fzhedu @zhouqiang-cl @dianqihanwangzi @sre-bot @winoros @ti-chi-bot @lzmhhh123 @purelind @ti-srebot @rebelice