From 8bd985fcd1de804ac395a78dd1ec25e1317e9bc8 Mon Sep 17 00:00:00 2001 From: zhexuany Date: Wed, 17 Oct 2018 03:36:45 +0800 Subject: [PATCH 1/5] eliminate if null on non null column --- planner/core/logical_plan_builder.go | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/planner/core/logical_plan_builder.go b/planner/core/logical_plan_builder.go index a00843d44f9fa..3a9007431f826 100644 --- a/planner/core/logical_plan_builder.go +++ b/planner/core/logical_plan_builder.go @@ -576,6 +576,23 @@ func (b *PlanBuilder) buildProjectionField(id, position int, field *ast.SelectFi } } +func eliminateIfNullOnNonNullColumn(p LogicalPlan, expr expression.Expression) expression.Expression { + if scalarExpr, ok := expr.(*expression.ScalarFunction); ok { + if scalarExpr.FuncName.L == ast.Ifnull { + if cExpr, ok := scalarExpr.GetArgs()[0].(*expression.Column); ok { + var colFound *model.ColumnInfo + if ds, ok := p.(*DataSource); ok { + colFound = model.FindColumnInfo(ds.Columns, cExpr.ColName.L) + if mysql.HasNotNullFlag(colFound.Flag) { + return scalarExpr.GetArgs()[0] + } + } + } + } + } + return expr +} + // buildProjection returns a Projection plan and non-aux columns length. func (b *PlanBuilder) buildProjection(p LogicalPlan, fields []*ast.SelectField, mapper map[*ast.AggregateFuncExpr]int) (LogicalPlan, int, error) { b.optFlag |= flagEliminateProjection @@ -585,6 +602,7 @@ func (b *PlanBuilder) buildProjection(p LogicalPlan, fields []*ast.SelectField, oldLen := 0 for _, field := range fields { newExpr, np, err := b.rewrite(field.Expr, p, mapper, true) + newExpr = eliminateIfNullOnNonNullColumn(p, newExpr) if err != nil { return nil, 0, errors.Trace(err) } From 4623f362d5e04c05ad9c1f0e4302c5023e1b4616 Mon Sep 17 00:00:00 2001 From: zhexuany Date: Wed, 17 Oct 2018 13:37:24 +0800 Subject: [PATCH 2/5] adding explain test --- cmd/explaintest/r/explain_easy.result | 26 ++++++++++++++++++++++++++ cmd/explaintest/t/explain_easy.test | 10 ++++++++++ 2 files changed, 36 insertions(+) diff --git a/cmd/explaintest/r/explain_easy.result b/cmd/explaintest/r/explain_easy.result index b9bf7c9e94c2e..83f8c725c48e6 100644 --- a/cmd/explaintest/r/explain_easy.result +++ b/cmd/explaintest/r/explain_easy.result @@ -385,3 +385,29 @@ Projection_5 8000.00 root test.ta.a └─TableReader_9 10000.00 root data:TableScan_8 └─TableScan_8 10000.00 cop table:ta, range:[-inf,+inf], keep order:false, stats:pseudo rollback; +drop table if exists t; +create table t(a int, nb int not null, nc int not null); +explain select ifnull(a, 0) from t; +id count task operator info +Projection_3 10000.00 root ifnull(test.t.a, 0) +└─TableReader_5 10000.00 root data:TableScan_4 + └─TableScan_4 10000.00 cop table:t, range:[-inf,+inf], keep order:false, stats:pseudo +explain select ifnull(nb, 0) from t; +id count task operator info +TableReader_5 10000.00 root data:TableScan_4 +└─TableScan_4 10000.00 cop table:t, range:[-inf,+inf], keep order:false, stats:pseudo +explain select ifnull(nb, 0), ifnull(nc, 0) from t; +id count task operator info +TableReader_5 10000.00 root data:TableScan_4 +└─TableScan_4 10000.00 cop table:t, range:[-inf,+inf], keep order:false, stats:pseudo +explain select ifnull(a, 0), ifnull(nb, 0) from t; +id count task operator info +Projection_3 10000.00 root ifnull(test.t.a, 0), test.t.nb +└─TableReader_5 10000.00 root data:TableScan_4 + └─TableScan_4 10000.00 cop table:t, range:[-inf,+inf], keep order:false, stats:pseudo +explain select ifnull(nb, 0), ifnull(nb, 0) from t; +id count task operator info +Projection_3 10000.00 root test.t.nb, test.t.nb +└─TableReader_5 10000.00 root data:TableScan_4 + └─TableScan_4 10000.00 cop table:t, range:[-inf,+inf], keep order:false, stats:pseudo +drop table if exists t; diff --git a/cmd/explaintest/t/explain_easy.test b/cmd/explaintest/t/explain_easy.test index 23214476f20aa..978168c576281 100644 --- a/cmd/explaintest/t/explain_easy.test +++ b/cmd/explaintest/t/explain_easy.test @@ -81,3 +81,13 @@ begin; insert tb values ('1'); explain select * from ta where a = 1; rollback; + +# https://github.com/pingcap/tidb/issues/7918 +drop table if exists t; +create table t(a int, nb int not null, nc int not null); +explain select ifnull(a, 0) from t; +explain select ifnull(nb, 0) from t; +explain select ifnull(nb, 0), ifnull(nc, 0) from t; +explain select ifnull(a, 0), ifnull(nb, 0) from t; +explain select ifnull(nb, 0), ifnull(nb, 0) from t; +drop table if exists t; From 7000f42801e3f0ffa512b94d26ec109fa0fc6ee1 Mon Sep 17 00:00:00 2001 From: zhexuany Date: Wed, 17 Oct 2018 18:15:39 +0800 Subject: [PATCH 3/5] recursivly trim expression tree --- cmd/explaintest/r/explain_easy.result | 11 +++++++ cmd/explaintest/t/explain_easy.test | 2 ++ expression/builtin.go | 6 ++++ expression/scalar_function.go | 5 ++++ planner/core/logical_plan_builder.go | 43 ++++++++++++++++++--------- 5 files changed, 53 insertions(+), 14 deletions(-) diff --git a/cmd/explaintest/r/explain_easy.result b/cmd/explaintest/r/explain_easy.result index 83f8c725c48e6..fa8a4e99b924d 100644 --- a/cmd/explaintest/r/explain_easy.result +++ b/cmd/explaintest/r/explain_easy.result @@ -410,4 +410,15 @@ id count task operator info Projection_3 10000.00 root test.t.nb, test.t.nb └─TableReader_5 10000.00 root data:TableScan_4 └─TableScan_4 10000.00 cop table:t, range:[-inf,+inf], keep order:false, stats:pseudo +explain select 1+ifnull(nb, 0) from t; +id count task operator info +Projection_3 10000.00 root plus(1, test.t.nb) +└─TableReader_5 10000.00 root data:TableScan_4 + └─TableScan_4 10000.00 cop table:t, range:[-inf,+inf], keep order:false, stats:pseudo +explain select 1+ifnull(a, 0) from t; +id count task operator info +Projection_3 10000.00 root plus(1, ifnull(test.t.a, 0)) +└─TableReader_5 10000.00 root data:TableScan_4 + └─TableScan_4 10000.00 cop table:t, range:[-inf,+inf], keep order:false, stats:pseudo +drop table if exists t; drop table if exists t; diff --git a/cmd/explaintest/t/explain_easy.test b/cmd/explaintest/t/explain_easy.test index 978168c576281..3800abd33ec67 100644 --- a/cmd/explaintest/t/explain_easy.test +++ b/cmd/explaintest/t/explain_easy.test @@ -90,4 +90,6 @@ explain select ifnull(nb, 0) from t; explain select ifnull(nb, 0), ifnull(nc, 0) from t; explain select ifnull(a, 0), ifnull(nb, 0) from t; explain select ifnull(nb, 0), ifnull(nb, 0) from t; +explain select 1+ifnull(nb, 0) from t; +explain select 1+ifnull(a, 0) from t; drop table if exists t; diff --git a/expression/builtin.go b/expression/builtin.go index ba165008278f9..aaf523b2d7d8e 100644 --- a/expression/builtin.go +++ b/expression/builtin.go @@ -162,6 +162,10 @@ func (b *baseBuiltinFunc) getArgs() []Expression { return b.args } +func (b *baseBuiltinFunc) setArgs(args []Expression) { + b.args = args +} + func (b *baseBuiltinFunc) evalInt(row chunk.Row) (int64, bool, error) { panic("baseBuiltinFunc.evalInt() should never be called.") } @@ -274,6 +278,8 @@ type builtinFunc interface { evalJSON(row chunk.Row) (val json.BinaryJSON, isNull bool, err error) // getArgs returns the arguments expressions. getArgs() []Expression + // setArgs set the arguments expressions to builtFunc. + setArgs(args []Expression) // equal check if this function equals to another function. equal(builtinFunc) bool // getCtx returns this function's context. diff --git a/expression/scalar_function.go b/expression/scalar_function.go index 34f0d7673e7ec..225528cb02c51 100644 --- a/expression/scalar_function.go +++ b/expression/scalar_function.go @@ -46,6 +46,11 @@ func (sf *ScalarFunction) GetArgs() []Expression { return sf.Function.getArgs() } +// SetArgs sets arguments of function. +func (sf *ScalarFunction) SetArgs(args []Expression) { + sf.Function.setArgs(args) +} + // GetCtx gets the context of function. func (sf *ScalarFunction) GetCtx() sessionctx.Context { return sf.Function.getCtx() diff --git a/planner/core/logical_plan_builder.go b/planner/core/logical_plan_builder.go index 3a9007431f826..5a6b6e49ec014 100644 --- a/planner/core/logical_plan_builder.go +++ b/planner/core/logical_plan_builder.go @@ -576,21 +576,36 @@ func (b *PlanBuilder) buildProjectionField(id, position int, field *ast.SelectFi } } -func eliminateIfNullOnNonNullColumn(p LogicalPlan, expr expression.Expression) expression.Expression { - if scalarExpr, ok := expr.(*expression.ScalarFunction); ok { - if scalarExpr.FuncName.L == ast.Ifnull { - if cExpr, ok := scalarExpr.GetArgs()[0].(*expression.Column); ok { - var colFound *model.ColumnInfo - if ds, ok := p.(*DataSource); ok { - colFound = model.FindColumnInfo(ds.Columns, cExpr.ColName.L) - if mysql.HasNotNullFlag(colFound.Flag) { - return scalarExpr.GetArgs()[0] - } - } - } +func eliminateIfNullOnNotNullCol(p LogicalPlan, expr expression.Expression) expression.Expression { + ds, isDs := p.(*DataSource) + if !isDs { + return expr + } + + scalarExpr, isScalarFunc := expr.(*expression.ScalarFunction) + if !isScalarFunc { + return expr + } + exprChildren := scalarExpr.GetArgs() + for i := 0; i < len(exprChildren); i++ { + exprChildren[i] = eliminateIfNullOnNotNullCol(p, exprChildren[i]) + } + + if scalarExpr.FuncName.L == ast.Ifnull { + colRef, isColRef := exprChildren[0].(*expression.Column) + if !isColRef { + return expr + } + + colInfo := model.FindColumnInfo(ds.Columns, colRef.ColName.L) + if !mysql.HasNotNullFlag(colInfo.Flag) { + return expr } + + return colRef } - return expr + scalarExpr.SetArgs(exprChildren) + return scalarExpr } // buildProjection returns a Projection plan and non-aux columns length. @@ -602,7 +617,7 @@ func (b *PlanBuilder) buildProjection(p LogicalPlan, fields []*ast.SelectField, oldLen := 0 for _, field := range fields { newExpr, np, err := b.rewrite(field.Expr, p, mapper, true) - newExpr = eliminateIfNullOnNonNullColumn(p, newExpr) + newExpr = eliminateIfNullOnNotNullCol(p, newExpr) if err != nil { return nil, 0, errors.Trace(err) } From 1e029ece89391f538933f084ba24bab0e6d244f4 Mon Sep 17 00:00:00 2001 From: zhexuany Date: Wed, 17 Oct 2018 21:46:54 +0800 Subject: [PATCH 4/5] address reviewer's comment --- expression/builtin.go | 6 ------ expression/scalar_function.go | 5 ----- planner/core/logical_plan_builder.go | 24 ++++++++++++------------ 3 files changed, 12 insertions(+), 23 deletions(-) diff --git a/expression/builtin.go b/expression/builtin.go index aaf523b2d7d8e..ba165008278f9 100644 --- a/expression/builtin.go +++ b/expression/builtin.go @@ -162,10 +162,6 @@ func (b *baseBuiltinFunc) getArgs() []Expression { return b.args } -func (b *baseBuiltinFunc) setArgs(args []Expression) { - b.args = args -} - func (b *baseBuiltinFunc) evalInt(row chunk.Row) (int64, bool, error) { panic("baseBuiltinFunc.evalInt() should never be called.") } @@ -278,8 +274,6 @@ type builtinFunc interface { evalJSON(row chunk.Row) (val json.BinaryJSON, isNull bool, err error) // getArgs returns the arguments expressions. getArgs() []Expression - // setArgs set the arguments expressions to builtFunc. - setArgs(args []Expression) // equal check if this function equals to another function. equal(builtinFunc) bool // getCtx returns this function's context. diff --git a/expression/scalar_function.go b/expression/scalar_function.go index 225528cb02c51..34f0d7673e7ec 100644 --- a/expression/scalar_function.go +++ b/expression/scalar_function.go @@ -46,11 +46,6 @@ func (sf *ScalarFunction) GetArgs() []Expression { return sf.Function.getArgs() } -// SetArgs sets arguments of function. -func (sf *ScalarFunction) SetArgs(args []Expression) { - sf.Function.setArgs(args) -} - // GetCtx gets the context of function. func (sf *ScalarFunction) GetCtx() sessionctx.Context { return sf.Function.getCtx() diff --git a/planner/core/logical_plan_builder.go b/planner/core/logical_plan_builder.go index 5a6b6e49ec014..e07878928a9f7 100644 --- a/planner/core/logical_plan_builder.go +++ b/planner/core/logical_plan_builder.go @@ -591,21 +591,21 @@ func eliminateIfNullOnNotNullCol(p LogicalPlan, expr expression.Expression) expr exprChildren[i] = eliminateIfNullOnNotNullCol(p, exprChildren[i]) } - if scalarExpr.FuncName.L == ast.Ifnull { - colRef, isColRef := exprChildren[0].(*expression.Column) - if !isColRef { - return expr - } + if scalarExpr.FuncName.L != ast.Ifnull { + return expr - colInfo := model.FindColumnInfo(ds.Columns, colRef.ColName.L) - if !mysql.HasNotNullFlag(colInfo.Flag) { - return expr - } + } + colRef, isColRef := exprChildren[0].(*expression.Column) + if !isColRef { + return expr + } - return colRef + colInfo := model.FindColumnInfo(ds.Columns, colRef.ColName.L) + if !mysql.HasNotNullFlag(colInfo.Flag) { + return expr } - scalarExpr.SetArgs(exprChildren) - return scalarExpr + + return colRef } // buildProjection returns a Projection plan and non-aux columns length. From a0f0a7f6013bec455f948b5e056cbdc4fb08492b Mon Sep 17 00:00:00 2001 From: zhexuany Date: Wed, 17 Oct 2018 21:47:34 +0800 Subject: [PATCH 5/5] remove extra line --- planner/core/logical_plan_builder.go | 1 - 1 file changed, 1 deletion(-) diff --git a/planner/core/logical_plan_builder.go b/planner/core/logical_plan_builder.go index e07878928a9f7..a82ca17db0947 100644 --- a/planner/core/logical_plan_builder.go +++ b/planner/core/logical_plan_builder.go @@ -593,7 +593,6 @@ func eliminateIfNullOnNotNullCol(p LogicalPlan, expr expression.Expression) expr if scalarExpr.FuncName.L != ast.Ifnull { return expr - } colRef, isColRef := exprChildren[0].(*expression.Column) if !isColRef {