planner: eliminate if null on non null column #7924

zhexuany · 2018-10-16T19:44:25Z

What problem does this PR solve?

This PR resolves #7918.

What is changed and how it works?

When buildProjection(), we filter ast.SelectField using certain rules. For these fields which does not pass the rules entering original logic.

details will be provided later.

Check List

Tests

the test case will be added later.

Code changes

Side effects

Possible performance regression

Related changes

zz-jason · 2018-10-17T02:21:05Z

@zhexuany please fix ci && add tests

winoros · 2018-10-17T02:55:39Z

This can only partially solve it.
ifnull is not only generated during plan building, but also generated by some optimization rule.
This can be done once we know its arg is not null. So it should actually be part of this method

tidb/expression/constant_fold.go

Line 60 in f3148da

func ifNullFoldHandler(expr *ScalarFunction) (Expression, bool) {

But we cannot integrate it since we don't maintain the column's NULL or NOT NULL information.

XuHuaiyu · 2018-10-17T02:56:18Z

planner/core/logical_plan_builder.go

+	var remainingFields []*ast.SelectField
+	fieldSet := make([]bool, len(fields))
+	for idx, field := range fields {
+		if strings.Contains(field.Text(), ast.Ifnull) {


Can we use if !bool to reduce the layers?

winoros · 2018-10-17T03:03:45Z

planner/core/logical_plan_builder.go

+		schema := expression.NewSchema(make([]*expression.Column, 0, len(remainingFields))...)
+		oldLen := 0
+		for _, field := range remainingFields {
+			newExpr, np, err := b.rewrite(field.Expr, p, mapper, true)


Do the optimization here may be a better idea.

lzmhhh123 · 2018-10-17T04:01:36Z

planner/core/logical_plan_builder.go

+					var colFound *model.ColumnInfo
+					if ds, ok := p.(*DataSource); ok {
+						colFound = model.FindColumnInfo(ds.Columns, cnExpr.Name.Name.L)
+						if mysql.HasNotNullFlag(colFound.Flag) {


Can we extract these judgement to a function? So that it has strong scalability.

zhexuany · 2018-10-17T05:38:10Z

@winoros @lzmhhh123 PTAL

zhexuany · 2018-10-17T05:46:09Z

/run-all-tests

lzmhhh123 · 2018-10-17T06:12:36Z

LGTM.

winoros

code changes lgtm

winoros · 2018-10-17T06:15:50Z

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 {


We usually use NotNull instead of NonNull.

s/NonNullColumn/NotNullCol/

winoros · 2018-10-17T06:16:59Z

planner/core/logical_plan_builder.go

@@ -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)


We can add a TODO here: Do this optimization automatically when creating new scalar function.

zz-jason · 2018-10-17T07:28:22Z

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 {


s/NonNullColumn/NotNullCol/

planner/core/logical_plan_builder.go

zz-jason · 2018-10-17T12:11:41Z

planner/core/logical_plan_builder.go

+
+		return colRef
+	}
+	scalarExpr.SetArgs(exprChildren)


this can be removed.

zz-jason · 2018-10-17T12:13:06Z

planner/core/logical_plan_builder.go

+		exprChildren[i] = eliminateIfNullOnNotNullCol(p, exprChildren[i])
+	}
+
+	if scalarExpr.FuncName.L == ast.Ifnull {


how about:

if scalarExpr.FuncName.L != ast.Ifnull { return expr }

zhexuany · 2018-10-17T13:48:50Z

@zz-jason @winkyao PTAL

zz-jason

LGTM

zz-jason · 2018-10-17T13:49:29Z

/run-all-tests

bugfix fixed pingcap#7518 expression: MySQL compatible current_user function (pingcap#7801) plan: propagate constant over outer join (pingcap#7794) - extract `outerCol = const` from join conditions and filter conditions, substitute `outerCol` in join conditions with `const`; - extract `outerCol = innerCol` from join conditions, derive new join conditions based on this column equal condition and `outerCol` related expressions in join conditions and filter conditions; util/timeutil: fix data race caused by forgetting set stats lease to 0 (pingcap#7901) stats: handle ddl event for partition table (pingcap#7903) plan: implement Operand and Pattern of cascades planner. (pingcap#7910) planner: not convert to TableDual if empty range is derived from deferred constants (pingcap#7808) plan: move projEliminate behind aggEliminate (pingcap#7909) admin: fix admin check table bug of byte compare (pingcap#7887) * admin: remove reflect deepEqual stats: fix panic caused by empty histogram (pingcap#7912) plan: fix panic caused by empty schema of LogicalTableDual (pingcap#7906) * fix drop view if exist error (pingcap#7833) executor: refine `explain analyze` (pingcap#7888) executor: add an variable to compatible with MySQL insert for OGG (pingcap#7863) expression: maintain `DeferredExpr` in aggressive constant folding. (pingcap#7915) stats: fix histogram boundaries overflow error (pingcap#7883) ddl:support the definition of `null` change to `not null` using `alter table` (pingcap#7771) * ddl:support the definition of null change to not null using alter table ddl: add check when create table with foreign key. (pingcap#7885) * ddl: add check when create table with foreign key planner: eliminate if null on non null column (pingcap#7924) executor: fix a bug in point get (pingcap#7934) planner, executor: refine ColumnPrune for LogicalUnionAll (pingcap#7930) executor: fix panic when limit is too large (pingcap#7936) ddl: add TiDB version to metrics (pingcap#7902) stats: limit the length of sample values (pingcap#7931) vendor: update tipb (pingcap#7893) planner: support the Group and GroupExpr for the cascades planner (pingcap#7917) store/tikv: log more information when other err occurs (pingcap#7948) types: fix date time parse (pingcap#7933) ddl: just print error message when ddl job is normal to calcel, to eliminate noisy log (pingcap#7875) stats: update delta info for partition table (pingcap#7947) explaintest: add explain test for partition pruning (pingcap#7505) util: move disjoint set to util package (pingcap#7950) util: add PreAlloc4Row and Insert for Chunk and List (pingcap#7916) executor: add the slow log for commit (pingcap#7951) expression: add builtin json_keys (pingcap#7776) privilege: add USAGE in `show grants` for mysql compatibility (pingcap#7955) ddl: fix invailid ddl job panic (pingcap#7940) *: move ast.NewValueExpr to standalone parser_driver package (pingcap#7952) Make the ast package get rid of the dependency of types.Datum server: allow cors http request (pingcap#7939) *: move `Statement` and `RecordSet` from ast to sqlexec package (pingcap#7970) pr suggestion update executor/aggfuncs: split unit tests to corresponding file (pingcap#7993) store/tikv: fix typo (pingcap#7990) executor, planner: clone proj schema for different children in buildProj4Union (pingcap#7999) executor: let information_schema be the first database in ShowDatabases (pingcap#7938) stats: use local feedback for partition table (pingcap#7963) executor: add unit test for aggfuncs (pingcap#7966) server: add log for binary execute statement (pingcap#7987) admin: refine admin check decoder (pingcap#7862) executor: improve wide table insert & update performance (pingcap#7935) ddl: fix reassigned partition id in `truncate table` does not take effect (pingcap#7919) fix reassigned partition id in truncate table does not take effect add changelog for 2.1.0 rc4 (pingcap#8020) *: make parser package dependency as small as possible (pingcap#7989) parser: support `:=` in the `set` syntax (pingcap#8018) According to MySQL document, `set` use the = assignment operator, but the := assignment operator is also permitted stats: garbage collect stats for partition table (pingcap#7962) docs: add the proposal for the column pool (pingcap#7988) expression: refine built-in func truncate to support uint arg (pingcap#8000) stats: support show stats for partition table (pingcap#8023) stats: update error rate for partition table (pingcap#8022) stats: fix estimation for out of range point queries (pingcap#8015) *: move parser to a separate repository (pingcap#8036) executor: fix wrong result when index join on union scan. (pingcap#8031) Do not modify Plan of dataReaderBuilder directly, because it would impact next batch of outer rows, as well as other concurrent inner workers. Instead, build a local child builder to store the child plan. planner: fix a panic of a cached prepared statement with IndexScan (pingcap#8017) *: fix the issue of executing DDL after executing SQL failure in txn (pingcap#8044) * ddl, executor: fix the issue of executing DDL after executing SQL failure in txn add unit test remove debug info add like evaluator case sensitive test ddl, domain: make schema correct after canceling jobs (pingcap#7997) unit test fix code format proposal: maintaining histograms in plan. (pingcap#7605) support _tidb_rowid for table scan range (pingcap#8047) var rename fix

zhexuany force-pushed the eliminate_if_null_on_non_null_column branch from c501899 to 293d420 Compare October 16, 2018 19:45

zhexuany added sig/planner SIG: Planner type/enhancement The issue or PR belongs to an enhancement. status/WIP labels Oct 16, 2018

XuHuaiyu changed the title ~~eliminate if null on non null column~~ planner: eliminate if null on non null column Oct 17, 2018

zhexuany force-pushed the eliminate_if_null_on_non_null_column branch from 293d420 to 3a3c360 Compare October 17, 2018 02:55

XuHuaiyu reviewed Oct 17, 2018

View reviewed changes

winoros reviewed Oct 17, 2018

View reviewed changes

zhexuany force-pushed the eliminate_if_null_on_non_null_column branch from 3a3c360 to 31c13ff Compare October 17, 2018 04:02

lzmhhh123 reviewed Oct 17, 2018

View reviewed changes

zhexuany force-pushed the eliminate_if_null_on_non_null_column branch from 31c13ff to 4ff9256 Compare October 17, 2018 04:51

zhexuany removed the status/WIP label Oct 17, 2018

lzmhhh123 added the status/LGT1 Indicates that a PR has LGTM 1. label Oct 17, 2018

winoros reviewed Oct 17, 2018

View reviewed changes

winoros added the status/all tests passed label Oct 17, 2018

zz-jason reviewed Oct 17, 2018

View reviewed changes

zhexuany force-pushed the eliminate_if_null_on_non_null_column branch from 3d1496f to 3614eb7 Compare October 17, 2018 10:16

zhexuany added 3 commits October 17, 2018 18:18

eliminate if null on non null column

8bd985f

adding explain test

4623f36

recursivly trim expression tree

7000f42

zhexuany force-pushed the eliminate_if_null_on_non_null_column branch from 3614eb7 to 7000f42 Compare October 17, 2018 10:18

zz-jason reviewed Oct 17, 2018

View reviewed changes

planner/core/logical_plan_builder.go Outdated

return colRef

}

scalarExpr.SetArgs(exprChildren)

Copy link

Member

zz-jason Oct 17, 2018

Choose a reason for hiding this comment

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

this can be removed.

zz-jason reviewed Oct 17, 2018

View reviewed changes

zhexuany added 2 commits October 17, 2018 21:46

address reviewer's comment

1e029ec

remove extra line

a0f0a7f

zz-jason approved these changes Oct 17, 2018

View reviewed changes

zz-jason added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Oct 17, 2018

Merge branch 'master' into eliminate_if_null_on_non_null_column

10ca98d

zhexuany merged commit 6e525f9 into pingcap:master Oct 17, 2018

zhexuany deleted the eliminate_if_null_on_non_null_column branch October 17, 2018 14:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

planner: eliminate if null on non null column #7924

planner: eliminate if null on non null column #7924

zhexuany commented Oct 16, 2018 •

edited

Loading

zz-jason commented Oct 17, 2018 •

edited

Loading

winoros commented Oct 17, 2018 •

edited

Loading

XuHuaiyu Oct 17, 2018

winoros Oct 17, 2018

lzmhhh123 Oct 17, 2018

zhexuany commented Oct 17, 2018

zhexuany commented Oct 17, 2018

lzmhhh123 commented Oct 17, 2018

winoros left a comment

winoros Oct 17, 2018

zz-jason Oct 17, 2018

winoros Oct 17, 2018

zz-jason Oct 17, 2018

zz-jason Oct 17, 2018

zz-jason Oct 17, 2018

zhexuany commented Oct 17, 2018

zz-jason left a comment

zz-jason commented Oct 17, 2018

planner: eliminate if null on non null column #7924

planner: eliminate if null on non null column #7924

Conversation

zhexuany commented Oct 16, 2018 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

zz-jason commented Oct 17, 2018 • edited Loading

winoros commented Oct 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhexuany commented Oct 17, 2018

zhexuany commented Oct 17, 2018

lzmhhh123 commented Oct 17, 2018

winoros left a 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

zhexuany commented Oct 17, 2018

zz-jason left a comment

Choose a reason for hiding this comment

zz-jason commented Oct 17, 2018

zhexuany commented Oct 16, 2018 •

edited

Loading

zz-jason commented Oct 17, 2018 •

edited

Loading

winoros commented Oct 17, 2018 •

edited

Loading