[release-20.0] fixes bugs around expression precedence and LIKE (vite…

…ssio#16934 & vitessio#16649) (vitessio#16946)

Signed-off-by: Andres Taylor <[email protected]>
Signed-off-by: Manan Gupta <[email protected]>
Co-authored-by: Andrés Taylor <[email protected]>
Co-authored-by: Manan Gupta <[email protected]>
Co-authored-by: Manan Gupta <[email protected]>

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;

go/test/endtoend/vtgate/vitess_tester/join/join.test

@@ -76,4 +76,3 @@ from t1
          left join (select t4.col, count(*) as count from t4 group by t4.col) t3 on t3.col = t2.id
 where t1.id IN (1, 2)
 group by t2.id, t4.col;
-

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

go/vt/sqlparser/ast_funcs.go

@@ -1515,10 +1515,6 @@ func (op BinaryExprOperator) ToString() string {
 		return ShiftLeftStr
 	case ShiftRightOp:
 		return ShiftRightStr
-	case JSONExtractOp:
-		return JSONExtractOpStr
-	case JSONUnquoteExtractOp:
-		return JSONUnquoteExtractOpStr
 	default:
 		return "Unknown BinaryExprOperator"
 	}

go/vt/sqlparser/constants.go

@@ -165,19 +165,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    = "+"
@@ -741,8 +739,6 @@ const (
 	ModOp
 	ShiftLeftOp
 	ShiftRightOp
-	JSONExtractOp
-	JSONUnquoteExtractOp
 )
 
 // Constant for Enum Type - UnaryExprOperator

go/vt/sqlparser/normalizer_test.go

@@ -428,6 +428,13 @@ func TestNormalize(t *testing.T) {
 			"bv2": sqltypes.Int64BindVariable(2),
 			"bv3": sqltypes.TestBindVariable([]any{1, 2}),
 		},
+	}, {
+		in:      "SELECT 1 WHERE (~ (1||0)) IS NULL",
+		outstmt: "select :bv1 /* INT64 */ from dual where ~(:bv1 /* INT64 */ or :bv2 /* INT64 */) is null",
+		outbv: map[string]*querypb.BindVariable{
+			"bv1": sqltypes.Int64BindVariable(1),
+			"bv2": sqltypes.Int64BindVariable(0),
+		},
 	}}
 	parser := NewTestParser()
 	for _, tc := range testcases {
@@ -518,6 +525,7 @@ func TestNormalizeOneCasae(t *testing.T) {
 	err = Normalize(tree, NewReservedVars("vtg", known), bv)
 	require.NoError(t, err)
 	normalizerOutput := String(tree)
+	require.EqualValues(t, testOne.output, normalizerOutput)
 	if normalizerOutput == "otheradmin" || normalizerOutput == "otherread" {
 		return
 	}

go/vt/sqlparser/parse_test.go

@@ -1005,9 +1005,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()",
 	}, {
@@ -5867,7 +5869,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\tkey zips ((cast(info -> '$.field' as unsigned array)))\n)",
+			output: "create table t (\n\tid int,\n\tinfo JSON,\n\tkey zips ((cast(json_extract(info, '$.field') as unsigned array)))\n)",
 		},
 	}
 	parser := NewTestParser()

go/vt/sqlparser/precedence.go

@@ -38,7 +38,6 @@ const (
 	P14
 	P15
 	P16
-	P17
 )
 
 // precedenceFor returns the precedence of an expression.
@@ -58,10 +57,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:
@@ -83,7 +79,7 @@ func precedenceFor(in Expr) Precendence {
 		switch node.Operator {
 		case UPlusOp, UMinusOp:
 			return P4
-		case BangOp:
+		default:
 			return P3
 		}
 	}

go/vt/sqlparser/precedence_test.go

@@ -158,6 +158,9 @@ 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||0)) IS NULL", expected: "~(1 or 0) is null"},
+		{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)"},
 	}
 
 	parser := NewTestParser()

go/vt/sqlparser/sql.y

