expression, plan: aggressive constant fold for null parameter expression to simplify outer join #7696

eurekaka · 2018-09-14T05:48:09Z

What problem does this PR solve?

Currently, foldConstant only handles expressions like constant op constant, for expressions like constant op column, it does nothing. Actually, if constant in constant op column is null, for a lot of operators, this expression can be folded into null. This PR works on this enhancement.

What is changed and how it works?

Substitute expression in constant op expression to constant One if the constant is null, and evaluate the new expression. If the result is null, we can fold this expression, otherwise, keep the original expression.

Check List

Tests

Unit test
Integration test

Side effects

Increased code complexity

only do null fold in null reject check

plan cache test should pass now, reopen it

… inner join now

…o inner join now

eurekaka · 2018-09-14T15:47:21Z

/run-all-tests

eurekaka · 2018-09-15T16:11:18Z

/run-all-tests

eurekaka · 2018-09-20T02:56:01Z

@zz-jason @XuHuaiyu @winoros PTAL

winoros · 2018-09-20T04:27:27Z

expression/constant_fold.go

+			if !hasNullArg || !sc.InNullRejectCheck {
+				return expr, isDeferredConst
+			}
+			dummyScalarFunc, _ := x.Clone().(*ScalarFunction)


Can we use NewFunction instead?

winoros · 2018-09-20T04:32:40Z

expression/constant_fold.go

