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

plan: always chose the smaller child as outer for IndexJoin #6996

Closed
wants to merge 3 commits into from

Conversation

zz-jason
Copy link
Member

@zz-jason zz-jason commented Jul 5, 2018

What have you changed? (mandatory)

Always chose the smaller child as the outer one for IndexJoin if possible.

What are the type of the changes (mandatory)?

  • Improvement (non-breaking change which is an improvement to an existing feature)

How has this PR been tested (mandatory)?

  • unit test
  • explain test

Copy link
Member

@winoros winoros left a comment

Choose a reason for hiding this comment

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

LGTM

@winoros winoros added status/LGT1 Indicates that a PR has LGTM 1. sig/planner SIG: Planner labels Jul 5, 2018
plan/plan.go Outdated
@@ -320,6 +323,13 @@ type basePlan struct {
stats *statsInfo
}

func (p *basePlan) cardinality() float64 {
if p.stats == nil {
return math.MaxFloat64
Copy link
Member

Choose a reason for hiding this comment

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

Why return MaxFloat64 here?

@@ -163,6 +164,8 @@ type LogicalPlan interface {
// deriveStats derives statistic info between plans.
deriveStats() (*statsInfo, error)

cardinality() float64
Copy link
Member

Choose a reason for hiding this comment

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

Add comment for it.

@@ -385,22 +385,30 @@ func (p *LogicalJoin) tryToGetIndexJoin(prop *requiredProp) ([]PhysicalPlan, boo
plans = append(plans, join...)
}
case InnerJoin:
join := p.getIndexJoinByOuterIdx(prop, 0)
if join != nil {
lhsCardinality := p.Children()[0].cardinality()
Copy link
Member

Choose a reason for hiding this comment

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

Can we move this to line 402?

if p.stats == nil {
return math.MaxFloat64
}
return p.stats.count
Copy link
Contributor

Choose a reason for hiding this comment

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

It is not cardinality, cardinality is the number of elements in a set.

@@ -163,6 +164,8 @@ type LogicalPlan interface {
// deriveStats derives statistic info between plans.
deriveStats() (*statsInfo, error)

cardinality() float64
Copy link
Member

Choose a reason for hiding this comment

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

Can we add getStats instead?
Then use the count to determine the outer table.

@zz-jason zz-jason closed this Jul 30, 2018
@zz-jason zz-jason deleted the dev/2.0-indexjoin branch October 24, 2018 11:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sig/planner SIG: Planner status/LGT1 Indicates that a PR has LGTM 1.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants