plan: consider limit during building physcial plan. #1760

hanfei1991 · 2016-09-23T02:03:56Z

The purpose of this pr is to push down topn operator.#1746 I change the plan part before supporting it in local store.
@shenli @coocood @zimulala @tiancaiamao @XuHuaiyu PTAL

coocood · 2016-09-23T02:48:48Z

plan/match_property.go

@@ -25,13 +25,16 @@ func (ts *PhysicalTableScan) matchProperty(prop *requiredProperty, infos ...*phy
 	rowCount := float64(infos[0].count)
 	cost := rowCount * netWorkFactor
 	if len(prop.props) == 0 {
-		return &physicalPlanInfo{p: ts, cost: cost, count: infos[0].count}
+		return enforceProperty(prop, &physicalPlanInfo{p: ts, cost: cost, count: infos[0].count})


if len(prop.props) == 0 && prop.limit != nil?

It's no need to check whether prop.limit is nil. If it is, it won't change the plan info.

coocood · 2016-09-23T02:50:20Z

plan/logical_plans.go

@@ -76,6 +76,8 @@ type Selection struct {
 	// but after we converted to CNF(Conjunctive normal form), it can be
 	// split into a list of AND conditions.
 	Conditions []expression.Expression
+
+	aboveTableScan bool


Add comments about this property.

coocood · 2016-09-23T02:52:45Z

plan/match_property.go

@@ -75,7 +78,7 @@ func (is *PhysicalIndexScan) matchProperty(prop *requiredProperty, infos ...*phy
 		cost *= 2
 	}
 	if len(prop.props) == 0 {
-		return &physicalPlanInfo{p: is, cost: cost, count: infos[0].count}
+		return enforceProperty(&requiredProperty{limit: prop.limit}, &physicalPlanInfo{p: is, cost: cost, count: infos[0].count})


Define a limitProperty, it will be used in several places.

coocood · 2016-09-23T03:01:48Z

plan/physical_plan_builder.go

+	return info
+}
+
+func removeLimit(prop *requiredProperty, removeAll bool) *requiredProperty {


Add comments about when and why we need to remove limit, and when removeAll need to be false when removeAll need to be true.

coocood · 2016-09-23T03:18:30Z

plan/physical_plan_builder.go

+		sortKeyLen: prop.sortKeyLen,
+		limit:      prop.limit}
+	if len(prop.props) > 0 {
+	}


Unfinished or useless?

hanfei1991 · 2016-09-23T10:48:14Z

@shenli @coocood @zimulala @XuHuaiyu @tiancaiamao PTAL

coocood · 2016-09-23T13:27:27Z

plan/physical_plan_builder.go

+// removeLimit removes limit from prop. For example, When handling Sort,Limit -> Selection, we can't pass the Limit
+// across the selection, because selection decreases the size of data, but we can pass the Sort below the selection. In
+// this case, we set removeAll true. When handling Limit(1,1) -> LeftOuterJoin, we can pass the limit across join's left
+// path, because the left outer join increases the size of data, but we can't pass offset value. So we set remove All to false.


So when removeAll is set to false, it's meant to remove offset, then how about define a different function named removeOffset?

Current designment is orthogonal

I don't get your point.
It's confusing that removeLimit is not doing what it's supposed to do.
When removeAll is set to false, it doesn't remove limit at all.

coocood · 2016-09-23T13:34:24Z

plan/logical_plans.go

@@ -77,6 +77,7 @@ type Selection struct {
 	// split into a list of AND conditions.
 	Conditions []expression.Expression

+	// aboveTableScan means if this selection's child is a table scan or index scan.


So aboveTableScan is false if the selection's child is a join or having? Then above is not accurate, and TableScan doesn't contains IndexScan.
How about onTable?

shenli · 2016-09-23T12:41:55Z

plan/expr_to_pb.go

 		}
 	}
 	err := types.SortDatums(datums)
 	if err != nil {
-		return nil, errors.Trace(err)
+		log.Error(err.Error())


We will loss the error.

I think this error shouldn't make sql fail. Instead, it can forbid this condition to push down.

shenli · 2016-09-23T12:42:30Z

plan/expr_to_pb.go

 	}
 	val, err := codec.EncodeValue(nil, datums...)
 	if err != nil {
-		return nil, errors.Trace(err)
+		log.Error(err.Error())


shenli · 2016-09-25T01:54:51Z

plan/physical_plan_builder.go

@@ -325,6 +323,58 @@ func addPlanToResponse(p PhysicalPlan, planInfo *physicalPlanInfo) *physicalPlan
 	return &physicalPlanInfo{p: np, cost: planInfo.cost, count: planInfo.count}
 }

+func enforceProperty(prop *requiredProperty, info *physicalPlanInfo) *physicalPlanInfo {


Add comment for this.

shenli · 2016-09-25T01:56:21Z

plan/physical_plan_builder.go

+// removeLimit removes limit from prop. For example, When handling Sort,Limit -> Selection, we can't pass the Limit
+// across the selection, because selection decreases the size of data, but we can pass the Sort below the selection. In
+// this case, we set removeAll true. When handling Limit(1,1) -> LeftOuterJoin, we can pass the limit across join's left
+// path, because the left outer join increases the size of data, but we can't pass offset value. So we set remove All to false.


remove All -> removeAll

shenli · 2016-09-25T01:57:43Z

plan/physical_plan_builder.go

@@ -677,33 +735,72 @@ func (p *Union) convert2PhysicalPlan(prop *requiredProperty) (*physicalPlanInfo,
 		childInfos = append(childInfos, info)
 	}
 	info = p.matchProperty(prop, childInfos...)
+	info = enforceProperty(limitProperty(limit), info)


When should we call this function?

We should call this function upon every operator that refuses the required property.

shenli · 2016-09-25T01:58:23Z

plan/physical_plan_builder.go

 	info, err := p.getPlanInfo(prop)
 	if err != nil {
 		return nil, errors.Trace(err)
 	}
 	if info != nil {
 		return info, nil
 	}
-	info, err = p.GetChildByIndex(0).(LogicalPlan).convert2PhysicalPlan(prop)
+	// Firstly, we try to push order


Is there a secondly?

I will add a "secondly" comment.

shenli · 2016-09-25T02:00:07Z

plan/physical_plans.go

+	}
+}
+
+func (p *physicalTableSource) addTopN(prop *requiredProperty) {


Do we support top-n on localstore?

We havn't. I will implement it in next pr.

hanfei1991 · 2016-09-26T00:35:39Z

@shenli @coocood @zimulala @tiancaiamao @XuHuaiyu PTAL

hanfei1991 · 2016-09-26T04:09:36Z

PTAL @coocood

coocood · 2016-09-26T05:20:25Z

LGTM

shenli · 2016-09-26T06:21:00Z

plan/physical_plan_builder.go

@@ -325,6 +323,62 @@ func addPlanToResponse(p PhysicalPlan, planInfo *physicalPlanInfo) *physicalPlan
 	return &physicalPlanInfo{p: np, cost: planInfo.cost, count: planInfo.count}
 }

+// enforceProperty add an topN or sort upon current operator.


May thie function add limit upon current operator?

Yes, you are right. I will update the comment

shenli · 2016-09-26T07:20:27Z

plan/expr_to_pb.go

 	}

 	id := column.ID
 	// Zero Column ID is not a column from table, can not support for now.
 	if id == 0 {
-		return nil, nil
+		return nil


if id == 0 || id == -1 { return nil}

hanfei1991 · 2016-09-26T09:30:22Z

@shenli PTAL

shenli · 2016-09-26T13:43:29Z

LGTM

hanfei1991 added 3 commits September 23, 2016 08:21

tmp commit

f97a85e

Merge branch 'master' into hanfei/topn

66c279b

consider limit during cbo

2a097db

coocood reviewed Sep 23, 2016

View reviewed changes

tiny change.

1cbdafc

fix bug.

b2526e9

hanfei1991 force-pushed the hanfei/topn branch from ead75fe to b2526e9 Compare September 23, 2016 10:51

coocood reviewed Sep 23, 2016

View reviewed changes

shenli reviewed Sep 24, 2016

View reviewed changes

shenli reviewed Sep 25, 2016

View reviewed changes

hanfei1991 force-pushed the hanfei/topn branch from 09fcec8 to 611e890 Compare September 26, 2016 00:33

tiny change.

80977e9

hanfei1991 force-pushed the hanfei/topn branch from 611e890 to 80977e9 Compare September 26, 2016 00:34

change removeLimit to removeLimit and convertLimitOffsetToCount

c78ed5a

shenli reviewed Sep 26, 2016

View reviewed changes

address comment.

971084d

shenli reviewed Sep 26, 2016

View reviewed changes

hanfei1991 added 2 commits September 26, 2016 17:29

address comment.

29b9cb6

Merge branch 'master' into hanfei/topn

52cd241

shenli merged commit 471731b into master Sep 26, 2016

shenli deleted the hanfei/topn branch September 26, 2016 13:53

hanfei1991 mentioned this pull request Sep 27, 2016

plan: support topn push down in plan phase. #1769

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

plan: consider limit during building physcial plan. #1760

plan: consider limit during building physcial plan. #1760

hanfei1991 commented Sep 23, 2016 •

edited

Loading

coocood Sep 23, 2016

hanfei1991 Sep 23, 2016

coocood Sep 23, 2016

coocood Sep 23, 2016

coocood Sep 23, 2016 •

edited

Loading

coocood Sep 23, 2016

hanfei1991 commented Sep 23, 2016

coocood Sep 23, 2016 •

edited

Loading

hanfei1991 Sep 26, 2016

coocood Sep 26, 2016

coocood Sep 23, 2016

shenli Sep 23, 2016

hanfei1991 Sep 24, 2016

shenli Sep 23, 2016

shenli Sep 25, 2016

shenli Sep 25, 2016

shenli Sep 25, 2016

hanfei1991 Sep 26, 2016

shenli Sep 25, 2016

hanfei1991 Sep 26, 2016

shenli Sep 25, 2016

hanfei1991 Sep 26, 2016

hanfei1991 commented Sep 26, 2016 •

edited

Loading

hanfei1991 commented Sep 26, 2016

coocood commented Sep 26, 2016

shenli Sep 26, 2016

hanfei1991 Sep 26, 2016

shenli Sep 26, 2016

hanfei1991 commented Sep 26, 2016

shenli commented Sep 26, 2016

plan: consider limit during building physcial plan. #1760

plan: consider limit during building physcial plan. #1760

Conversation

hanfei1991 commented Sep 23, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coocood Sep 23, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hanfei1991 commented Sep 23, 2016

coocood Sep 23, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hanfei1991 commented Sep 26, 2016 • edited Loading

hanfei1991 commented Sep 26, 2016

coocood commented Sep 26, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hanfei1991 commented Sep 26, 2016

shenli commented Sep 26, 2016

hanfei1991 commented Sep 23, 2016 •

edited

Loading

coocood Sep 23, 2016 •

edited

Loading

coocood Sep 23, 2016 •

edited

Loading

hanfei1991 commented Sep 26, 2016 •

edited

Loading