expression: let PushDownNot does not change the argument (#10363)

pingcap · May 6, 2019 · 0106361 · 0106361
1 parent 7e80053
commit 0106361
Show file tree

Hide file tree

Showing 5 changed files with 46 additions and 23 deletions.
diff --git a/cmd/explaintest/r/explain_easy.result b/cmd/explaintest/r/explain_easy.result
@@ -643,3 +643,20 @@ id	count	task	operator info
 Point_Get_1	1.00	root	table:t, index:i j
 rollback;
 drop table if exists t;
+create table t(a int);
+begin;
+insert into t values (1);
+explain select * from t left outer join t t1 on t.a = t1.a where t.a not between 1 and 2;
+id	count	task	operator info
+Projection_8	8320.83	root	test.t.a, test.t1.a
+└─HashLeftJoin_9	8320.83	root	left outer join, inner:UnionScan_14, equal:[eq(test.t.a, test.t1.a)]
+  ├─UnionScan_10	6656.67	root	not(and(ge(test.t.a, 1), le(test.t.a, 2)))
+  │ └─TableReader_13	6656.67	root	data:Selection_12
+  │   └─Selection_12	6656.67	cop	or(lt(test.t.a, 1), gt(test.t.a, 2))
+  │     └─TableScan_11	10000.00	cop	table:t, range:[-inf,+inf], keep order:false, stats:pseudo
+  └─UnionScan_14	6656.67	root	not(and(ge(test.t1.a, 1), le(test.t1.a, 2))), not(isnull(test.t1.a))
+    └─TableReader_17	6656.67	root	data:Selection_16
+      └─Selection_16	6656.67	cop	not(isnull(test.t1.a)), or(lt(test.t1.a, 1), gt(test.t1.a, 2))
+        └─TableScan_15	10000.00	cop	table:t, range:[-inf,+inf], keep order:false, stats:pseudo
+rollback;
+drop table if exists t;
diff --git a/cmd/explaintest/t/explain_easy.test b/cmd/explaintest/t/explain_easy.test
@@ -137,3 +137,11 @@ insert into t values (1, 1);
 explain update t set j = -j where i = 1 and j = 1;
 rollback;
 drop table if exists t;
+
+# https://github.com/pingcap/tidb/issues/10344
+create table t(a int);
+begin;
+insert into t values (1);
+explain select * from t left outer join t t1 on t.a = t1.a where t.a not between 1 and 2;
+rollback;
+drop table if exists t;
diff --git a/expression/util.go b/expression/util.go
@@ -289,6 +289,14 @@ var symmetricOp = map[opcode.Op]opcode.Op{
 	opcode.NullEQ: opcode.NullEQ,
 }
 
+func doPushDownNot(ctx sessionctx.Context, exprs []Expression, not bool) []Expression {
+	newExprs := make([]Expression, 0, len(exprs))
+	for _, expr := range exprs {
+		newExprs = append(newExprs, PushDownNot(ctx, expr, not))
+	}
+	return newExprs
+}
+
 // PushDownNot pushes the `not` function down to the expression's arguments.
 func PushDownNot(ctx sessionctx.Context, expr Expression, not bool) Expression {
 	if f, ok := expr.(*ScalarFunction); ok {
@@ -299,34 +307,22 @@ func PushDownNot(ctx sessionctx.Context, expr Expression, not bool) Expression {
 			if not {
 				return NewFunctionInternal(f.GetCtx(), oppositeOp[f.FuncName.L], f.GetType(), f.GetArgs()...)
 			}
-			for i, arg := range f.GetArgs() {
-				f.GetArgs()[i] = PushDownNot(f.GetCtx(), arg, false)
-			}
-			return f
+			newArgs := doPushDownNot(f.GetCtx(), f.GetArgs(), false)
+			return NewFunctionInternal(f.GetCtx(), f.FuncName.L, f.GetType(), newArgs...)
 		case ast.LogicAnd:
 			if not {
-				args := f.GetArgs()
-				for i, a := range args {
-					args[i] = PushDownNot(f.GetCtx(), a, true)
-				}
-				return NewFunctionInternal(f.GetCtx(), ast.LogicOr, f.GetType(), args...)
-			}
-			for i, arg := range f.GetArgs() {
-				f.GetArgs()[i] = PushDownNot(f.GetCtx(), arg, false)
+				newArgs := doPushDownNot(f.GetCtx(), f.GetArgs(), true)
+				return NewFunctionInternal(f.GetCtx(), ast.LogicOr, f.GetType(), newArgs...)
 			}
-			return f
+			newArgs := doPushDownNot(f.GetCtx(), f.GetArgs(), false)
+			return NewFunctionInternal(f.GetCtx(), f.FuncName.L, f.GetType(), newArgs...)
 		case ast.LogicOr:
 			if not {
-				args := f.GetArgs()
-				for i, a := range args {
-					args[i] = PushDownNot(f.GetCtx(), a, true)
-				}
-				return NewFunctionInternal(f.GetCtx(), ast.LogicAnd, f.GetType(), args...)
-			}
-			for i, arg := range f.GetArgs() {
-				f.GetArgs()[i] = PushDownNot(f.GetCtx(), arg, false)
+				newArgs := doPushDownNot(f.GetCtx(), f.GetArgs(), true)
+				return NewFunctionInternal(f.GetCtx(), ast.LogicAnd, f.GetType(), newArgs...)
 			}
-			return f
+			newArgs := doPushDownNot(f.GetCtx(), f.GetArgs(), false)
+			return NewFunctionInternal(f.GetCtx(), f.FuncName.L, f.GetType(), newArgs...)
 		}
 	}
 	if not {

diff --git a/expression/util_test.go b/expression/util_test.go
@@ -68,8 +68,10 @@ func (s *testUtilSuite) TestPushDownNot(c *check.C) {
 	neFunc := newFunction(ast.NE, col, One)
 	andFunc2 := newFunction(ast.LogicAnd, neFunc, neFunc)
 	orFunc2 := newFunction(ast.LogicOr, andFunc2, neFunc)
+	notFuncCopy := notFunc.Clone()
 	ret := PushDownNot(ctx, notFunc, false)
 	c.Assert(ret.Equal(ctx, orFunc2), check.IsTrue)
+	c.Assert(notFunc.Equal(ctx, notFuncCopy), check.IsTrue)
 }
 
 func (s *testUtilSuite) TestFilter(c *check.C) {

diff --git a/planner/core/logical_plan_test.go b/planner/core/logical_plan_test.go
@@ -395,7 +395,7 @@ func (s *testPlanSuite) TestSimplifyOuterJoin(c *C) {
 		},
 		{
 			sql:      "select * from t t1 left join t t2 on t1.b = t2.b where not (t1.c > 1 and t2.c > 1);",
-			best:     "Join{DataScan(t1)->DataScan(t2)}(test.t1.b,test.t2.b)->Sel([not(and(le(test.t1.c, 1), le(test.t2.c, 1)))])->Projection",
+			best:     "Join{DataScan(t1)->DataScan(t2)}(test.t1.b,test.t2.b)->Sel([not(and(gt(test.t1.c, 1), gt(test.t2.c, 1)))])->Projection",
 			joinType: "left outer join",
 		},
 		{