expression: let PushDownNot does not change the argument (pingcap#1…

…0363)
alivxxx · May 8, 2019 · f941510 · f941510
1 parent 9773b7d
commit f941510
Show file tree

Hide file tree

Showing 5 changed files with 47 additions and 23 deletions.
diff --git a/cmd/explaintest/r/explain_easy.result b/cmd/explaintest/r/explain_easy.result
@@ -470,3 +470,20 @@ Projection_4	2666.67	root	test.t.a
 └─TableReader_7	2666.67	root	data:Selection_6
   └─Selection_6	2666.67	cop	gt(test.t.a, 0)
     └─TableScan_5	3333.33	cop	table:t, range:(0,+inf], keep order:false, stats:pseudo
+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	10000.00	root	
+    └─TableReader_16	10000.00	root	data:TableScan_15
+      └─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
@@ -96,3 +96,12 @@ explain select * from t where _tidb_rowid = 0;
 explain select * from t where _tidb_rowid > 0;
 explain select a, _tidb_rowid from t where a > 0;
 explain select * from t where _tidb_rowid > 0 and a > 0;
+
+# https://github.com/pingcap/tidb/issues/10344
+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;
+rollback;
+drop table if exists t;
diff --git a/expression/util.go b/expression/util.go
@@ -273,6 +273,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 {
@@ -283,34 +291,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
@@ -622,7 +622,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",
 		},
 		{