From 721ebbdd9a6b79db09965f27c18de0a3e9602405 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9s=20Taylor?= Date: Mon, 14 Oct 2024 11:01:19 +0200 Subject: [PATCH] fixes bugs around expression precedence and LIKE (#16934) Signed-off-by: Andres Taylor Signed-off-by: Manan Gupta Co-authored-by: Manan Gupta --- .../expressions/expressions.test | 30 ++++++++++++++++ go/vt/sqlparser/analyzer.go | 2 +- go/vt/sqlparser/ast_funcs.go | 4 --- go/vt/sqlparser/constants.go | 26 ++++++-------- go/vt/sqlparser/parse_test.go | 8 +++-- go/vt/sqlparser/precedence.go | 5 +-- go/vt/sqlparser/precedence_test.go | 2 ++ go/vt/sqlparser/sql.go | 4 +-- go/vt/sqlparser/sql.y | 4 +-- go/vt/sqlparser/tracked_buffer_test.go | 2 +- go/vt/vtgate/evalengine/expr.go | 2 +- go/vt/vtgate/evalengine/expr_compare.go | 34 +++++++++---------- go/vt/vtgate/evalengine/testcases/cases.go | 16 +++++---- go/vt/vtgate/evalengine/translate.go | 6 +--- .../planbuilder/testdata/select_cases.json | 4 +-- go/vt/vtgate/semantics/early_rewriter_test.go | 3 ++ 16 files changed, 87 insertions(+), 65 deletions(-) create mode 100644 go/test/endtoend/vtgate/vitess_tester/expressions/expressions.test diff --git a/go/test/endtoend/vtgate/vitess_tester/expressions/expressions.test b/go/test/endtoend/vtgate/vitess_tester/expressions/expressions.test new file mode 100644 index 00000000000..60c1e641463 --- /dev/null +++ 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 index f3b1fc10340..905d4deaa08 100644 --- 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 index 07c3421988b..290c57f9d97 100644 --- 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 index 1be3124aa24..bce0cb32cba 100644 --- 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 index aa007febe9a..fa189def53c 100644 --- 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 index ec590b23f95..58d5fa078ea 100644 --- 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 index a6cbffee351..d93acd4e738 100644 --- 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 index 51d3c77cd92..9ea4d0bc595 100644 --- a/go/vt/sqlparser/sql.go +++ b/go/vt/sqlparser/sql.go @@ -17840,7 +17840,7 @@ yydefault: var yyLOCAL Expr //line sql.y:5487 { - yyLOCAL = &BinaryExpr{Left: yyDollar[1].exprUnion(), Operator: JSONExtractOp, Right: yyDollar[3].exprUnion()} + yyLOCAL = &JSONExtractExpr{JSONDoc: yyDollar[1].exprUnion(), PathList: []Expr{yyDollar[3].exprUnion()}} } yyVAL.union = yyLOCAL case 1063: @@ -17848,7 +17848,7 @@ yydefault: var yyLOCAL Expr //line sql.y:5491 { - yyLOCAL = &BinaryExpr{Left: yyDollar[1].exprUnion(), Operator: JSONUnquoteExtractOp, Right: yyDollar[3].exprUnion()} + yyLOCAL = &JSONUnquoteExpr{JSONValue: &JSONExtractExpr{JSONDoc: yyDollar[1].exprUnion(), PathList: []Expr{yyDollar[3].exprUnion()}}} } yyVAL.union = yyLOCAL case 1064: diff --git a/go/vt/sqlparser/sql.y b/go/vt/sqlparser/sql.y index a3f7a2e1a82..9cdd14cfdcf 100644 --- 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 index 6924bf11911..378e386192f 100644 --- 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 index dfa8491391e..0be178ee149 100644 --- 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 index b723609160c..efc5fdee04a 100644 --- 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 index cd52631c00c..d25415ae674 100644 --- 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 index 37a4037d21e..b379812c949 100644 --- 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 index d8f0d09a64e..016cfdb0fd1 100644 --- 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 index ba5510c5bf5..13823e5351f 100644 --- 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) {