-		if !canFold {
-			return expr, isDeferredConst
+		if len(nonConstArgIdx) > 0 {
+			if !hasNullArg || !sc.InNullRejectCheck {


Do we need to check whether it's in null-reject?

Yes we need, for cases like select null and a < 1 from t;, if we fold null constant for the projection part, the result would be null for all rows, but actually for t.a >= 1, the result should be 0

fold null if result is null or false

winoros

lgtm

XuHuaiyu · 2018-09-20T08:16:53Z

sessionctx/stmtctx/stmtctx.go

@@ -60,6 +60,7 @@ type StatementContext struct {
 	UseCache               bool
 	PadCharToFullLength    bool
 	BatchCheck             bool
+	InNullRejectCheck      bool


Can we make this an input parameter for foldConstant?

If we make it a parameter of foldConstant, we have to modify signature of FoldConstant, NewFunction, NewFunctionInternal, EvaluateExprWithNull, this would lead to a lot of unnecessary code changes

XuHuaiyu · 2018-09-20T08:20:02Z

expression/constant_fold.go

+				if argIsConst[i] {
+					constArgs[i] = arg
+				} else {
+					constArgs[i] = One


Would it be incorrect if the ScalarFunction is NullEQ?

is null and is not null should have only one parameter, so they would not fall into code path of this patch IMHO.

What I mentioned here is the operator <=>.

Oh you are right, will fix this, thanks.

XuHuaiyu · 2018-09-20T08:25:54Z

plan/logical_plan_test.go

@@ -548,8 +547,8 @@ func (s *testPlanSuite) TestOuterWherePredicatePushDown(c *C) {
 	}{
 		// issue #7628, left join with where condition
 		{
-			sql:   "select * from t as t1 left join t as t2 on t1.b = t2.b where (t1.a=1 and t2.a=1) or (t1.a=2 and t2.a=2)",
-			sel:   "[or(and(eq(t1.a, 1), eq(t2.a, 1)), and(eq(t1.a, 2), eq(t2.a, 2)))]",
+			sql:   "select * from t as t1 left join t as t2 on t1.b = t2.b where (t1.a=1 and t2.a is null) or (t1.a=2 and t2.a=2)",


Why change this?

The purpose of this test case is to verify predicate pushdown over outer join, after applying this PR, the outer join would be converted to inner join now, so I changed the where condition to keep the outer join.

eurekaka · 2018-09-20T08:51:55Z

/run-all-tests

zz-jason · 2018-09-20T08:46:38Z

expression/constant_fold.go

-			if !conOK {
-				canFold = false
+			con, conOK := foldedArg.(*Constant)
+			if conOK {


how about:

argIsConst[i] = conOk allConstArg = allConstArg && conOk hasNullArg = hasNullArg || (conOk && con.Value.IsNull())

Maybe this is more clearer.

thanks, patch updated

zz-jason · 2018-09-20T08:56:19Z

plan/rule_predicate_push_down.go

 	if x.Value.IsNull() {
 		return true
-	} else if isTrue, err := x.Value.ToBool(sc); err != nil || isTrue == 0 {
+	} else if isTrue, err := x.Value.ToBool(sc); err == nil && isTrue == 0 {


should it be err == nil && isTrue != 0?

isTrue != 0 means the where condition is true if the inner table columns are null, so it cannot be rejected?

add unit test

eurekaka · 2018-09-21T07:09:55Z

@zz-jason @XuHuaiyu comments addressed, PTAL

zz-jason

LGTM

XuHuaiyu

LGTM

eurekaka added 15 commits September 9, 2018 10:42

expression: try to fold constant for expression with null parameter

4653f81

substitute non-Constant arg to One

b23e33d

add PruneColumns for LogicalTableDual

26cc553

disable TestPreparedNullParam for plan cache enabled setting

6260ca0

push down not first before foldConstant

ce4033e

only do null fold in null reject check

revert PruneColumns of LogicalTableDual, this would be in another PR

615d985

plan cache test should pass now, reopen it

move PushDownNot to isNullRejected to avoid stack overflow

6effd4f

update tpch explain test because left outer semi join is converted to…

fbef1a3

… inner join now

add unit test and explain test

0304e81

resolve conflicts

21f75ca

resolve conflicts

be77917

update TestOuterWherePredicatePushDown since it would be simplified t…

33190d3

…o inner join now

fix compilation error

b6825da

index out of range

2feffbe

fix index out of range

9a781b5

eurekaka added type/enhancement The issue or PR belongs to an enhancement. status/WIP component/expression labels Sep 14, 2018

eurekaka force-pushed the const_prop_simplify_outer branch from 80a48f9 to c9b6239 Compare September 15, 2018 13:04

eurekaka added the sig/planner SIG: Planner label Sep 15, 2018

eurekaka added the status/all tests passed label Sep 15, 2018

eurekaka changed the title ~~expression: try to fold constant for expression with null parameter~~ expression, plan: aggressive constant fold for null parameter expression to simplify outer join Sep 17, 2018

eurekaka removed the status/WIP label Sep 17, 2018

eurekaka force-pushed the const_prop_simplify_outer branch from f6008c6 to 0304e81 Compare September 17, 2018 08:24

winoros reviewed Sep 20, 2018

View reviewed changes

eurekaka added 2 commits September 20, 2018 15:49

Merge branch 'master' into const_prop_simplify_outer

fb05e5a

use NewFunctionInternal instead of Clone

210f8d5

fold null if result is null or false

winoros reviewed Sep 20, 2018

View reviewed changes

XuHuaiyu reviewed Sep 20, 2018

View reviewed changes

Merge branch 'master' into const_prop_simplify_outer

14826cd

zz-jason reviewed Sep 20, 2018

View reviewed changes

eurekaka added 4 commits September 20, 2018 20:10

Merge branch 'master' into const_prop_simplify_outer

84b357c

address comments

33c620a

Merge branch 'master' into const_prop_simplify_outer

a6302bd

do not fold null for nulleq function

d4a3200

add unit test

zz-jason reviewed Sep 21, 2018

View reviewed changes

Merge branch 'master' into const_prop_simplify_outer

c900e9c

zz-jason added the status/LGT1 Indicates that a PR has LGTM 1. label Sep 21, 2018

eurekaka added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Sep 21, 2018

XuHuaiyu approved these changes Sep 21, 2018

View reviewed changes

Merge branch 'master' into const_prop_simplify_outer

8bbcdfe

zz-jason merged commit 7b4686d into pingcap:master Sep 23, 2018

eurekaka deleted the const_prop_simplify_outer branch September 25, 2018 01:52

eurekaka mentioned this pull request Sep 27, 2018

plan: propagate constant over outer join #7794

Merged

eurekaka mentioned this pull request Oct 16, 2018

DeferredExpr is lost in constant folding #7914

Closed

zz-jason mentioned this pull request Apr 18, 2019

planner: fix stack overflow caused by folding constant #10174

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

expression, plan: aggressive constant fold for null parameter expression to simplify outer join #7696

expression, plan: aggressive constant fold for null parameter expression to simplify outer join #7696

eurekaka commented Sep 14, 2018 •

edited

Loading

eurekaka commented Sep 14, 2018

eurekaka commented Sep 15, 2018

eurekaka commented Sep 20, 2018

winoros Sep 20, 2018

eurekaka Sep 20, 2018

winoros Sep 20, 2018

eurekaka Sep 20, 2018

winoros left a comment

XuHuaiyu Sep 20, 2018

eurekaka Sep 20, 2018

XuHuaiyu Sep 20, 2018

eurekaka Sep 20, 2018

XuHuaiyu Sep 21, 2018

eurekaka Sep 21, 2018

XuHuaiyu Sep 20, 2018

eurekaka Sep 20, 2018

eurekaka commented Sep 20, 2018

zz-jason Sep 20, 2018

eurekaka Sep 20, 2018

zz-jason Sep 20, 2018

eurekaka Sep 20, 2018

eurekaka commented Sep 21, 2018

zz-jason left a comment

XuHuaiyu left a comment

expression, plan: aggressive constant fold for null parameter expression to simplify outer join #7696

expression, plan: aggressive constant fold for null parameter expression to simplify outer join #7696

Conversation

eurekaka commented Sep 14, 2018 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

eurekaka commented Sep 14, 2018

eurekaka commented Sep 15, 2018

eurekaka commented Sep 20, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eurekaka commented Sep 20, 2018

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

eurekaka commented Sep 21, 2018

zz-jason left a comment

Choose a reason for hiding this comment

XuHuaiyu left a comment

Choose a reason for hiding this comment

eurekaka commented Sep 14, 2018 •

edited

Loading