@@ -5513,11 +5513,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:

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\tKEY `zips` ((CAST(`info` -> '$.field' AS unsigned array)))\n)",
+			"CREATE TABLE `t` (\n\t`id` int,\n\t`info` JSON,\n\tKEY `zips` ((CAST(JSON_EXTRACT(`info`, '$.field') AS unsigned array)))\n)",
 		},
 		{
 			"select 1 from t1 into outfile 'test/t1.txt'",

go/vt/vtgate/evalengine/compiler_test.go

@@ -24,6 +24,8 @@ import (
 	"testing"
 	"time"
 
+	"github.com/stretchr/testify/assert"
+
 	"github.com/olekukonko/tablewriter"
 	"github.com/stretchr/testify/require"
 
@@ -94,7 +96,18 @@ func (s *Tracker) String() string {
 	return s.buf.String()
 }
 
+func TestOneCase(t *testing.T) {
+	query := ``
+	if query == "" {
+		t.Skip("no query to test")
+	}
+	venv := vtenv.NewTestEnv()
+	env := evalengine.EmptyExpressionEnv(venv)
+	testCompilerCase(t, query, venv, nil, env)
+}
+
 func TestCompilerReference(t *testing.T) {
+	// This test runs a lot of queries and compares the results of the evalengine in eval mode to the results of the compiler.
 	now := time.Now()
 	evalengine.SystemTime = func() time.Time { return now }
 	defer func() { evalengine.SystemTime = time.Now }()
@@ -108,52 +121,11 @@ func TestCompilerReference(t *testing.T) {
 
 			tc.Run(func(query string, row []sqltypes.Value) {
 				env.Row = row
-
-				stmt, err := venv.Parser().ParseExpr(query)
-				if err != nil {
-					// no need to test un-parseable queries
-					return
-				}
-
-				fields := evalengine.FieldResolver(tc.Schema)
-				cfg := &evalengine.Config{
-					ResolveColumn:     fields.Column,
-					ResolveType:       fields.Type,
-					Collation:         collations.CollationUtf8mb4ID,
-					Environment:       venv,
-					NoConstantFolding: true,
-				}
-
-				converted, err := evalengine.Translate(stmt, cfg)
-				if err != nil {
-					return
-				}
-
-				expected, evalErr := env.EvaluateAST(converted)
 				total++
-
-				res, vmErr := env.Evaluate(converted)
-				if vmErr != nil {
-					switch {
-					case evalErr == nil:
-						t.Errorf("failed evaluation from compiler:\nSQL:  %s\nError: %s", query, vmErr)
-					case evalErr.Error() != vmErr.Error():
-						t.Errorf("error mismatch:\nSQL:  %s\nError eval: %s\nError comp: %s", query, evalErr, vmErr)
-					default:
-						supported++
-					}
-					return
+				testCompilerCase(t, query, venv, tc.Schema, env)
+				if !t.Failed() {
+					supported++
 				}
-
-				eval := expected.String()
-				comp := res.String()
-
-				if eval != comp {
-					t.Errorf("bad evaluation from compiler:\nSQL:  %s\nEval: %s\nComp: %s", query, eval, comp)
-					return
-				}
-
-				supported++
 			})
 
 			track.Add(tc.Name(), supported, total)
@@ -163,6 +135,51 @@ func TestCompilerReference(t *testing.T) {
 	t.Logf("\n%s", track.String())
 }
 
+func testCompilerCase(t *testing.T, query string, venv *vtenv.Environment, schema []*querypb.Field, env *evalengine.ExpressionEnv) {
+	stmt, err := venv.Parser().ParseExpr(query)
+	if err != nil {
+		// no need to test un-parseable queries
+		return
+	}
+
+	fields := evalengine.FieldResolver(schema)
+	cfg := &evalengine.Config{
+		ResolveColumn:     fields.Column,
+		ResolveType:       fields.Type,
+		Collation:         collations.CollationUtf8mb4ID,
+		Environment:       venv,
+		NoConstantFolding: true,
+	}
+
+	converted, err := evalengine.Translate(stmt, cfg)
+	if err != nil {
+		return
+	}
+
+	var expected evalengine.EvalResult
+	var evalErr error
+	assert.NotPanics(t, func() {
+		expected, evalErr = env.EvaluateAST(converted)
+	})
+	var res evalengine.EvalResult
+	var vmErr error
+	assert.NotPanics(t, func() {
+		res, vmErr = env.Evaluate(converted)
+	})
+	switch {
+	case vmErr == nil && evalErr == nil:
+		eval := expected.String()
+		comp := res.String()
+		assert.Equalf(t, eval, comp, "bad evaluation from compiler:\nSQL:  %s\nEval: %s\nComp: %s", query, eval, comp)
+	case vmErr == nil:
+		t.Errorf("failed evaluation from evalengine:\nSQL:  %s\nError: %s", query, evalErr)
+	case evalErr == nil:
+		t.Errorf("failed evaluation from compiler:\nSQL:  %s\nError: %s", query, vmErr)
+	case evalErr.Error() != vmErr.Error():
+		t.Errorf("error mismatch:\nSQL:  %s\nError eval: %s\nError comp: %s", query, evalErr, vmErr)
+	}
+}
+
 func TestCompilerSingle(t *testing.T) {
 	var testCases = []struct {
 		expression string

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
 }