*: Selection can control the conditions of the below scan plan. #2834

winoros · 2017-03-15T11:50:12Z

If one of the conditions of this selection contains both key and correlated column and the child of it is a table scan, this condition could be pushed into the access conditions or filter conditions because the correlated column is constant when executed.
After this, we could change some join to index nested loop join.
PTAL @hanfei1991 @shenli @coocood @lamxTyler

hanfei1991 · 2017-03-15T11:59:44Z

Good Job!

hanfei1991 · 2017-03-16T08:37:38Z

executor/executor.go

 // SelectionExec represents a filter executor.
 type SelectionExec struct {
 	Src       Executor
 	Condition expression.Expression
 	ctx       context.Context
 	schema    *expression.Schema
+
+	scanController      bool


add comments for these two fields.

hanfei1991 · 2017-03-16T08:39:05Z

executor/executor.go

+		}
+		var conds []expression.Expression
+		x.where, _, conds = plan.ExpressionsToPB(sc, tblFilters, client)
+		e.Condition = expression.ComposeCNFCondition(e.ctx, conds...)


e.Condition don't need to be changed.

hanfei1991 · 2017-03-16T08:40:04Z

executor/executor.go

@@ -461,21 +461,104 @@ func (e *TableDualExec) Close() error {
 	return nil
 }

+func convertCorCol2Constant(expr expression.Expression) (expression.Expression, error) {


convert-> substitute

hanfei1991 · 2017-03-16T08:41:26Z

executor/executor.go

+			}
+			newArgs = append(newArgs, newArg)
+		}
+		newSf, _ := expression.NewFunction(x.GetCtx(), x.FuncName.L, x.GetType(), newArgs...)


you may consider the case of "Cast"

i will look into this tonight.

hanfei1991 · 2017-03-16T08:42:31Z

executor/executor.go

+		newSf, _ := expression.NewFunction(x.GetCtx(), x.FuncName.L, x.GetType(), newArgs...)
+		return newSf, nil
+	case *expression.CorrelatedColumn:
+		val, err := x.Eval(nil)


you can use the x.Data directly, without considering err

hanfei1991 · 2017-03-16T08:42:52Z

plan/expr_to_pb.go

@@ -25,7 +25,8 @@ import (
 	"github.com/pingcap/tipb/go-tipb"
 )

