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: forbid limit to push across selection. #2793

Merged
merged 2 commits into from
Mar 8, 2017
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 1 addition & 4 deletions plan/match_property.go
Original file line number Diff line number Diff line change
Expand Up @@ -231,10 +231,7 @@ func (p *Selection) matchProperty(prop *requiredProperty, childPlanInfo ...*phys
res.count = uint64(float64(res.count) * selectionFactor)
return res
}
np := *p
np.SetChildren(childPlanInfo[0].p)
count := uint64(float64(childPlanInfo[0].count) * selectionFactor)
return &physicalPlanInfo{p: &np, cost: childPlanInfo[0].cost, count: count}
return childPlanInfo[0]
Copy link
Member

Choose a reason for hiding this comment

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

Here the childPlanInfo is actually not child, it is the selection itself?
Then the matchProperty is not used as it is supposed to be.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, in case of not on table, it's the selection itself.

}

// matchProperty implements PhysicalPlan matchProperty interface.
Expand Down
35 changes: 24 additions & 11 deletions plan/physical_plan_builder.go
Original file line number Diff line number Diff line change
Expand Up @@ -807,20 +807,21 @@ func (p *Selection) convert2PhysicalPlan(prop *requiredProperty) (*physicalPlanI
info = infoEnforce
}
}
if ds, ok := p.children[0].(*DataSource); !ok {
info = p.matchProperty(prop, info)
} else {
client := p.ctx.GetClient()
memDB := infoschema.IsMemoryDB(ds.DBName.L)
isDistReq := !memDB && client != nil && client.SupportRequestType(kv.ReqTypeSelect, 0)
if !isDistReq {
info = p.matchProperty(prop, info)
}
}
info = p.matchProperty(prop, info)
p.storePlanInfo(prop, info)
return info, nil
}

func (p *Selection) appendSelToInfo(info *physicalPlanInfo) *physicalPlanInfo {
np := *p
np.SetChildren(info.p)
return &physicalPlanInfo{
p: &np,
cost: info.cost,
count: uint64(float64(info.count) * selectionFactor),
}
}

func (p *Selection) convert2PhysicalPlanPushOrder(prop *requiredProperty) (*physicalPlanInfo, error) {
child := p.children[0].(LogicalPlan)
limit := prop.limit
Expand All @@ -838,7 +839,17 @@ func (p *Selection) convert2PhysicalPlanPushOrder(prop *requiredProperty) (*phys
info = enforceProperty(&requiredProperty{limit: limit}, info)
}
} else {
info = enforceProperty(&requiredProperty{limit: limit}, info)
info = enforceProperty(&requiredProperty{limit: limit}, p.appendSelToInfo(info))
}
} else if ds, ok := p.children[0].(*DataSource); !ok {
// TODO: It's tooooooo tricky here, we will remove all these logic after implementing DAG push down.
info = p.appendSelToInfo(info)
} else {
client := p.ctx.GetClient()
memDB := infoschema.IsMemoryDB(ds.DBName.L)
isDistReq := !memDB && client != nil && client.SupportRequestType(kv.ReqTypeSelect, 0)
if !isDistReq {
info = p.appendSelToInfo(info)
}
}
return info, nil
Expand All @@ -855,6 +866,8 @@ func (p *Selection) convert2PhysicalPlanEnforce(prop *requiredProperty) (*physic
if prop.limit != nil && len(prop.props) > 0 {
if t, ok := info.p.(physicalDistSQLPlan); ok {
t.addTopN(p.ctx, prop)
} else if _, ok := info.p.(*Selection); !ok {
Copy link
Member

Choose a reason for hiding this comment

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

If the child info is *Selection, the Selection p will be lost?

Copy link
Member Author

Choose a reason for hiding this comment

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

If child is selection, it means the selection has been processed

Copy link
Member

@coocood coocood Mar 8, 2017

Choose a reason for hiding this comment

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

Oh, I remembered, the child has pulled the parent Selection down.
This design is anti-pattern.

info = p.appendSelToInfo(info)
}
info = enforceProperty(prop, info)
} else if len(prop.props) != 0 {
Expand Down
8 changes: 8 additions & 0 deletions plan/physical_plan_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -417,6 +417,14 @@ func (s *testPlanSuite) TestCBO(c *C) {
sql: "select sum(a.b), sum(b.b) from t a join t b on a.c = b.c group by a.d order by a.d",
best: "LeftHashJoin{Table(t)->Table(t)}(a.c,b.c)->HashAgg->Sort->Trim",
},
{
sql: "select * from t t1 left outer join t t2 on true where least(1,2,3,t1.a,t2.b) > 0 order by t2.a limit 10",
best: "LeftHashJoin{Table(t)->Table(t)}->Selection->Sort + Limit(10) + Offset(0)",
},
{
sql: "select * from t t1 left outer join t t2 on true where least(1,2,3,t1.a,t2.b) > 0 limit 10",
best: "LeftHashJoin{Table(t)->Table(t)}->Selection->Limit",
},
{
sql: "select count(*) from t where concat(a,b) = 'abc' group by c",
best: "Index(t.c_d_e)[[<nil>,+inf]]->Selection->StreamAgg",
Expand Down
13 changes: 13 additions & 0 deletions plan/plan.go
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,19 @@ func (p *requiredProperty) getHashKey() ([]byte, error) {
return bytes, errors.Trace(err)
}

// String implements fmt.Stringer interface. Just for test.
func (p *requiredProperty) String() string {
ret := "Prop{"
for _, colProp := range p.props {
ret += fmt.Sprintf("col: %s, desc %v, ", colProp.col, colProp.desc)
}
ret += fmt.Sprintf("}, Len: %d", p.sortKeyLen)
if p.limit != nil {
ret += fmt.Sprintf(", Limit: %d,%d", p.limit.Offset, p.limit.Count)
}
return ret
}

type physicalPlanInfo struct {
p PhysicalPlan
cost float64
Expand Down