fixes bugs around expression precedence and LIKE (#16934)

Signed-off-by: Andres Taylor <[email protected]> Signed-off-by: Manan Gupta <[email protected]> Co-authored-by: Manan Gupta <[email protected]>
vitessio · Oct 14, 2024 · 721ebbd · 721ebbd
1 parent 654d248
commit 721ebbd
Show file tree

Hide file tree

Showing 16 changed files with 87 additions and 65 deletions.
diff --git a/go/test/endtoend/vtgate/vitess_tester/expressions/expressions.test b/go/test/endtoend/vtgate/vitess_tester/expressions/expressions.test
@@ -0,0 +1,30 @@
+# This file contains queries that test expressions in Vitess.
+# We've found a number of bugs around precedences that we want to test.
+CREATE TABLE t0
+(
+    c1 BIT,
+    INDEX idx_c1 (c1)
+);
+
+INSERT INTO t0(c1)
+VALUES ('');
+
+
+SELECT *
+FROM t0;
+
+SELECT ((t0.c1 = 'a'))
+FROM t0;
+
+SELECT *
+FROM t0
+WHERE ((t0.c1 = 'a'));
+
+
+SELECT (1 LIKE ('a' IS NULL));
+SELECT (NOT (1 LIKE ('a' IS NULL)));
+
+SELECT (~ (1 || 0)) IS NULL;
+
+SELECT 1
+WHERE (~ (1 || 0)) IS NULL;
diff --git a/go/vt/sqlparser/analyzer.go b/go/vt/sqlparser/analyzer.go
@@ -137,7 +137,7 @@ func ASTToStatementType(stmt Statement) StatementType {
 // CanNormalize takes Statement and returns if the statement can be normalized.
 func CanNormalize(stmt Statement) bool {
 	switch stmt.(type) {
-	case *Select, *Union, *Insert, *Update, *Delete, *Set, *CallProc, *Stream: // TODO: we could merge this logic into ASTrewriter
+	case *Select, *Union, *Insert, *Update, *Delete, *Set, *CallProc, *Stream, *VExplainStmt: // TODO: we could merge this logic into ASTrewriter
 		return true
 	}
 	return false

diff --git a/go/vt/sqlparser/ast_funcs.go b/go/vt/sqlparser/ast_funcs.go
@@ -1435,10 +1435,6 @@ func (op BinaryExprOperator) ToString() string {
 		return ShiftLeftStr
 	case ShiftRightOp:
 		return ShiftRightStr
-	case JSONExtractOp:
-		return JSONExtractOpStr
-	case JSONUnquoteExtractOp:
-		return JSONUnquoteExtractOpStr
 	default:
 		return "Unknown BinaryExprOperator"
 	}

diff --git a/go/vt/sqlparser/constants.go b/go/vt/sqlparser/constants.go
@@ -155,19 +155,17 @@ const (
 	IsNotFalseStr = "is not false"
 
 	// BinaryExpr.Operator
-	BitAndStr               = "&"
-	BitOrStr                = "|"
-	BitXorStr               = "^"
-	PlusStr                 = "+"
-	MinusStr                = "-"
-	MultStr                 = "*"
-	DivStr                  = "/"
-	IntDivStr               = "div"
-	ModStr                  = "%"
-	ShiftLeftStr            = "<<"
-	ShiftRightStr           = ">>"
-	JSONExtractOpStr        = "->"
-	JSONUnquoteExtractOpStr = "->>"
+	BitAndStr     = "&"
+	BitOrStr      = "|"
+	BitXorStr     = "^"
+	PlusStr       = "+"
+	MinusStr      = "-"
+	MultStr       = "*"
+	DivStr        = "/"
+	IntDivStr     = "div"
+	ModStr        = "%"
+	ShiftLeftStr  = "<<"
+	ShiftRightStr = ">>"
 
 	// UnaryExpr.Operator
 	UPlusStr    = "+"
@@ -718,8 +716,6 @@ const (
 	ModOp
 	ShiftLeftOp
 	ShiftRightOp
-	JSONExtractOp
-	JSONUnquoteExtractOp
 )
 
 // Constant for Enum Type - UnaryExprOperator

diff --git a/go/vt/sqlparser/parse_test.go b/go/vt/sqlparser/parse_test.go
@@ -994,9 +994,11 @@ var (
 	}, {
 		input: "select /* u~ */ 1 from t where a = ~b",
 	}, {
-		input: "select /* -> */ a.b -> 'ab' from t",
+		input:  "select /* -> */ a.b -> 'ab' from t",
+		output: "select /* -> */ json_extract(a.b, 'ab') from t",
 	}, {
-		input: "select /* -> */ a.b ->> 'ab' from t",
+		input:  "select /* -> */ a.b ->> 'ab' from t",
+		output: "select /* -> */ json_unquote(json_extract(a.b, 'ab')) from t",
 	}, {
 		input: "select /* empty function */ 1 from t where a = b()",
 	}, {
@@ -5675,7 +5677,7 @@ partition by range (YEAR(purchased)) subpartition by hash (TO_DAYS(purchased))
 		},
 		{
 			input:  "create table t (id int, info JSON, INDEX zips((CAST(info->'$.field' AS unsigned ARRAY))))",
-			output: "create table t (\n\tid int,\n\tinfo JSON,\n\tINDEX zips ((cast(info -> '$.field' as unsigned array)))\n)",
+			output: "create table t (\n\tid int,\n\tinfo JSON,\n\tINDEX zips ((cast(json_extract(info, '$.field') as unsigned array)))\n)",
 		},
 	}
 	for _, test := range createTableQueries {

diff --git a/go/vt/sqlparser/precedence.go b/go/vt/sqlparser/precedence.go
@@ -58,10 +58,7 @@ func precedenceFor(in Expr) Precendence {
 	case *BetweenExpr:
 		return P12
 	case *ComparisonExpr:
-		switch node.Operator {
-		case EqualOp, NotEqualOp, GreaterThanOp, GreaterEqualOp, LessThanOp, LessEqualOp, LikeOp, InOp, RegexpOp, NullSafeEqualOp:
-			return P11
-		}
+		return P11
 	case *IsExpr:
 		return P11
 	case *BinaryExpr:

diff --git a/go/vt/sqlparser/precedence_test.go b/go/vt/sqlparser/precedence_test.go
@@ -156,6 +156,8 @@ func TestParens(t *testing.T) {
 		{in: "(10 - 2) - 1", expected: "10 - 2 - 1"},
 		{in: "10 - (2 - 1)", expected: "10 - (2 - 1)"},
 		{in: "0 <=> (1 and 0)", expected: "0 <=> (1 and 0)"},
+		{in: "1 not like ('a' is null)", expected: "1 not like ('a' is null)"},
+		{in: ":vtg1 not like (:vtg2 is null)", expected: ":vtg1 not like (:vtg2 is null)"},
 	}
 
 	for _, tc := range tests {

diff --git a/go/vt/sqlparser/sql.go b/go/vt/sqlparser/sql.go
diff --git a/go/vt/sqlparser/sql.y b/go/vt/sqlparser/sql.y
@@ -5485,11 +5485,11 @@ function_call_keyword
   }
 | column_name_or_offset JSON_EXTRACT_OP text_literal_or_arg
   {
-	$$ = &BinaryExpr{Left: $1, Operator: JSONExtractOp, Right: $3}
+    $$ = &JSONExtractExpr{JSONDoc: $1, PathList: []Expr{$3}}
   }
 | column_name_or_offset JSON_UNQUOTE_EXTRACT_OP text_literal_or_arg
   {
-	$$ = &BinaryExpr{Left: $1, Operator: JSONUnquoteExtractOp, Right: $3}
+    $$ = &JSONUnquoteExpr{JSONValue: &JSONExtractExpr{JSONDoc: $1, PathList: []Expr{$3}}}
   }
 
 column_names_opt_paren:

diff --git a/go/vt/sqlparser/tracked_buffer_test.go b/go/vt/sqlparser/tracked_buffer_test.go
@@ -270,7 +270,7 @@ func TestCanonicalOutput(t *testing.T) {
 		},
 		{
 			"create table t (id int, info JSON, INDEX zips((CAST(info->'$.field' AS unsigned array))))",
-			"CREATE TABLE `t` (\n\t`id` int,\n\t`info` JSON,\n\tINDEX `zips` ((CAST(`info` -> '$.field' AS unsigned array)))\n)",
+			"CREATE TABLE `t` (\n\t`id` int,\n\t`info` JSON,\n\tINDEX `zips` ((CAST(JSON_EXTRACT(`info`, '$.field') AS unsigned array)))\n)",
 		},
 		{
 			"select 1 from t1 into outfile 'test/t1.txt'",

diff --git a/go/vt/vtgate/evalengine/expr.go b/go/vt/vtgate/evalengine/expr.go
@@ -56,7 +56,7 @@ func (expr *BinaryExpr) arguments(env *ExpressionEnv) (eval, eval, error) {
 	}
 	right, err := expr.Right.eval(env)
 	if err != nil {
-		return nil, nil, err
+		return left, nil, err
 	}
 	return left, right, nil
 }
diff --git a/go/vt/vtgate/evalengine/expr_compare.go b/go/vt/vtgate/evalengine/expr_compare.go
@@ -578,13 +578,18 @@ func (l *LikeExpr) matchWildcard(left, right []byte, coll collations.ID) bool {
 	}
 	fullColl := colldata.Lookup(coll)
 	wc := fullColl.Wildcard(right, 0, 0, 0)
-	return wc.Match(left)
+	return wc.Match(left) == !l.Negate
 }
 
 func (l *LikeExpr) eval(env *ExpressionEnv) (eval, error) {
-	left, right, err := l.arguments(env)
-	if left == nil || right == nil || err != nil {
-		return nil, err
+	left, err := l.Left.eval(env)
+	if err != nil || left == nil {
+		return left, err
+	}
+
+	right, err := l.Right.eval(env)
+	if err != nil || right == nil {
+		return right, err
 	}
 
 	var col collations.TypedCollation
@@ -593,18 +598,9 @@ func (l *LikeExpr) eval(env *ExpressionEnv) (eval, error) {
 		return nil, err
 	}
 
-	var matched bool
-	switch {
-	case typeIsTextual(left.SQLType()) && typeIsTextual(right.SQLType()):
-		matched = l.matchWildcard(left.(*evalBytes).bytes, right.(*evalBytes).bytes, col.Collation)
-	case typeIsTextual(right.SQLType()):
-		matched = l.matchWildcard(left.ToRawBytes(), right.(*evalBytes).bytes, col.Collation)
-	case typeIsTextual(left.SQLType()):
-		matched = l.matchWildcard(left.(*evalBytes).bytes, right.ToRawBytes(), col.Collation)
-	default:
-		matched = l.matchWildcard(left.ToRawBytes(), right.ToRawBytes(), collations.CollationBinaryID)
-	}
-	return newEvalBool(matched == !l.Negate), nil
+	matched := l.matchWildcard(left.ToRawBytes(), right.ToRawBytes(), col.Collation)
+
+	return newEvalBool(matched), nil
 }
 
 // typeof implements the ComparisonOp interface
@@ -620,12 +616,14 @@ func (expr *LikeExpr) compile(c *compiler) (ctype, error) {
 		return ctype{}, err
 	}
 
+	skip1 := c.compileNullCheck1(lt)
+
 	rt, err := expr.Right.compile(c)
 	if err != nil {
 		return ctype{}, err
 	}
 
-	skip := c.compileNullCheck2(lt, rt)
+	skip2 := c.compileNullCheck1(rt)
 
 	if !lt.isTextual() {
 		c.asm.Convert_xc(2, sqltypes.VarChar, c.cfg.Collation, nil)
@@ -678,6 +676,6 @@ func (expr *LikeExpr) compile(c *compiler) (ctype, error) {
 		})
 	}
 
-	c.asm.jumpDestination(skip)
+	c.asm.jumpDestination(skip1, skip2)
 	return ctype{Type: sqltypes.Int64, Col: collationNumeric, Flag: flagIsBoolean | flagNullable}, nil
 }
diff --git a/go/vt/vtgate/evalengine/testcases/cases.go b/go/vt/vtgate/evalengine/testcases/cases.go
@@ -1060,24 +1060,26 @@ func CollationOperations(yield Query) {
 }
 
 func LikeComparison(yield Query) {
-	var left = []string{
+	var left = append(inputConversions,
 		`'foobar'`, `'FOOBAR'`,
 		`'1234'`, `1234`,
 		`_utf8mb4 'foobar' COLLATE utf8mb4_0900_as_cs`,
-		`_utf8mb4 'FOOBAR' COLLATE utf8mb4_0900_as_cs`,
-	}
-	var right = append([]string{
+		`_utf8mb4 'FOOBAR' COLLATE utf8mb4_0900_as_cs`)
+
+	var right = append(left,
+		`NULL`, `1`, `0`,
 		`'foo%'`, `'FOO%'`, `'foo_ar'`, `'FOO_AR'`,
 		`'12%'`, `'12_4'`,
 		`_utf8mb4 'foo%' COLLATE utf8mb4_0900_as_cs`,
 		`_utf8mb4 'FOO%' COLLATE utf8mb4_0900_as_cs`,
 		`_utf8mb4 'foo_ar' COLLATE utf8mb4_0900_as_cs`,
-		`_utf8mb4 'FOO_AR' COLLATE utf8mb4_0900_as_cs`,
-	}, left...)
+		`_utf8mb4 'FOO_AR' COLLATE utf8mb4_0900_as_cs`)
 
 	for _, lhs := range left {
 		for _, rhs := range right {
-			yield(fmt.Sprintf("%s LIKE %s", lhs, rhs), nil)
+			for _, op := range []string{"LIKE", "NOT LIKE"} {
+				yield(fmt.Sprintf("%s %s %s", lhs, op, rhs), nil)
+			}
 		}
 	}
 }

diff --git a/go/vt/vtgate/evalengine/translate.go b/go/vt/vtgate/evalengine/translate.go
@@ -84,7 +84,7 @@ func (ast *astCompiler) translateComparisonExpr2(op sqlparser.ComparisonExprOper
 			Negate: op == sqlparser.NotRegexpOp,
 		}, nil
 	default:
-		return nil, vterrors.Errorf(vtrpcpb.Code_UNIMPLEMENTED, op.ToString())
+		return nil, vterrors.New(vtrpcpb.Code_UNIMPLEMENTED, op.ToString())
 	}
 }
 
@@ -298,10 +298,6 @@ func (ast *astCompiler) translateBinaryExpr(binary *sqlparser.BinaryExpr) (Expr,
 		return &BitwiseExpr{BinaryExpr: binaryExpr, Op: &opBitShl{}}, nil
 	case sqlparser.ShiftRightOp:
 		return &BitwiseExpr{BinaryExpr: binaryExpr, Op: &opBitShr{}}, nil
-	case sqlparser.JSONExtractOp:
-		return builtinJSONExtractRewrite(left, right)
-	case sqlparser.JSONUnquoteExtractOp:
-		return builtinJSONExtractUnquoteRewrite(left, right)
 	default:
 		return nil, translateExprNotSupported(binary)
 	}

diff --git a/go/vt/vtgate/planbuilder/testdata/select_cases.json b/go/vt/vtgate/planbuilder/testdata/select_cases.json
@@ -2800,8 +2800,8 @@
           "Name": "user",
           "Sharded": true
         },
-        "FieldQuery": "select a -> '$[4]', a ->> '$[3]' from `user` where 1 != 1",
-        "Query": "select a -> '$[4]', a ->> '$[3]' from `user`",
+        "FieldQuery": "select json_extract(a, '$[4]'), json_unquote(json_extract(a, '$[3]')) from `user` where 1 != 1",
+        "Query": "select json_extract(a, '$[4]'), json_unquote(json_extract(a, '$[3]')) from `user`",
         "Table": "`user`"
       },
       "TablesUsed": [

diff --git a/go/vt/vtgate/semantics/early_rewriter_test.go b/go/vt/vtgate/semantics/early_rewriter_test.go
@@ -823,6 +823,9 @@ func TestRewriteNot(t *testing.T) {
 	}, {
 		sql:      "select a from t1 where not a > 12",
 		expected: "select a from t1 where a <= 12",
+	}, {
+		sql:      "select (not (1 like ('a' is null)))",
+		expected: "select 1 not like ('a' is null) from dual",
 	}}
 	for _, tcase := range tcases {
 		t.Run(tcase.sql, func(t *testing.T) {