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: support topn push down in plan phase. #1769

Merged
merged 8 commits into from Oct 11, 2016

Conversation

Projects
None yet
5 participants
@hanfei1991
Copy link
Member

commented Sep 27, 2016

It's a additional pr for #1760
It tries to push topn to table scan and index scan.
@coocood @shenli @zimulala @tiancaiamao @XuHuaiyu PTAL

@hanfei1991 hanfei1991 changed the title support topn in plan phase. plan: support topn push down in plan phase. Sep 27, 2016

@coocood

This comment has been minimized.

Copy link
Member

commented Sep 27, 2016

Any test covers the case when selection condition can not be converted to PB, then topN will not be pushed?

@@ -114,6 +113,7 @@ func (p *physicalTableSource) addTopN(prop *requiredProperty) {
for _, item := range prop.props {
p.SortItems = append(p.SortItems, sortByItemToPB(p.client, item.col, item.desc))

This comment has been minimized.

Copy link
@coocood

coocood Sep 27, 2016

Member

sortByItem may return nil, then addTopN should return false.

@hanfei1991

This comment has been minimized.

Copy link
Member Author

commented Sep 27, 2016

@coocood PTAL

if prop.limit != nil {
count := int64(prop.limit.Count + prop.limit.Offset)
p.LimitCount = &count
if prop.limit == nil || len(prop.props) == 0 {

This comment has been minimized.

Copy link
@XuHuaiyu

XuHuaiyu Sep 28, 2016

Contributor

There may be no need to judge "prop.limit == nil", it has been promised before invoking this function.

This comment has been minimized.

Copy link
@hanfei1991

hanfei1991 Sep 28, 2016

Author Member

It's safer to check this again inside this function.

@@ -74,6 +72,7 @@ type physicalTableSource struct {
AggFuncs []*tipb.Expr
GbyItems []*tipb.ByItem

// ConditionPBExpr is the pb structure of conditions that pushed down.

This comment has been minimized.

Copy link
@XuHuaiyu

XuHuaiyu Sep 28, 2016

Contributor

pushed down --> be pushed down

@coocood

This comment has been minimized.

Copy link
Member

commented Sep 28, 2016

The added test case doesn't make much sense as the test use a mock context which returns a nil Client, and TopN is not supported by local store.
So TopN will never be pushed down.

@hanfei1991

This comment has been minimized.

Copy link
Member Author

commented Sep 29, 2016

@coocood PTAL

@hanfei1991 hanfei1991 force-pushed the hanfei/topn branch from 731e8ef to c0cd833 Sep 29, 2016

@@ -373,6 +535,10 @@ func (s *testPlanSuite) TestCBO(c *C) {
best: "Index(t.c_d_e)[[-inf,10000)]->Sort + Limit(2) + Offset(0)->Projection",
},
{
sql: "select * from t a where a.c < 10000 and a.d in (1000, a.e) order by a.a limit 2",

This comment has been minimized.

Copy link
@coocood

coocood Sep 29, 2016

Member

move this case to TestTopnPushDown and verify that top n is not pushed down.

This comment has been minimized.

Copy link
@shenli
@@ -106,21 +141,38 @@ func (is *PhysicalIndexScan) matchProperty(prop *requiredProperty, infos ...*phy
if allAsc {

This comment has been minimized.

Copy link
@XuHuaiyu

XuHuaiyu Sep 29, 2016

Contributor
if allAsc || allDesc{
  if allDesc {
    sortedIs.Desc = true
  }  
 // Duplicated code
}

would this be more brief

This comment has been minimized.

Copy link
@shenli
@XuHuaiyu

This comment has been minimized.

Copy link
Contributor

commented Sep 30, 2016

Please take a look at function handleLeftJoin/handleRightJoin in physical_plan_builder.go, the local variable allLeft/allRight been checked if false three times, but if allLeft/allRight is false, the function will be returned just in the first check, and there is no value write of allLeft/allRight after this check.

selIdxReq.OrderBy = append(selIdxReq.OrderBy, &tipb.ByItem{Desc: e.indexPlan.Desc})
if len(e.indexPlan.SortItems) > 0 {
selIdxReq.OrderBy = e.indexPlan.SortItems
} else if e.indexPlan.Desc {

This comment has been minimized.

Copy link
@shenli

shenli Sep 30, 2016

Member

How about e.indexPlan.SortItems > 0 and e.indexPlan.Desc is true?

This comment has been minimized.

Copy link
@shenli

This comment has been minimized.

Copy link
@hanfei1991

hanfei1991 Oct 8, 2016

Author Member

If e.indexPlan.SortItems > 0, we don't care if the table is desc scanned or not.

@@ -46,6 +50,10 @@ func (s *testPlanSuite) SetUpSuite(c *C) {
s.Parser = parser.New()
}

func newType() types.FieldType {

This comment has been minimized.

Copy link
@XuHuaiyu

XuHuaiyu Sep 30, 2016

Contributor

newTypeLong() be better?

if len(e.orderByList) > 0 {
selReq.OrderBy = e.orderByList
} else if e.supportDesc && e.desc {
selReq.OrderBy = []*tipb.ByItem{{Desc: e.desc}}

This comment has been minimized.

Copy link
@shenli

shenli Oct 8, 2016

Member

{{ -> {

This comment has been minimized.

Copy link
@shenli

shenli Oct 8, 2016

Member

When will e.desc is true and len(e.orderByList) == 0?

This comment has been minimized.

Copy link
@hanfei1991

hanfei1991 Oct 8, 2016

Author Member

If no topn is pushed, the order by list will be empty.

if len(prop.props) == 0 {
return enforceProperty(prop, &physicalPlanInfo{p: ts, cost: cost, count: infos[0].count})
p := tryToAddUnionScan(ts.txn, ts.conditions, ts)

This comment has been minimized.

Copy link
@shenli

shenli Oct 8, 2016

Member

Why add union scan here?

This comment has been minimized.

Copy link
@hanfei1991

hanfei1991 Oct 8, 2016

Author Member

"try to add union scan" will check if the txn is read-only. If it is, it will add union scan for it.

}
if len(prop.props) == 1 && ts.pkCol != nil && ts.pkCol == prop.props[0].col {
sortedTs := *ts
sortedTs.Desc = prop.props[0].desc
sortedTs.KeepOrder = true
p := tryToAddUnionScan(ts.txn, ts.conditions, &sortedTs)

This comment has been minimized.

Copy link
@shenli

shenli Oct 8, 2016

Member

Should we also addTopN in this condition?

This comment has been minimized.

Copy link
@hanfei1991

hanfei1991 Oct 8, 2016

Author Member

This condition means the pk column can match the required property.

@@ -106,21 +141,38 @@ func (is *PhysicalIndexScan) matchProperty(prop *requiredProperty, infos ...*phy
if allAsc {

This comment has been minimized.

Copy link
@shenli
@@ -259,12 +259,8 @@ func (p *basePlan) initID() {
// basePlan implements base Plan interface.
// Should be used as embedded struct in Plan implementations.
type basePlan struct {
fields []*ast.ResultField
startupCost float64
totalCost float64

This comment has been minimized.

Copy link
@shenli

shenli Oct 8, 2016

Member

These attributes are useless?

This comment has been minimized.

Copy link
@hanfei1991

hanfei1991 Oct 8, 2016

Author Member

yeah

@@ -46,6 +50,10 @@ func (s *testPlanSuite) SetUpSuite(c *C) {
s.Parser = parser.New()
}

func newType() types.FieldType {

This comment has been minimized.

Copy link
@shenli

shenli Oct 8, 2016

Member

newLongType is better.

@@ -373,6 +535,10 @@ func (s *testPlanSuite) TestCBO(c *C) {
best: "Index(t.c_d_e)[[-inf,10000)]->Sort + Limit(2) + Offset(0)->Projection",
},
{
sql: "select * from t a where a.c < 10000 and a.d in (1000, a.e) order by a.a limit 2",

This comment has been minimized.

Copy link
@shenli

hanfei1991 added some commits Oct 8, 2016

@hanfei1991

This comment has been minimized.

Copy link
Member Author

commented Oct 8, 2016

@@ -406,9 +385,6 @@ func (p *Join) handleLeftJoin(prop *requiredProperty, innerJoin bool) (*physical
allLeft = false
}
}
if !allLeft {

This comment has been minimized.

Copy link
@shenli

shenli Oct 10, 2016

Member

Make best effort to push down sort. Because if there is a limit and sort, it will be converted to top-n. Running top-n as early as possible will save running time.

@shenli

This comment has been minimized.

Copy link
Member

commented Oct 10, 2016

LGTM

@@ -54,15 +53,14 @@ type PhysicalIndexScan struct {

accessEqualCount int
AccessCondition []expression.Expression
// ConditionPBExpr is the pb structure of conditions that pushed down.
ConditionPBExpr *tipb.Expr
conditions []expression.Expression

TableAsName *model.CIStr
}

type physicalDistSQLPlan interface {

This comment has been minimized.

Copy link
@coocood

coocood Oct 10, 2016

Member

Add comment about this interface and the methods, describe why we need this interface, how those method should be implemented and used.

@@ -40,7 +39,7 @@ type PhysicalIndexScan struct {
basePlan
physicalTableSource

ctx context.Context
txn kv.Transaction

This comment has been minimized.

Copy link
@coocood

coocood Oct 10, 2016

Member

We don't need to keep a txn reference, we only need to know if it is read only.

@@ -178,7 +179,7 @@ type PhysicalTableScan struct {
basePlan
physicalTableSource

ctx context.Context
txn kv.Transaction

This comment has been minimized.

Copy link
@coocood

coocood Oct 10, 2016

Member

ditto

@coocood

This comment has been minimized.

Copy link
Member

commented Oct 11, 2016

LGTM

@hanfei1991 hanfei1991 merged commit 9b9c011 into master Oct 11, 2016

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details

@hanfei1991 hanfei1991 deleted the hanfei/topn branch Oct 11, 2016

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.