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

planner/core: simple greedy join reorder based on CBO #8394

Merged
merged 11 commits into from Dec 7, 2018

Conversation

Projects
None yet
4 participants
@winoros
Copy link
Member

winoros commented Nov 22, 2018

What problem does this PR solve?

Let the default join reorder by greedy use CBO.

What is changed and how it works?

Removed old one, add the new greedy algo.

Check List

Tests

  • Unit test
  • Integration test
  • Manual test: TPCH performance checking.

Side effects

No

Related changes

  • Need to be included in the release note. Let default join reorder use CBO.

This change is Reviewable

@winoros

This comment has been minimized.

Copy link
Member Author

winoros commented Nov 22, 2018

TPCH
Q5: cannot get result -> 55s
Q8: 46s->11s
Q17 66s->105s
Q18 80s->90s
Q21: 1min24s->24s

Need some investigation on Q17 and Q18.

@ngaut

This comment has been minimized.

Copy link
Member

ngaut commented Nov 22, 2018

What's the results of star benchmark?

@winoros

This comment has been minimized.

Copy link
Member Author

winoros commented Nov 22, 2018

@ngaut TiDB haven't tested SSB yet. It will takes some time to do it.

@winoros

This comment has been minimized.

Copy link
Member Author

winoros commented Nov 22, 2018

Update for TPCH Q17:
Original join order is Join{Join{DataSource[2K rows], DataSource[60M rows]}[6w rows], Agg[2M rows]}[5K rows]

New order is Join{Join{DataSource[2K rows], Agg[2M rows]}[5K rows], DataSource[60M rows]}[5K rows]

You cannot say that the new order(don't care about the actual operator node below) is worse than before.
The problem is that the old one can let agg and inner join execute at the same time. While the new one will let the inner join waiting for agg's execution. And after that, begin to join with the 60M rows' DataSource. So it loses.

This can be optimized i think. But will leave it for future pr to do.

@zz-jason

This comment has been minimized.

Copy link
Member

zz-jason commented Nov 22, 2018

The problem here is we haven't consider the physical cost of the join order, only the number of rows of the intermediate result are considered in this greedy algorithm.

@eurekaka
Copy link
Contributor

eurekaka left a comment

LGTM

Show resolved Hide resolved planner/core/rule_join_reorder_dp.go Outdated
Show resolved Hide resolved planner/core/rule_join_reorder_dp.go Outdated
)

func extractInnerJoinGroup(p *LogicalJoin) ([]LogicalPlan, []*expression.ScalarFunction, []expression.Expression) {
if p.reordered || p.preferJoinType > uint(0) || p.JoinType != InnerJoin || p.StraightJoin {

This comment has been minimized.

Copy link
@zz-jason

zz-jason Nov 27, 2018

Member

Why stop extracting inner join groups when p.preferJoinType > uint(0)? IMHO, It's only a hint about join algorithm, not the join order.

This comment has been minimized.

Copy link
@winoros

winoros Nov 29, 2018

Author Member

Emm, we can do this.

This comment has been minimized.

Copy link
@zz-jason

zz-jason Dec 6, 2018

Member

OK. Could you please update code to remove this check in this function?

This comment has been minimized.

Copy link
@winoros

winoros Dec 6, 2018

Author Member

Oh, when do join reorder, we create new join node. And when creating it. We missed the join hint information. We need to change the way we hold it before we remove this check.

Show resolved Hide resolved planner/core/rule_join_reorder_greedy.go Outdated
Show resolved Hide resolved planner/core/rule_join_reorder_greedy.go Outdated
Show resolved Hide resolved planner/core/rule_join_reorder_greedy.go
Show resolved Hide resolved planner/core/rule_join_reorder_greedy.go
Show resolved Hide resolved planner/core/rule_join_reorder_greedy.go Outdated
Show resolved Hide resolved planner/core/rule_join_reorder_greedy.go Outdated
Show resolved Hide resolved planner/core/rule_join_reorder_greedy.go Outdated
@winoros

This comment has been minimized.

Copy link
Member Author

winoros commented Nov 30, 2018

/run-all-tests tidb-test=pr/548

@zz-jason

This comment has been minimized.

Copy link
Member

zz-jason commented Dec 6, 2018

/run-all-tests

Show resolved Hide resolved planner/core/rule_join_reorder_dp.go Outdated
Show resolved Hide resolved planner/core/rule_join_reorder_dp.go Outdated
)

func extractInnerJoinGroup(p *LogicalJoin) ([]LogicalPlan, []*expression.ScalarFunction, []expression.Expression) {
if p.reordered || p.preferJoinType > uint(0) || p.JoinType != InnerJoin || p.StraightJoin {

This comment has been minimized.

Copy link
@zz-jason

zz-jason Dec 6, 2018

Member

OK. Could you please update code to remove this check in this function?

Show resolved Hide resolved planner/core/rule_join_reorder_greedy.go Outdated
Show resolved Hide resolved planner/core/rule_join_reorder_greedy.go
Show resolved Hide resolved planner/core/rule_join_reorder_greedy.go Outdated
Show resolved Hide resolved planner/core/rule_join_reorder_greedy.go
Show resolved Hide resolved planner/core/rule_join_reorder_greedy.go

winoros added some commits Dec 6, 2018

Show resolved Hide resolved planner/core/rule_join_reorder_greedy.go Outdated
Show resolved Hide resolved planner/core/rule_join_reorder_greedy.go
Show resolved Hide resolved planner/core/rule_join_reorder_greedy.go Outdated
Show resolved Hide resolved planner/core/rule_join_reorder_greedy.go Outdated
@zz-jason
Copy link
Member

zz-jason left a comment

LGTM

@zz-jason

This comment has been minimized.

Copy link
Member

zz-jason commented Dec 7, 2018

/run-all-tests

@zz-jason zz-jason added status/LGT2 and removed status/LGT1 labels Dec 7, 2018

@zz-jason

This comment has been minimized.

Copy link
Member

zz-jason commented Dec 7, 2018

/run-integration-common-test

@winoros winoros merged commit 6bd8bdd into pingcap:master Dec 7, 2018

11 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci-tidb/build Jenkins job succeeded.
Details
jenkins-ci-tidb/common-test Jenkins job succeeded.
Details
jenkins-ci-tidb/integration-common-test Jenkins job succeeded.
Details
jenkins-ci-tidb/integration-compatibility-test Jenkins job succeeded.
Details
jenkins-ci-tidb/integration-ddl-test Jenkins job succeeded.
Details
jenkins-ci-tidb/mybatis-test Jenkins job succeeded.
Details
jenkins-ci-tidb/sqllogic-test Jenkins job succeeded.
Details
jenkins-ci-tidb/unit-test Jenkins job succeeded.
Details
license/cla Contributor License Agreement is signed.
Details

@winoros winoros deleted the winoros:join-reorder-cbo-greedy branch Dec 7, 2018

iamzhoug37 added a commit to iamzhoug37/tidb that referenced this pull request Dec 13, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.