-func expressionsToPB(sc *variable.StatementContext, exprs []expression.Expression, client kv.Client) (pbExpr *tipb.Expr, pushed []expression.Expression, remained []expression.Expression) {
+// ExpressionsToPB change expression to tipb.Expr


shenli · 2017-03-16T13:36:22Z

plan/expr_to_pb.go

@@ -25,7 +25,8 @@ import (
 	"github.com/pingcap/tipb/go-tipb"
 )

-func expressionsToPB(sc *variable.StatementContext, exprs []expression.Expression, client kv.Client) (pbExpr *tipb.Expr, pushed []expression.Expression, remained []expression.Expression) {
+// ExpressionsToPB change expression to tipb.Expr.


change -> converts

shenli · 2017-03-16T13:39:37Z

executor/executor.go

 }

 // Schema implements the Executor Schema interface.
 func (e *SelectionExec) Schema() *expression.Schema {
 	return e.schema
 }

+// initController will init the conditions of the below scan executor.


Add more comments for this function.

shenli · 2017-03-16T13:43:44Z

plan/physical_plan_builder.go

+	}
+}
+
+func (p *Selection) getUsableIndicesAndPk(ds *DataSource) ([]*model.IndexInfo, model.CIStr) {


Add comment for this function.

hanfei1991 · 2017-03-18T05:18:41Z

executor/executor.go

+			allConstant = allConstant && ok
+		}
+		if allConstant {
+			val, _ := x.Eval(nil)


This error can't be ignored ...

hanfei1991 · 2017-03-18T05:22:35Z

executor/executor.go

+		if err != nil {
+			return errors.Trace(err)
+		}
+		x.indexPlan.IndexConditionPBExpr, _, _ = plan.ExpressionsToPB(sc, e.idxFilterConditions, client)


When do we update the e.idxFilterConditions?

e.idxFilterCondtion use the value of *Select.IdxConditions directly, i think this don't need to update in executor phase.

hanfei1991 · 2017-03-18T05:23:07Z

executor/executor.go

@@ -484,6 +567,9 @@ func (e *SelectionExec) Next() (*Row, error) {
 		if srcRow == nil {
 			return nil, nil
 		}
+		if e.Condition == nil {


we can remove this.

hanfei1991 · 2017-03-18T05:23:43Z

plan/logical_plans.go

@@ -146,6 +146,13 @@ type Selection struct {

 	// onTable means if this selection's child is a table scan or index scan.
 	onTable bool
+
+	// If ScanController is true, then the child of this selection is a scan,
+	// which use pk or index


hanfei1991 · 2017-03-18T05:31:21Z

plan/physical_plan_builder.go

@@ -792,6 +1027,11 @@ func (p *Selection) convert2PhysicalPlan(prop *requiredProperty) (*physicalPlanI
 	if info != nil {
 		return info, nil
 	}
+	info = p.tryToBuildScanByKeyAndCorCol()


This check is very heavy. It may be called multiple times. I suggested that If 'flagDecorrelated' is true, check it in an other logic but in physical plan builder.

hanfei1991 · 2017-03-20T03:44:50Z

executor/executor.go

 	switch x := expr.(type) {
 	case *expression.ScalarFunction:
 		allConstant := true
 		newArgs := make([]expression.Expression, 0, len(x.GetArgs()))
 		for _, arg := range x.GetArgs() {
-			newArg, ok := substituteCorCol2Constant(arg)
+			newArg, ok, err := substituteCorCol2Constant(arg)


The ok is redundant. You can just check if newArg is constant

hanfei1991 · 2017-03-20T03:53:58Z

plan/physical_plan_builder.go

+		TableAsName:         p.TableAsName,
+		OutOfOrder:          true,
+		DBName:              p.DBName,
+		physicalTableSource: physicalTableSource{client: p.ctx.GetClient()},


client: client

hanfei1991 · 2017-03-20T10:12:33Z

plan/physical_plan_builder.go

@@ -783,6 +785,261 @@ func (p *Union) convert2PhysicalPlan(prop *requiredProperty) (*physicalPlanInfo,
 	return info, nil
 }

+func buildTableScanByKeyAndCorCol(p *DataSource, pkName model.CIStr, fakeConds []expression.Expression, origConds []expression.Expression) (*physicalPlanInfo, []expression.Expression, []expression.Expression) {


I seems we don't need to do like that. Building filter condition during runtime is more reasonable.

hanfei1991 · 2017-03-21T08:30:48Z

executor/builder.go

+		schema:         v.Schema(),
+		ctx:            b.ctx,
+		scanController: v.ScanController,
+		Conditions:     v.Conditions,


If you store the conditions, you don't need to store condition any more. Use a for loop to check condition.

hanfei1991 · 2017-03-21T08:36:25Z

plan/physical_plan_builder.go

+// getUsableIndicesAndPk will simply check whether the pk or one index could used in this situation by
+// checking whether this index or pk is contained in one condition that has correlated column,
+// and whether this condition can be used as an access condition.
+func (p *Selection) getUsableIndicesAndPk(ds *DataSource) ([]*model.IndexInfo, model.CIStr) {


Don't peek all indices out, just ensure one index during plan building.

hanfei1991 · 2017-03-22T03:04:36Z

executor/executor.go

+		newArgs := make([]expression.Expression, 0, len(x.GetArgs()))
+		for _, arg := range x.GetArgs() {
+			newArg, err := substituteCorCol2Constant(arg)
+			_, ok := newArg.(*expression.Constant)


Put this after checking error.

hanfei1991 · 2017-03-22T03:05:28Z

plan/physical_plan_builder.go

+// The first return value is the new expression, the second is a bool value tell whether the below expression is all constant.
+// The Second is used for simplify the scalar function.
+// If the args of one scalar function are all constant, we will substitute it to constant.
+func substituteCorCol2Constant(cond expression.Expression) expression.Expression {


You implement this function for two times. Why not put this function to expression package?

One is change correlated column to expression.One, another is eval the correlated column to Datum. I thought eval a correlated column when its Data is nothing doesn't make sense.

Extract all cor columns and set them to one, then you can reuse this function.

hanfei1991 · 2017-03-22T03:06:39Z

executor/executor.go

+	scanController bool
+	controllerInit bool
+	Conditions     []expression.Expression
+	usableIndices  []*model.IndexInfo


I think you can remove this from selection executor.

hanfei1991 · 2017-03-22T05:42:43Z

executor/executor.go

+		}
+		x.where = x.indexPlan.TableConditionPBExpr
+	default:
+		return errors.New("Error type of PhysicalPlan")


print %T to tout put the plan type

hanfei1991 · 2017-03-22T05:46:52Z

plan/physical_plan_builder.go

+	var newConds []expression.Expression
+	for _, expr := range p.Conditions {
+		cond := pushDownNot(expr.Clone(), false, nil)
+		if !cond.IsCorrelated() {


check this before push down not

hanfei1991 · 2017-03-22T05:48:28Z

plan/physical_plan_builder.go

+// checking whether this index or pk is contained in one condition that has correlated column,
+// and whether this condition can be used as an access condition.
+func (p *Selection) getUsableIndicesAndPk(ds *DataSource) ([]*model.IndexInfo, model.CIStr) {
+	indices, _ := availableIndices(ds.indexHints, ds.tableInfo)


Why ignore the includeTableScan

hanfei1991 · 2017-03-22T05:49:45Z

plan/physical_plan_builder.go

+			} else {
+				is.readOnly = true
+			}
+			is.AccessCondition, condsBackUp = DetachIndexScanConditions(condsBackUp, is)


Why do we need to calculate access condtion, index conds and tbl conds here ?

Calculating access condition is to choose which index is better by accessEqualCount and accessInAndEqCount.
Idx conds and tbl conds is used to make explain result clear, can be removed.

remove idx and tbl conds. and their pb

hanfei1991 · 2017-03-22T07:39:54Z

plan/logical_plans.go

+	// Since one selection may call convert2PhysicalScan many times. We extract the PkName and indices
+	// used for scanController only once and store them to judge whether this selection can convert to
+	// scanController mode.
+	usefulPkName         model.CIStr


I think we don't need to store the usefulPkName and usefulIndices.

If we don't store the usefulPkName and usefulIndices. We will need to extract them one more time when build controller.

okay, it seems not cost too much.

hanfei1991 · 2017-03-22T07:46:38Z

plan/logical_plans.go

+	// scanController mode.
+	usefulPkName         model.CIStr
+	usefulIndices        []*model.IndexInfo
+	extractedUsefulThing bool


set a flag that we set controller when the flag is true. The flag is true only the selection has correlated column and its child is DataSource. You can check this during decorrelation.

coocood · 2017-03-22T08:08:44Z

plan/physical_plan_builder.go

+			if chosenPlan == nil || chosenPlan.accessEqualCount < is.accessEqualCount || chosenPlan.accessInAndEqCount < is.accessInAndEqCount {
+				chosenPlan = is
+			}
+			is.DoubleRead = isCoveringIndex(is.Columns, is.Index.Columns, is.Table.PKIsHandle)


missing !?

yes, fixed.

add test for it!

hanfei1991 · 2017-03-22T09:21:15Z

plan/decorrelate.go

@@ -158,6 +160,11 @@ func (s *decorrelateSolver) optimize(p LogicalPlan, _ context.Context, _ *idAllo
 			}
 		}
 	}
+	if sel, ok := p.(*Selection); ok {
+		if ds, ok := p.(*DataSource); ok {


p.GetChildren()[0]...
output controller to sel explain result. and add explain test for it.

hanfei1991 · 2017-03-22T11:46:45Z

LGTM
resolve the conflict.
@shenli @coocood PTAL

winoros · 2017-03-22T11:54:33Z

plan/physical_plan_builder.go

+		p:     &newSel,
+		count: uint64(ds.statisticTable.Count),
+	}
+	info.cost = float64(info.count) * selectionFactor


Is this cost calculation okay there?

shenli · 2017-03-23T08:30:25Z

expression/util.go

+		return &Constant{Value: *x.Data, RetType: x.GetType()}, nil
+	case *Constant:
+		return x.Clone(), nil
+	default:


In which case we will fall to default?

*Column will fall to default. i will move *Constant to default too.

coocood · 2017-03-23T10:23:46Z

@winoros
The double read fix is not covered by test.
I removed !, the test still pass.

coocood · 2017-03-23T10:29:18Z

@winoros
I checked the test coverage, the new makeScanController function is not covered at all.

coocood · 2017-03-23T10:37:21Z

expression/util.go

+// If the args of one scalar function are all constant, we will substitute it to constant.
+// If it's called in plan phase, correlated column will change to expression.One.
+// If in executor phase, correlated column will change to value it contains.
+func SubstituteCorCol2Constant(expr Expression) (Expression, error) {


I think it's not too hard to write a test for this function.

coocood · 2017-03-23T10:39:33Z

plan/decorrelate.go

+// hasUsableIndicesAndPk will simply check whether the pk or one index could used in this situation by
+// checking whether this index or pk is contained in one condition that has correlated column,
+// and whether this condition can be used as an access condition.
+func (p *Selection) hasUsableIndicesAndPk(ds *DataSource) bool {


This function is very complex, need to write a test.

coocood · 2017-03-23T11:00:22Z

plan/decorrelate.go

+			// If one cond is ok, then this index is useful.
+			if checker.check(cond) {
+				usable = true
+				break


Why not just return true?

coocood · 2017-03-24T10:53:03Z

plan/physical_plan_builder.go

+		}
+		child = ts
+	} else {
+		var chosenPlan *PhysicalIndexScan


This branch is not covered.
availableIndices returns all indices for you to chose if there is no index hint, but if the table has a PKHandle column, other indices will never be used.

shenli · 2017-03-27T04:59:55Z

plan/decorrelate.go

@@ -167,3 +173,64 @@ func (s *decorrelateSolver) optimize(p LogicalPlan, _ context.Context, _ *idAllo
 	p.SetChildren(newChildren...)
 	return p, nil
 }
+
+// hasUsableIndicesAndPk will simply check whether the pk or one index could used in this situation by


could be used

shenli · 2017-03-27T05:00:18Z

plan/decorrelate.go

@@ -167,3 +173,64 @@ func (s *decorrelateSolver) optimize(p LogicalPlan, _ context.Context, _ *idAllo
 	p.SetChildren(newChildren...)
 	return p, nil
 }
+
+// hasUsableIndicesAndPk will simply check whether the pk or one index could used in this situation by
+// checking whether this index or pk is contained in one condition that has correlated column,


in any condition

shenli · 2017-03-27T05:17:07Z

plan/decorrelate.go

+	if ds.tableInfo.PKIsHandle && includeTableScan {
+		var pkCol *expression.Column
+		for i, col := range ds.Columns {
+			if mysql.HasPriKeyFlag(col.Flag) {


Consider composed index?

shenli · 2017-03-27T05:17:52Z

plan/decorrelate.go

+		if pkCol != nil {
+			checker := conditionChecker{
+				pkName: pkCol.ColName,
+				length: types.UnspecifiedLength,


Why use UnspecifiedLength?

This is the same with DetachTableScanConditions() in refiner.go.

shenli · 2017-03-27T05:22:05Z

plan/physical_plan_builder.go

@@ -981,6 +982,98 @@ func (p *Union) convert2PhysicalPlan(prop *requiredProperty) (*physicalPlanInfo,
 	return info, nil
 }

+func (p *Selection) makeScanController() *physicalPlanInfo {


This function could be merged with hasUsableIndicesAndPk.

winoros · 2017-03-27T11:28:43Z

PTAL @shenli

shenli · 2017-03-28T01:01:26Z

LGTM
Well done!

winoros added 6 commits March 14, 2017 20:08

*: add indexScan controlled by correlated column

5db58a9

Merge branch 'master' into yiding/apply_optimize

1fe024d

tiny change

5ce9107

fix merge bug

364e172

remove log

36283cf

remove log

7451287

fix bug and tiny change

053c2d4

hanfei1991 reviewed Mar 16, 2017

View reviewed changes

winoros added 2 commits March 16, 2017 02:27

address some comments

e663291

manage cast in plan build phase

a25a3eb

shenli reviewed Mar 17, 2017

View reviewed changes

address comments and manage cast in executor phase

6aadeaa

hanfei1991 reviewed Mar 18, 2017

View reviewed changes

address some comments

ba29a95

hanfei1991 reviewed Mar 20, 2017

View reviewed changes

winoros added 3 commits March 19, 2017 22:50

remove useless args

02cbcf1

change some logic

9a9ae32

fix bug

ef178a3

winoros force-pushed the yiding/inlj_1 branch from d08c2fe to ef178a3 Compare March 20, 2017 09:50

hanfei1991 reviewed Mar 20, 2017

View reviewed changes

refactor

8029c1e

hanfei1991 reviewed Mar 21, 2017

View reviewed changes

winoros added 2 commits March 21, 2017 19:43

address comments

9771129

tiny change

7251827

hanfei1991 reviewed Mar 22, 2017

View reviewed changes

address some comments

e45f669

hanfei1991 reviewed Mar 22, 2017

View reviewed changes

coocood reviewed Mar 22, 2017

View reviewed changes

move code

c791ff7

hanfei1991 reviewed Mar 22, 2017

View reviewed changes

typo error && add explain

8674c3f

winoros changed the title *: Selection can control the accessConditions of the below scan plan. *: Selection can control the conditions of the below scan plan. Mar 22, 2017

winoros commented Mar 22, 2017

View reviewed changes

winoros added the status/LGT1 Indicates that a PR has LGTM 1. label Mar 23, 2017

winoros added 2 commits March 23, 2017 10:02

Merge branch 'master' into yiding/inlj_1

107a042

remove outdate comment

3fd0435

shenli reviewed Mar 23, 2017

View reviewed changes

coocood reviewed Mar 23, 2017

View reviewed changes

winoros added 2 commits March 24, 2017 16:31

add tests and some tiny change

8f29d17

tiny change

088e9e6

coocood reviewed Mar 24, 2017

View reviewed changes

add test

6351036

shenli reviewed Mar 27, 2017

View reviewed changes

address comments

97998fc

shenli approved these changes Mar 28, 2017

View reviewed changes

Merge branch 'master' into yiding/inlj_1

55f92e6

shenli merged commit 0e172ac into master Mar 28, 2017

hanfei1991 deleted the yiding/inlj_1 branch March 28, 2017 08:03

winoros mentioned this pull request Mar 30, 2017

*: implement index nested loop join #2945

Merged

*: Selection can control the conditions of the below scan plan. #2834

*: Selection can control the conditions of the below scan plan. #2834

Conversation

winoros commented Mar 15, 2017

hanfei1991 commented Mar 15, 2017

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

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

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 Mar 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coocood commented Mar 23, 2017

coocood commented Mar 23, 2017

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

winoros commented Mar 27, 2017

shenli commented Mar 28, 2017