expression, planner: do not eliminate the innermost NOT when push down not (#16451)

Author: XuHuaiyu

expression/builtin_control.go

@@ -174,7 +174,7 @@ func (c *caseWhenFunctionClass) getFunction(ctx sessionctx.Context, args []Expre
 	}
 	argTps := make([]types.EvalType, 0, l)
 	for i := 0; i < l-1; i += 2 {
-		if args[i], err = wrapWithIsTrue(ctx, true, args[i], false); err != nil {
+		if args[i], err = wrapWithIsTrue(ctx, true, args[i]); err != nil {
 			return nil, err
 		}
 		argTps = append(argTps, types.ETInt, tp)
@@ -474,7 +474,7 @@ func (c *ifFunctionClass) getFunction(ctx sessionctx.Context, args []Expression)
 	}
 	retTp := InferType4ControlFuncs(args[1].GetType(), args[2].GetType())
 	evalTps := retTp.EvalType()
-	args[0], err = wrapWithIsTrue(ctx, true, args[0], false)
+	args[0], err = wrapWithIsTrue(ctx, true, args[0])
 	if err != nil {
 		return nil, err
 	}

expression/builtin_op.go

@@ -67,11 +67,11 @@ func (c *logicAndFunctionClass) getFunction(ctx sessionctx.Context, args []Expre
 	if err != nil {
 		return nil, err
 	}
-	args[0], err = wrapWithIsTrue(ctx, true, args[0], false)
+	args[0], err = wrapWithIsTrue(ctx, true, args[0])
 	if err != nil {
 		return nil, errors.Trace(err)
 	}
-	args[1], err = wrapWithIsTrue(ctx, true, args[1], false)
+	args[1], err = wrapWithIsTrue(ctx, true, args[1])
 	if err != nil {
 		return nil, errors.Trace(err)
 	}
@@ -117,11 +117,11 @@ func (c *logicOrFunctionClass) getFunction(ctx sessionctx.Context, args []Expres
 	if err != nil {
 		return nil, err
 	}
-	args[0], err = wrapWithIsTrue(ctx, true, args[0], false)
+	args[0], err = wrapWithIsTrue(ctx, true, args[0])
 	if err != nil {
 		return nil, errors.Trace(err)
 	}
-	args[1], err = wrapWithIsTrue(ctx, true, args[1], false)
+	args[1], err = wrapWithIsTrue(ctx, true, args[1])
 	if err != nil {
 		return nil, errors.Trace(err)
 	}

expression/expression.go

@@ -388,17 +388,9 @@ func IsBinaryLiteral(expr Expression) bool {
 // The `keepNull` controls what the istrue function will return when `arg` is null:
 // 1. keepNull is true and arg is null, the istrue function returns null.
 // 2. keepNull is false and arg is null, the istrue function returns 0.
-// The `wrapForInt` indicates whether we need to wrapIsTrue for non-logical Expression with int type.
-func wrapWithIsTrue(ctx sessionctx.Context, keepNull bool, arg Expression, wrapForInt bool) (Expression, error) {
+func wrapWithIsTrue(ctx sessionctx.Context, keepNull bool, arg Expression) (Expression, error) {
 	if arg.GetType().EvalType() == types.ETInt {
-		if !wrapForInt {
-			return arg, nil
-		}
-		if child, ok := arg.(*ScalarFunction); ok {
-			if _, isLogicalOp := logicalOps[child.FuncName.L]; isLogicalOp {
-				return arg, nil
-			}
-		}
+		return arg, nil
 	}
 	fc := &isTrueOrFalseFunctionClass{baseFunctionClass{ast.IsTruth, 1, 1}, opcode.IsTruth, keepNull}
 	f, err := fc.getFunction(ctx, []Expression{arg})

expression/util.go

@@ -328,16 +328,22 @@ func pushNotAcrossExpr(ctx sessionctx.Context, expr Expression, not bool) (_ Exp
 	if f, ok := expr.(*ScalarFunction); ok {
 		switch f.FuncName.L {
 		case ast.UnaryNot:
-			child, err := wrapWithIsTrue(ctx, true, f.GetArgs()[0], true)
-			if err != nil {
-				return expr, false
-			}
 			var childExpr Expression
-			childExpr, changed = pushNotAcrossExpr(f.GetCtx(), child, !not)
-			if !changed && !not {
+			// UnaryNot only returns 0/1/NULL, thus we should not eliminate the
+			// innermost NOT if the arg is not a logical operator.
+			switch child := f.GetArgs()[0].(type) {
+			case *ScalarFunction:
+				if _, isLogicalOp := logicalOps[child.FuncName.L]; !isLogicalOp {
+					return expr, false
+				}
+				childExpr, changed = pushNotAcrossExpr(f.GetCtx(), f.GetArgs()[0], !not)
+				if !changed && !not {
+					return expr, false
+				}
+				return childExpr, true
+			case *Column:
 				return expr, false
 			}
-			return childExpr, true
 		case ast.LT, ast.GE, ast.GT, ast.LE, ast.EQ, ast.NE:
 			if not {
 				return NewFunctionInternal(f.GetCtx(), oppositeOp[f.FuncName.L], f.GetType(), f.GetArgs()...), true

expression/util_test.go

@@ -127,33 +127,33 @@ func (s *testUtilSuite) TestPushDownNot(c *check.C) {
 	c.Assert(notFunc.Equal(ctx, notFuncCopy), check.IsTrue)
 
 	// issue 15725
-	// (not not a) should be optimized to (a is true)
+	// (not not a) should not be optimized to (a)
 	notFunc = newFunction(ast.UnaryNot, col)
 	notFunc = newFunction(ast.UnaryNot, notFunc)
 	ret = PushDownNot(ctx, notFunc)
-	c.Assert(ret.Equal(ctx, newFunction(ast.IsTruth, col)), check.IsTrue)
+	c.Assert(ret.Equal(ctx, col), check.IsFalse)
 
-	// (not not (a+1)) should be optimized to (a+1 is true)
+	// (not not (a+1)) should not be optimized to (a+1)
 	plusFunc := newFunction(ast.Plus, col, One)
 	notFunc = newFunction(ast.UnaryNot, plusFunc)
 	notFunc = newFunction(ast.UnaryNot, notFunc)
 	ret = PushDownNot(ctx, notFunc)
-	c.Assert(ret.Equal(ctx, newFunction(ast.IsTruth, plusFunc)), check.IsTrue)
+	c.Assert(ret.Equal(ctx, col), check.IsFalse)
 
-	// (not not not a) should be optimized to (not (a is true))
+	// (not not not a) should be optimized to (not a)
 	notFunc = newFunction(ast.UnaryNot, col)
 	notFunc = newFunction(ast.UnaryNot, notFunc)
 	notFunc = newFunction(ast.UnaryNot, notFunc)
 	ret = PushDownNot(ctx, notFunc)
-	c.Assert(ret.Equal(ctx, newFunction(ast.UnaryNot, newFunction(ast.IsTruth, col))), check.IsTrue)
+	c.Assert(ret.Equal(ctx, newFunction(ast.UnaryNot, col)), check.IsTrue)
 
-	// (not not not not a) should be optimized to (a is true)
+	// (not not not not a) should be optimized to (not not a)
 	notFunc = newFunction(ast.UnaryNot, col)
 	notFunc = newFunction(ast.UnaryNot, notFunc)
 	notFunc = newFunction(ast.UnaryNot, notFunc)
 	notFunc = newFunction(ast.UnaryNot, notFunc)
 	ret = PushDownNot(ctx, notFunc)
-	c.Assert(ret.Equal(ctx, newFunction(ast.IsTruth, col)), check.IsTrue)
+	c.Assert(ret.Equal(ctx, newFunction(ast.UnaryNot, newFunction(ast.UnaryNot, col))), check.IsTrue)
 }
 
 func (s *testUtilSuite) TestFilter(c *check.C) {

planner/core/integration_test.go

@@ -320,3 +320,14 @@ func (s *testIntegrationSuite) TestIssue15813(c *C) {
 	tk.MustExec("CREATE INDEX i0 ON t1(c0)")
 	tk.MustQuery("select /*+ MERGE_JOIN(t0, t1) */ * from t0, t1 where t0.c0 = t1.c0").Check(testkit.Rows())
 }
+
+func (s *testIntegrationSuite) TestIssue16440(c *C) {
+	tk := testkit.NewTestKit(c, s.store)
+
+	tk.MustExec("use test")
+	tk.MustExec("drop table if exists t1")
+	tk.MustExec("create table t1(c0 int)")
+	tk.MustExec("INSERT INTO t1(c0) VALUES (NULL);")
+	tk.MustQuery("SELECT t1.c0 FROM t1 WHERE NOT t1.c0;").Check(testkit.Rows())
+	tk.MustExec("drop table t1")
+}

planner/core/testdata/plan_suite_out.json

@@ -687,7 +687,7 @@
       },
       {
         "SQL": "select a from t where not a",
-        "Best": "TableReader(Table(t))"
+        "Best": "IndexReader(Index(t.c_d_e)[[NULL,+inf]]->Sel([not(test.t.a)]))"
       },
       {
         "SQL": "select a from t where c in (1)",