From e5b41352181708e0670476a824af933ae5ed2d2a Mon Sep 17 00:00:00 2001 From: Andres Taylor Date: Fri, 11 Oct 2024 08:37:37 +0200 Subject: [PATCH 01/15] fixes precedence of NOT LIKE Signed-off-by: Andres Taylor --- .../expressions/expressions.test | 31 +++++++++++++++++++ go/vt/sqlparser/analyzer.go | 2 +- go/vt/sqlparser/precedence.go | 5 +-- go/vt/sqlparser/precedence_test.go | 2 ++ 4 files changed, 35 insertions(+), 5 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..43f189119f4 --- /dev/null +++ b/go/test/endtoend/vtgate/vitess_tester/expressions/expressions.test @@ -0,0 +1,31 @@ +# 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 ea0773d99cc..98b7677a1f3 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/precedence.go b/go/vt/sqlparser/precedence.go index 6df5e8e227e..1b5576f65b1 100644 --- a/go/vt/sqlparser/precedence.go +++ b/go/vt/sqlparser/precedence.go @@ -57,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: diff --git a/go/vt/sqlparser/precedence_test.go b/go/vt/sqlparser/precedence_test.go index 0a14df5a2c1..690e6df8647 100644 --- a/go/vt/sqlparser/precedence_test.go +++ b/go/vt/sqlparser/precedence_test.go @@ -159,6 +159,8 @@ func TestParens(t *testing.T) { {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() From b7662bd76a45fa99c95761f7f9a0aac0dfa94621 Mon Sep 17 00:00:00 2001 From: Andres Taylor Date: Fri, 11 Oct 2024 09:14:32 +0200 Subject: [PATCH 02/15] update test expectations Signed-off-by: Andres Taylor --- go/vt/vtgate/executor_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/go/vt/vtgate/executor_test.go b/go/vt/vtgate/executor_test.go index 7151bf7b834..3fef43486ba 100644 --- a/go/vt/vtgate/executor_test.go +++ b/go/vt/vtgate/executor_test.go @@ -2290,7 +2290,7 @@ func TestExecutorVExplain(t *testing.T) { result, err = executorExec(ctx, executor, session, "vexplain plan select 42", bindVars) require.NoError(t, err) - expected := `[[VARCHAR("{\n\t\"OperatorType\": \"Projection\",\n\t\"Expressions\": [\n\t\t\"42 as 42\"\n\t],\n\t\"Inputs\": [\n\t\t{\n\t\t\t\"OperatorType\": \"SingleRow\"\n\t\t}\n\t]\n}")]]` + expected := `[[VARCHAR("{\n\t\"OperatorType\": \"Projection\",\n\t\"Expressions\": [\n\t\t\":vtg1 as :vtg1 /* INT64 */\"\n\t],\n\t\"Inputs\": [\n\t\t{\n\t\t\t\"OperatorType\": \"SingleRow\"\n\t\t}\n\t]\n}")]]` require.Equal(t, expected, fmt.Sprintf("%v", result.Rows)) } From fd3ecaa471981002a9f27cc33d3790a122a67af3 Mon Sep 17 00:00:00 2001 From: Andres Taylor Date: Fri, 11 Oct 2024 09:45:24 +0200 Subject: [PATCH 03/15] remove JSONExtract as binary op. Just keep it as a function call Signed-off-by: Andres Taylor --- go/vt/sqlparser/ast_funcs.go | 2 -- go/vt/sqlparser/parse_test.go | 5 +++-- go/vt/sqlparser/sql.go | 2 +- go/vt/sqlparser/sql.y | 2 +- go/vt/sqlparser/tracked_buffer_test.go | 2 +- 5 files changed, 6 insertions(+), 7 deletions(-) diff --git a/go/vt/sqlparser/ast_funcs.go b/go/vt/sqlparser/ast_funcs.go index c327947580e..c3ac50b1a10 100644 --- a/go/vt/sqlparser/ast_funcs.go +++ b/go/vt/sqlparser/ast_funcs.go @@ -1666,8 +1666,6 @@ func (op BinaryExprOperator) ToString() string { return ShiftLeftStr case ShiftRightOp: return ShiftRightStr - case JSONExtractOp: - return JSONExtractOpStr case JSONUnquoteExtractOp: return JSONUnquoteExtractOpStr default: diff --git a/go/vt/sqlparser/parse_test.go b/go/vt/sqlparser/parse_test.go index 1780344ab73..2bb2e7f2e7f 100644 --- a/go/vt/sqlparser/parse_test.go +++ b/go/vt/sqlparser/parse_test.go @@ -1005,7 +1005,8 @@ 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", }, { @@ -5937,7 +5938,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)", }, { input: "create table t (id int, s varchar(255) default 'foo\"bar')", diff --git a/go/vt/sqlparser/sql.go b/go/vt/sqlparser/sql.go index 6892c5cc586..851314b9398 100644 --- a/go/vt/sqlparser/sql.go +++ b/go/vt/sqlparser/sql.go @@ -18297,7 +18297,7 @@ yydefault: var yyLOCAL Expr //line sql.y:5593 { - 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 1081: diff --git a/go/vt/sqlparser/sql.y b/go/vt/sqlparser/sql.y index faa7a09db1a..8b6ee49ab89 100644 --- a/go/vt/sqlparser/sql.y +++ b/go/vt/sqlparser/sql.y @@ -5591,7 +5591,7 @@ 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 { diff --git a/go/vt/sqlparser/tracked_buffer_test.go b/go/vt/sqlparser/tracked_buffer_test.go index 96f2174481e..13b1363421e 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\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'", From 8d3825adcb930ad9d905f081ceb93f4e3b2bcd04 Mon Sep 17 00:00:00 2001 From: Andres Taylor Date: Fri, 11 Oct 2024 09:57:40 +0200 Subject: [PATCH 04/15] parse ->> as json_unquote(json_extract()) to simplify precendence logic Signed-off-by: Andres Taylor --- go/vt/sqlparser/ast_funcs.go | 2 -- go/vt/sqlparser/constants.go | 2 -- go/vt/sqlparser/parse_test.go | 3 ++- go/vt/sqlparser/sql.go | 2 +- go/vt/sqlparser/sql.y | 2 +- go/vt/vtgate/evalengine/translate.go | 4 ---- 6 files changed, 4 insertions(+), 11 deletions(-) diff --git a/go/vt/sqlparser/ast_funcs.go b/go/vt/sqlparser/ast_funcs.go index c3ac50b1a10..836c824010d 100644 --- a/go/vt/sqlparser/ast_funcs.go +++ b/go/vt/sqlparser/ast_funcs.go @@ -1666,8 +1666,6 @@ func (op BinaryExprOperator) ToString() string { return ShiftLeftStr case ShiftRightOp: return ShiftRightStr - case JSONUnquoteExtractOp: - return JSONUnquoteExtractOpStr default: return "Unknown BinaryExprOperator" } diff --git a/go/vt/sqlparser/constants.go b/go/vt/sqlparser/constants.go index 08538fbd749..f32c0e1e059 100644 --- a/go/vt/sqlparser/constants.go +++ b/go/vt/sqlparser/constants.go @@ -715,8 +715,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 2bb2e7f2e7f..941e19abc7c 100644 --- a/go/vt/sqlparser/parse_test.go +++ b/go/vt/sqlparser/parse_test.go @@ -1008,7 +1008,8 @@ var ( 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()", }, { diff --git a/go/vt/sqlparser/sql.go b/go/vt/sqlparser/sql.go index 851314b9398..ecfb75432e1 100644 --- a/go/vt/sqlparser/sql.go +++ b/go/vt/sqlparser/sql.go @@ -18305,7 +18305,7 @@ yydefault: var yyLOCAL Expr //line sql.y:5597 { - 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 1082: diff --git a/go/vt/sqlparser/sql.y b/go/vt/sqlparser/sql.y index 8b6ee49ab89..0881c2d0b59 100644 --- a/go/vt/sqlparser/sql.y +++ b/go/vt/sqlparser/sql.y @@ -5595,7 +5595,7 @@ function_call_keyword } | 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/vtgate/evalengine/translate.go b/go/vt/vtgate/evalengine/translate.go index 99e1508cc04..e93d338952c 100644 --- a/go/vt/vtgate/evalengine/translate.go +++ b/go/vt/vtgate/evalengine/translate.go @@ -309,10 +309,6 @@ func (ast *astCompiler) translateBinaryExpr(binary *sqlparser.BinaryExpr) (IR, e 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) } From 6d5c269e0e58ed2556ce0e1e187e6da59e3ed71a Mon Sep 17 00:00:00 2001 From: Andres Taylor Date: Fri, 11 Oct 2024 10:30:00 +0200 Subject: [PATCH 05/15] update test expectations Signed-off-by: Andres Taylor --- go/vt/vtgate/planbuilder/testdata/select_cases.json | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/go/vt/vtgate/planbuilder/testdata/select_cases.json b/go/vt/vtgate/planbuilder/testdata/select_cases.json index 856e56265ca..bd41b2f93ec 100644 --- a/go/vt/vtgate/planbuilder/testdata/select_cases.json +++ b/go/vt/vtgate/planbuilder/testdata/select_cases.json @@ -3603,8 +3603,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": [ From 46f7bd518c8a4b39d2b02027f2770ab53fc96db2 Mon Sep 17 00:00:00 2001 From: Andres Taylor Date: Fri, 11 Oct 2024 10:35:09 +0200 Subject: [PATCH 06/15] remove unused constants Signed-off-by: Andres Taylor --- go/vt/sqlparser/constants.go | 24 +++++++++++------------- 1 file changed, 11 insertions(+), 13 deletions(-) diff --git a/go/vt/sqlparser/constants.go b/go/vt/sqlparser/constants.go index f32c0e1e059..1c8f831274c 100644 --- a/go/vt/sqlparser/constants.go +++ b/go/vt/sqlparser/constants.go @@ -169,19 +169,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 = "+" From b0b93d9e7ec79245570cb170a8f04e6765c6932c Mon Sep 17 00:00:00 2001 From: Andres Taylor Date: Fri, 11 Oct 2024 10:56:09 +0200 Subject: [PATCH 07/15] add more tests for LIKE in the evalengine Signed-off-by: Andres Taylor --- .../expressions/expressions.test | 36 +++---------------- go/vt/vtgate/evalengine/testcases/cases.go | 8 ++--- go/vt/vtgate/semantics/early_rewriter_test.go | 3 ++ 3 files changed, 12 insertions(+), 35 deletions(-) diff --git a/go/test/endtoend/vtgate/vitess_tester/expressions/expressions.test b/go/test/endtoend/vtgate/vitess_tester/expressions/expressions.test index 43f189119f4..1c22dfe1fea 100644 --- a/go/test/endtoend/vtgate/vitess_tester/expressions/expressions.test +++ b/go/test/endtoend/vtgate/vitess_tester/expressions/expressions.test @@ -1,31 +1,5 @@ -# 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; +select (not (1 like ('a' is null))); +select not (1 like ('a' is null)); +select 1 not like ('a' is null); +select (not (1 like 0)); +select 1 not like 0; diff --git a/go/vt/vtgate/evalengine/testcases/cases.go b/go/vt/vtgate/evalengine/testcases/cases.go index ed1c5ed1f76..7ed3af96042 100644 --- a/go/vt/vtgate/evalengine/testcases/cases.go +++ b/go/vt/vtgate/evalengine/testcases/cases.go @@ -1097,20 +1097,20 @@ 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{ + ) + var right = append(left, `'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...) + ) for _, lhs := range left { for _, rhs := range right { diff --git a/go/vt/vtgate/semantics/early_rewriter_test.go b/go/vt/vtgate/semantics/early_rewriter_test.go index 4f550d46392..1ec7786a46c 100644 --- a/go/vt/vtgate/semantics/early_rewriter_test.go +++ b/go/vt/vtgate/semantics/early_rewriter_test.go @@ -840,6 +840,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) { From 6ffb2e02433caaf6dbb6daf9d7a3cecac4439053 Mon Sep 17 00:00:00 2001 From: Andres Taylor Date: Fri, 11 Oct 2024 13:35:05 +0200 Subject: [PATCH 08/15] bugfix: two fixes for (NOT) LIKE 1. We forgot the negation for NOT LIKE in the compiled version 2. When the LHS is NULL we have to exit early and not evaluate the RHS Signed-off-by: Andres Taylor --- go/vt/vtgate/evalengine/expr_compare.go | 11 +++++++---- go/vt/vtgate/evalengine/testcases/cases.go | 12 +++++++----- 2 files changed, 14 insertions(+), 9 deletions(-) diff --git a/go/vt/vtgate/evalengine/expr_compare.go b/go/vt/vtgate/evalengine/expr_compare.go index ce1e16af787..8e7b0d0a8a4 100644 --- a/go/vt/vtgate/evalengine/expr_compare.go +++ b/go/vt/vtgate/evalengine/expr_compare.go @@ -592,7 +592,7 @@ 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) { @@ -618,7 +618,7 @@ func (l *LikeExpr) eval(env *ExpressionEnv) (eval, error) { default: matched = l.matchWildcard(left.ToRawBytes(), right.ToRawBytes(), collations.CollationBinaryID) } - return newEvalBool(matched == !l.Negate), nil + return newEvalBool(matched), nil } func (expr *LikeExpr) compile(c *compiler) (ctype, error) { @@ -627,12 +627,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.collation, nil) @@ -684,6 +686,7 @@ func (expr *LikeExpr) compile(c *compiler) (ctype, error) { }) } - c.asm.jumpDestination(skip) + c.asm.jumpDestination(skip1) + c.asm.jumpDestination(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 7ed3af96042..ff6c0c0f311 100644 --- a/go/vt/vtgate/evalengine/testcases/cases.go +++ b/go/vt/vtgate/evalengine/testcases/cases.go @@ -1101,20 +1101,22 @@ func LikeComparison(yield Query) { `'foobar'`, `'FOOBAR'`, `'1234'`, `1234`, `_utf8mb4 'foobar' COLLATE utf8mb4_0900_as_cs`, - `_utf8mb4 'FOOBAR' COLLATE utf8mb4_0900_as_cs`, - ) + `_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`, - ) + `_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) + } } } } From ab3d00002cb058d6d1c9fd932a0f44cc47b9ebf7 Mon Sep 17 00:00:00 2001 From: Andres Taylor Date: Fri, 11 Oct 2024 14:39:36 +0200 Subject: [PATCH 09/15] bugfix: don't trust SQLTypes Signed-off-by: Andres Taylor --- go/vt/vtgate/evalengine/compiler_test.go | 15 +++++++++++++-- go/vt/vtgate/evalengine/expr_compare.go | 15 +++++++++------ 2 files changed, 22 insertions(+), 8 deletions(-) diff --git a/go/vt/vtgate/evalengine/compiler_test.go b/go/vt/vtgate/evalengine/compiler_test.go index 04eb72ad4f2..2b9eb826a89 100644 --- a/go/vt/vtgate/evalengine/compiler_test.go +++ b/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" @@ -129,10 +131,19 @@ func TestCompilerReference(t *testing.T) { return } - expected, evalErr := env.EvaluateAST(converted) + var expected evalengine.EvalResult + var evalErr error + assert.NotPanics(t, func() { + expected, evalErr = env.EvaluateAST(converted) + }) total++ - res, vmErr := env.Evaluate(converted) + var res evalengine.EvalResult + var vmErr error + assert.NotPanics(t, func() { + res, vmErr = env.Evaluate(converted) + }) + if vmErr != nil { switch { case evalErr == nil: diff --git a/go/vt/vtgate/evalengine/expr_compare.go b/go/vt/vtgate/evalengine/expr_compare.go index 8e7b0d0a8a4..ecf4c3f7684 100644 --- a/go/vt/vtgate/evalengine/expr_compare.go +++ b/go/vt/vtgate/evalengine/expr_compare.go @@ -607,14 +607,17 @@ func (l *LikeExpr) eval(env *ExpressionEnv) (eval, error) { return nil, err } + lbytes, lok := left.(*evalBytes) + rbytes, rok := right.(*evalBytes) + 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) + case lok && rok: + matched = l.matchWildcard(lbytes.bytes, rbytes.bytes, col.Collation) + case rok: + matched = l.matchWildcard(left.ToRawBytes(), rbytes.bytes, col.Collation) + case lok: + matched = l.matchWildcard(lbytes.bytes, right.ToRawBytes(), col.Collation) default: matched = l.matchWildcard(left.ToRawBytes(), right.ToRawBytes(), collations.CollationBinaryID) } From f1b52eae0835e2f9f9b1c6abd64216af3fd191b7 Mon Sep 17 00:00:00 2001 From: Andres Taylor Date: Fri, 11 Oct 2024 14:46:41 +0200 Subject: [PATCH 10/15] add end2end test with problematic queries Signed-off-by: Andres Taylor --- .../expressions/expressions.test | 35 ++++++++++++++++--- 1 file changed, 30 insertions(+), 5 deletions(-) diff --git a/go/test/endtoend/vtgate/vitess_tester/expressions/expressions.test b/go/test/endtoend/vtgate/vitess_tester/expressions/expressions.test index 1c22dfe1fea..60c1e641463 100644 --- a/go/test/endtoend/vtgate/vitess_tester/expressions/expressions.test +++ b/go/test/endtoend/vtgate/vitess_tester/expressions/expressions.test @@ -1,5 +1,30 @@ -select (not (1 like ('a' is null))); -select not (1 like ('a' is null)); -select 1 not like ('a' is null); -select (not (1 like 0)); -select 1 not like 0; +# 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; From c38c3681cda8a005b7038e7dd423e2c012b07e1c Mon Sep 17 00:00:00 2001 From: Andres Taylor Date: Fri, 11 Oct 2024 15:41:38 +0200 Subject: [PATCH 11/15] wip - another attempt at the evalengine fix Signed-off-by: Andres Taylor --- go/vt/vtgate/evalengine/compiler_test.go | 4 +- go/vt/vtgate/evalengine/expr.go | 2 +- go/vt/vtgate/evalengine/expr_compare.go | 10 +- go/vt/vtgate/evalengine/testcases/cases.go | 288 ++++++++++----------- 4 files changed, 157 insertions(+), 147 deletions(-) diff --git a/go/vt/vtgate/evalengine/compiler_test.go b/go/vt/vtgate/evalengine/compiler_test.go index 2b9eb826a89..fbef6b017a9 100644 --- a/go/vt/vtgate/evalengine/compiler_test.go +++ b/go/vt/vtgate/evalengine/compiler_test.go @@ -144,8 +144,10 @@ func TestCompilerReference(t *testing.T) { res, vmErr = env.Evaluate(converted) }) - if vmErr != nil { + if vmErr != nil || evalErr != nil { switch { + 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(): diff --git a/go/vt/vtgate/evalengine/expr.go b/go/vt/vtgate/evalengine/expr.go index 44026f97e69..b90390e1ba8 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 ecf4c3f7684..98ba5cd4462 100644 --- a/go/vt/vtgate/evalengine/expr_compare.go +++ b/go/vt/vtgate/evalengine/expr_compare.go @@ -597,10 +597,18 @@ func (l *LikeExpr) matchWildcard(left, right []byte, coll collations.ID) bool { func (l *LikeExpr) eval(env *ExpressionEnv) (eval, error) { left, right, err := l.arguments(env) - if left == nil || right == nil || err != nil { + if left == nil { + return nil, nil + } + + if err != nil { return nil, err } + if right == nil { + return nil, nil + } + var col collations.TypedCollation left, right, col, err = mergeAndCoerceCollations(left, right, env.collationEnv) if err != nil { diff --git a/go/vt/vtgate/evalengine/testcases/cases.go b/go/vt/vtgate/evalengine/testcases/cases.go index ff6c0c0f311..bbacde3013e 100644 --- a/go/vt/vtgate/evalengine/testcases/cases.go +++ b/go/vt/vtgate/evalengine/testcases/cases.go @@ -29,151 +29,151 @@ import ( ) var Cases = []TestCase{ - {Run: JSONExtract, Schema: JSONExtract_Schema}, - {Run: JSONPathOperations}, - {Run: JSONArray}, - {Run: JSONObject}, - {Run: CharsetConversionOperators}, - {Run: CaseExprWithPredicate}, - {Run: CaseExprWithValue}, - {Run: If}, - {Run: Base64}, - {Run: Conversion}, - {Run: LargeDecimals}, - {Run: LargeIntegers}, - {Run: DecimalClamping}, - {Run: BitwiseOperatorsUnary}, - {Run: BitwiseOperators}, - {Run: WeightString}, - {Run: FloatFormatting}, - {Run: UnderscoreAndPercentage}, - {Run: Types}, - {Run: Arithmetic}, - {Run: HexArithmetic}, - {Run: NumericTypes}, - {Run: NegateArithmetic}, - {Run: CollationOperations}, + // {Run: JSONExtract, Schema: JSONExtract_Schema}, + // {Run: JSONPathOperations}, + // {Run: JSONArray}, + // {Run: JSONObject}, + // {Run: CharsetConversionOperators}, + // {Run: CaseExprWithPredicate}, + // {Run: CaseExprWithValue}, + // {Run: If}, + // {Run: Base64}, + // {Run: Conversion}, + // {Run: LargeDecimals}, + // {Run: LargeIntegers}, + // {Run: DecimalClamping}, + // {Run: BitwiseOperatorsUnary}, + // {Run: BitwiseOperators}, + // {Run: WeightString}, + // {Run: FloatFormatting}, + // {Run: UnderscoreAndPercentage}, + // {Run: Types}, + // {Run: Arithmetic}, + // {Run: HexArithmetic}, + // {Run: NumericTypes}, + // {Run: NegateArithmetic}, + // {Run: CollationOperations}, {Run: LikeComparison}, - {Run: StrcmpComparison}, - {Run: MultiComparisons}, - {Run: IntervalStatement}, - {Run: IsStatement}, - {Run: NotStatement}, - {Run: LogicalStatement}, - {Run: TupleComparisons}, - {Run: Comparisons}, - {Run: InStatement}, - {Run: FnField}, - {Run: FnElt}, - {Run: FnInsert}, - {Run: FnLower}, - {Run: FnUpper}, - {Run: FnCharLength}, - {Run: FnLength}, - {Run: FnBitLength}, - {Run: FnAscii}, - {Run: FnReverse}, - {Run: FnSpace}, - {Run: FnOrd}, - {Run: FnRepeat}, - {Run: FnLeft}, - {Run: FnLpad}, - {Run: FnRight}, - {Run: FnRpad}, - {Run: FnLTrim}, - {Run: FnRTrim}, - {Run: FnTrim}, - {Run: FnSubstr}, - {Run: FnLocate}, - {Run: FnReplace}, - {Run: FnConcat}, - {Run: FnConcatWs}, - {Run: FnChar}, - {Run: FnHex}, - {Run: FnUnhex}, - {Run: FnCeil}, - {Run: FnFloor}, - {Run: FnAbs}, - {Run: FnPi}, - {Run: FnAcos}, - {Run: FnAsin}, - {Run: FnAtan}, - {Run: FnAtan2}, - {Run: FnCos}, - {Run: FnCot}, - {Run: FnSin}, - {Run: FnTan}, - {Run: FnDegrees}, - {Run: FnRadians}, - {Run: FnNow, Compare: &Comparison{LooseTime: true}}, - {Run: FnInfo}, - {Run: FnExp}, - {Run: FnLn}, - {Run: FnLog}, - {Run: FnLog10}, - {Run: FnMod}, - {Run: FnLog2}, - {Run: FnPow}, - {Run: FnSign}, - {Run: FnSqrt}, - {Run: FnRound}, - {Run: FnTruncate}, - {Run: FnCrc32}, - {Run: FnConv}, - {Run: FnBin}, - {Run: FnOct}, - {Run: FnMD5}, - {Run: FnSHA1}, - {Run: FnSHA2}, - {Run: FnRandomBytes}, - {Run: FnDateFormat}, - {Run: FnConvertTz}, - {Run: FnDate}, - {Run: FnDayOfMonth}, - {Run: FnDayOfWeek}, - {Run: FnDayOfYear}, - {Run: FnFromUnixtime}, - {Run: FnHour}, - {Run: FnMakedate}, - {Run: FnMaketime}, - {Run: FnMicroSecond}, - {Run: FnMinute}, - {Run: FnMonth}, - {Run: FnMonthName}, - {Run: FnLastDay}, - {Run: FnToDays}, - {Run: FnFromDays}, - {Run: FnSecToTime}, - {Run: FnTimeToSec}, - {Run: FnToSeconds}, - {Run: FnQuarter}, - {Run: FnSecond}, - {Run: FnTime}, - {Run: FnUnixTimestamp}, - {Run: FnWeek}, - {Run: FnWeekDay}, - {Run: FnWeekOfYear}, - {Run: FnYear}, - {Run: FnYearWeek}, - {Run: FnPeriodAdd}, - {Run: FnPeriodDiff}, - {Run: FnInetAton}, - {Run: FnInetNtoa}, - {Run: FnInet6Aton}, - {Run: FnInet6Ntoa}, - {Run: FnIsIPv4}, - {Run: FnIsIPv4Compat}, - {Run: FnIsIPv4Mapped}, - {Run: FnIsIPv6}, - {Run: FnBinToUUID}, - {Run: FnIsUUID}, - {Run: FnUUID}, - {Run: FnUUIDToBin}, - {Run: DateMath}, - {Run: RegexpLike}, - {Run: RegexpInstr}, - {Run: RegexpSubstr}, - {Run: RegexpReplace}, + // {Run: StrcmpComparison}, + // {Run: MultiComparisons}, + // {Run: IntervalStatement}, + // {Run: IsStatement}, + // {Run: NotStatement}, + // {Run: LogicalStatement}, + // {Run: TupleComparisons}, + // {Run: Comparisons}, + // {Run: InStatement}, + // {Run: FnField}, + // {Run: FnElt}, + // {Run: FnInsert}, + // {Run: FnLower}, + // {Run: FnUpper}, + // {Run: FnCharLength}, + // {Run: FnLength}, + // {Run: FnBitLength}, + // {Run: FnAscii}, + // {Run: FnReverse}, + // {Run: FnSpace}, + // {Run: FnOrd}, + // {Run: FnRepeat}, + // {Run: FnLeft}, + // {Run: FnLpad}, + // {Run: FnRight}, + // {Run: FnRpad}, + // {Run: FnLTrim}, + // {Run: FnRTrim}, + // {Run: FnTrim}, + // {Run: FnSubstr}, + // {Run: FnLocate}, + // {Run: FnReplace}, + // {Run: FnConcat}, + // {Run: FnConcatWs}, + // {Run: FnChar}, + // {Run: FnHex}, + // {Run: FnUnhex}, + // {Run: FnCeil}, + // {Run: FnFloor}, + // {Run: FnAbs}, + // {Run: FnPi}, + // {Run: FnAcos}, + // {Run: FnAsin}, + // {Run: FnAtan}, + // {Run: FnAtan2}, + // {Run: FnCos}, + // {Run: FnCot}, + // {Run: FnSin}, + // {Run: FnTan}, + // {Run: FnDegrees}, + // {Run: FnRadians}, + // {Run: FnNow, Compare: &Comparison{LooseTime: true}}, + // {Run: FnInfo}, + // {Run: FnExp}, + // {Run: FnLn}, + // {Run: FnLog}, + // {Run: FnLog10}, + // {Run: FnMod}, + // {Run: FnLog2}, + // {Run: FnPow}, + // {Run: FnSign}, + // {Run: FnSqrt}, + // {Run: FnRound}, + // {Run: FnTruncate}, + // {Run: FnCrc32}, + // {Run: FnConv}, + // {Run: FnBin}, + // {Run: FnOct}, + // {Run: FnMD5}, + // {Run: FnSHA1}, + // {Run: FnSHA2}, + // {Run: FnRandomBytes}, + // {Run: FnDateFormat}, + // {Run: FnConvertTz}, + // {Run: FnDate}, + // {Run: FnDayOfMonth}, + // {Run: FnDayOfWeek}, + // {Run: FnDayOfYear}, + // {Run: FnFromUnixtime}, + // {Run: FnHour}, + // {Run: FnMakedate}, + // {Run: FnMaketime}, + // {Run: FnMicroSecond}, + // {Run: FnMinute}, + // {Run: FnMonth}, + // {Run: FnMonthName}, + // {Run: FnLastDay}, + // {Run: FnToDays}, + // {Run: FnFromDays}, + // {Run: FnSecToTime}, + // {Run: FnTimeToSec}, + // {Run: FnToSeconds}, + // {Run: FnQuarter}, + // {Run: FnSecond}, + // {Run: FnTime}, + // {Run: FnUnixTimestamp}, + // {Run: FnWeek}, + // {Run: FnWeekDay}, + // {Run: FnWeekOfYear}, + // {Run: FnYear}, + // {Run: FnYearWeek}, + // {Run: FnPeriodAdd}, + // {Run: FnPeriodDiff}, + // {Run: FnInetAton}, + // {Run: FnInetNtoa}, + // {Run: FnInet6Aton}, + // {Run: FnInet6Ntoa}, + // {Run: FnIsIPv4}, + // {Run: FnIsIPv4Compat}, + // {Run: FnIsIPv4Mapped}, + // {Run: FnIsIPv6}, + // {Run: FnBinToUUID}, + // {Run: FnIsUUID}, + // {Run: FnUUID}, + // {Run: FnUUIDToBin}, + // {Run: DateMath}, + // {Run: RegexpLike}, + // {Run: RegexpInstr}, + // {Run: RegexpSubstr}, + // {Run: RegexpReplace}, } func JSONPathOperations(yield Query) { From 867bc95b0560c924d39949fcb01750ae0c625c7d Mon Sep 17 00:00:00 2001 From: Andres Taylor Date: Sat, 12 Oct 2024 09:35:02 +0200 Subject: [PATCH 12/15] make sure to handle eval and compilation similarly for LIKE Signed-off-by: Andres Taylor --- go/vt/vtgate/evalengine/compiler_test.go | 35 +-- go/vt/vtgate/evalengine/expr_compare.go | 7 +- go/vt/vtgate/evalengine/testcases/cases.go | 288 ++++++++++----------- 3 files changed, 163 insertions(+), 167 deletions(-) diff --git a/go/vt/vtgate/evalengine/compiler_test.go b/go/vt/vtgate/evalengine/compiler_test.go index fbef6b017a9..67c9467de48 100644 --- a/go/vt/vtgate/evalengine/compiler_test.go +++ b/go/vt/vtgate/evalengine/compiler_test.go @@ -97,6 +97,7 @@ func (s *Tracker) String() string { } 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 }() @@ -144,29 +145,19 @@ func TestCompilerReference(t *testing.T) { res, vmErr = env.Evaluate(converted) }) - if vmErr != nil || evalErr != nil { - switch { - 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) - default: - supported++ - } - return - } - - 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 + 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) + supported++ + 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) } - - supported++ }) track.Add(tc.Name(), supported, total) diff --git a/go/vt/vtgate/evalengine/expr_compare.go b/go/vt/vtgate/evalengine/expr_compare.go index 98ba5cd4462..43ebd9e3e19 100644 --- a/go/vt/vtgate/evalengine/expr_compare.go +++ b/go/vt/vtgate/evalengine/expr_compare.go @@ -596,11 +596,16 @@ func (l *LikeExpr) matchWildcard(left, right []byte, coll collations.ID) bool { } func (l *LikeExpr) eval(env *ExpressionEnv) (eval, error) { - left, right, err := l.arguments(env) + left, err := l.Left.eval(env) + if err != nil { + return nil, err + } + if left == nil { return nil, nil } + right, err := l.Right.eval(env) if err != nil { return nil, err } diff --git a/go/vt/vtgate/evalengine/testcases/cases.go b/go/vt/vtgate/evalengine/testcases/cases.go index bbacde3013e..ff6c0c0f311 100644 --- a/go/vt/vtgate/evalengine/testcases/cases.go +++ b/go/vt/vtgate/evalengine/testcases/cases.go @@ -29,151 +29,151 @@ import ( ) var Cases = []TestCase{ - // {Run: JSONExtract, Schema: JSONExtract_Schema}, - // {Run: JSONPathOperations}, - // {Run: JSONArray}, - // {Run: JSONObject}, - // {Run: CharsetConversionOperators}, - // {Run: CaseExprWithPredicate}, - // {Run: CaseExprWithValue}, - // {Run: If}, - // {Run: Base64}, - // {Run: Conversion}, - // {Run: LargeDecimals}, - // {Run: LargeIntegers}, - // {Run: DecimalClamping}, - // {Run: BitwiseOperatorsUnary}, - // {Run: BitwiseOperators}, - // {Run: WeightString}, - // {Run: FloatFormatting}, - // {Run: UnderscoreAndPercentage}, - // {Run: Types}, - // {Run: Arithmetic}, - // {Run: HexArithmetic}, - // {Run: NumericTypes}, - // {Run: NegateArithmetic}, - // {Run: CollationOperations}, + {Run: JSONExtract, Schema: JSONExtract_Schema}, + {Run: JSONPathOperations}, + {Run: JSONArray}, + {Run: JSONObject}, + {Run: CharsetConversionOperators}, + {Run: CaseExprWithPredicate}, + {Run: CaseExprWithValue}, + {Run: If}, + {Run: Base64}, + {Run: Conversion}, + {Run: LargeDecimals}, + {Run: LargeIntegers}, + {Run: DecimalClamping}, + {Run: BitwiseOperatorsUnary}, + {Run: BitwiseOperators}, + {Run: WeightString}, + {Run: FloatFormatting}, + {Run: UnderscoreAndPercentage}, + {Run: Types}, + {Run: Arithmetic}, + {Run: HexArithmetic}, + {Run: NumericTypes}, + {Run: NegateArithmetic}, + {Run: CollationOperations}, {Run: LikeComparison}, - // {Run: StrcmpComparison}, - // {Run: MultiComparisons}, - // {Run: IntervalStatement}, - // {Run: IsStatement}, - // {Run: NotStatement}, - // {Run: LogicalStatement}, - // {Run: TupleComparisons}, - // {Run: Comparisons}, - // {Run: InStatement}, - // {Run: FnField}, - // {Run: FnElt}, - // {Run: FnInsert}, - // {Run: FnLower}, - // {Run: FnUpper}, - // {Run: FnCharLength}, - // {Run: FnLength}, - // {Run: FnBitLength}, - // {Run: FnAscii}, - // {Run: FnReverse}, - // {Run: FnSpace}, - // {Run: FnOrd}, - // {Run: FnRepeat}, - // {Run: FnLeft}, - // {Run: FnLpad}, - // {Run: FnRight}, - // {Run: FnRpad}, - // {Run: FnLTrim}, - // {Run: FnRTrim}, - // {Run: FnTrim}, - // {Run: FnSubstr}, - // {Run: FnLocate}, - // {Run: FnReplace}, - // {Run: FnConcat}, - // {Run: FnConcatWs}, - // {Run: FnChar}, - // {Run: FnHex}, - // {Run: FnUnhex}, - // {Run: FnCeil}, - // {Run: FnFloor}, - // {Run: FnAbs}, - // {Run: FnPi}, - // {Run: FnAcos}, - // {Run: FnAsin}, - // {Run: FnAtan}, - // {Run: FnAtan2}, - // {Run: FnCos}, - // {Run: FnCot}, - // {Run: FnSin}, - // {Run: FnTan}, - // {Run: FnDegrees}, - // {Run: FnRadians}, - // {Run: FnNow, Compare: &Comparison{LooseTime: true}}, - // {Run: FnInfo}, - // {Run: FnExp}, - // {Run: FnLn}, - // {Run: FnLog}, - // {Run: FnLog10}, - // {Run: FnMod}, - // {Run: FnLog2}, - // {Run: FnPow}, - // {Run: FnSign}, - // {Run: FnSqrt}, - // {Run: FnRound}, - // {Run: FnTruncate}, - // {Run: FnCrc32}, - // {Run: FnConv}, - // {Run: FnBin}, - // {Run: FnOct}, - // {Run: FnMD5}, - // {Run: FnSHA1}, - // {Run: FnSHA2}, - // {Run: FnRandomBytes}, - // {Run: FnDateFormat}, - // {Run: FnConvertTz}, - // {Run: FnDate}, - // {Run: FnDayOfMonth}, - // {Run: FnDayOfWeek}, - // {Run: FnDayOfYear}, - // {Run: FnFromUnixtime}, - // {Run: FnHour}, - // {Run: FnMakedate}, - // {Run: FnMaketime}, - // {Run: FnMicroSecond}, - // {Run: FnMinute}, - // {Run: FnMonth}, - // {Run: FnMonthName}, - // {Run: FnLastDay}, - // {Run: FnToDays}, - // {Run: FnFromDays}, - // {Run: FnSecToTime}, - // {Run: FnTimeToSec}, - // {Run: FnToSeconds}, - // {Run: FnQuarter}, - // {Run: FnSecond}, - // {Run: FnTime}, - // {Run: FnUnixTimestamp}, - // {Run: FnWeek}, - // {Run: FnWeekDay}, - // {Run: FnWeekOfYear}, - // {Run: FnYear}, - // {Run: FnYearWeek}, - // {Run: FnPeriodAdd}, - // {Run: FnPeriodDiff}, - // {Run: FnInetAton}, - // {Run: FnInetNtoa}, - // {Run: FnInet6Aton}, - // {Run: FnInet6Ntoa}, - // {Run: FnIsIPv4}, - // {Run: FnIsIPv4Compat}, - // {Run: FnIsIPv4Mapped}, - // {Run: FnIsIPv6}, - // {Run: FnBinToUUID}, - // {Run: FnIsUUID}, - // {Run: FnUUID}, - // {Run: FnUUIDToBin}, - // {Run: DateMath}, - // {Run: RegexpLike}, - // {Run: RegexpInstr}, - // {Run: RegexpSubstr}, - // {Run: RegexpReplace}, + {Run: StrcmpComparison}, + {Run: MultiComparisons}, + {Run: IntervalStatement}, + {Run: IsStatement}, + {Run: NotStatement}, + {Run: LogicalStatement}, + {Run: TupleComparisons}, + {Run: Comparisons}, + {Run: InStatement}, + {Run: FnField}, + {Run: FnElt}, + {Run: FnInsert}, + {Run: FnLower}, + {Run: FnUpper}, + {Run: FnCharLength}, + {Run: FnLength}, + {Run: FnBitLength}, + {Run: FnAscii}, + {Run: FnReverse}, + {Run: FnSpace}, + {Run: FnOrd}, + {Run: FnRepeat}, + {Run: FnLeft}, + {Run: FnLpad}, + {Run: FnRight}, + {Run: FnRpad}, + {Run: FnLTrim}, + {Run: FnRTrim}, + {Run: FnTrim}, + {Run: FnSubstr}, + {Run: FnLocate}, + {Run: FnReplace}, + {Run: FnConcat}, + {Run: FnConcatWs}, + {Run: FnChar}, + {Run: FnHex}, + {Run: FnUnhex}, + {Run: FnCeil}, + {Run: FnFloor}, + {Run: FnAbs}, + {Run: FnPi}, + {Run: FnAcos}, + {Run: FnAsin}, + {Run: FnAtan}, + {Run: FnAtan2}, + {Run: FnCos}, + {Run: FnCot}, + {Run: FnSin}, + {Run: FnTan}, + {Run: FnDegrees}, + {Run: FnRadians}, + {Run: FnNow, Compare: &Comparison{LooseTime: true}}, + {Run: FnInfo}, + {Run: FnExp}, + {Run: FnLn}, + {Run: FnLog}, + {Run: FnLog10}, + {Run: FnMod}, + {Run: FnLog2}, + {Run: FnPow}, + {Run: FnSign}, + {Run: FnSqrt}, + {Run: FnRound}, + {Run: FnTruncate}, + {Run: FnCrc32}, + {Run: FnConv}, + {Run: FnBin}, + {Run: FnOct}, + {Run: FnMD5}, + {Run: FnSHA1}, + {Run: FnSHA2}, + {Run: FnRandomBytes}, + {Run: FnDateFormat}, + {Run: FnConvertTz}, + {Run: FnDate}, + {Run: FnDayOfMonth}, + {Run: FnDayOfWeek}, + {Run: FnDayOfYear}, + {Run: FnFromUnixtime}, + {Run: FnHour}, + {Run: FnMakedate}, + {Run: FnMaketime}, + {Run: FnMicroSecond}, + {Run: FnMinute}, + {Run: FnMonth}, + {Run: FnMonthName}, + {Run: FnLastDay}, + {Run: FnToDays}, + {Run: FnFromDays}, + {Run: FnSecToTime}, + {Run: FnTimeToSec}, + {Run: FnToSeconds}, + {Run: FnQuarter}, + {Run: FnSecond}, + {Run: FnTime}, + {Run: FnUnixTimestamp}, + {Run: FnWeek}, + {Run: FnWeekDay}, + {Run: FnWeekOfYear}, + {Run: FnYear}, + {Run: FnYearWeek}, + {Run: FnPeriodAdd}, + {Run: FnPeriodDiff}, + {Run: FnInetAton}, + {Run: FnInetNtoa}, + {Run: FnInet6Aton}, + {Run: FnInet6Ntoa}, + {Run: FnIsIPv4}, + {Run: FnIsIPv4Compat}, + {Run: FnIsIPv4Mapped}, + {Run: FnIsIPv6}, + {Run: FnBinToUUID}, + {Run: FnIsUUID}, + {Run: FnUUID}, + {Run: FnUUIDToBin}, + {Run: DateMath}, + {Run: RegexpLike}, + {Run: RegexpInstr}, + {Run: RegexpSubstr}, + {Run: RegexpReplace}, } func JSONPathOperations(yield Query) { From 9efcf486d5a9ba81e12a1d253e39243679084c36 Mon Sep 17 00:00:00 2001 From: Andres Taylor Date: Mon, 14 Oct 2024 07:20:56 +0200 Subject: [PATCH 13/15] clean up code Signed-off-by: Andres Taylor --- go/vt/vtgate/evalengine/expr_compare.go | 19 +++++-------------- 1 file changed, 5 insertions(+), 14 deletions(-) diff --git a/go/vt/vtgate/evalengine/expr_compare.go b/go/vt/vtgate/evalengine/expr_compare.go index 43ebd9e3e19..293f885197c 100644 --- a/go/vt/vtgate/evalengine/expr_compare.go +++ b/go/vt/vtgate/evalengine/expr_compare.go @@ -597,21 +597,13 @@ func (l *LikeExpr) matchWildcard(left, right []byte, coll collations.ID) bool { func (l *LikeExpr) eval(env *ExpressionEnv) (eval, error) { left, err := l.Left.eval(env) - if err != nil { - return nil, err - } - - if left == nil { - return nil, nil + if err != nil || left == nil { + return left, err } right, err := l.Right.eval(env) - if err != nil { - return nil, err - } - - if right == nil { - return nil, nil + if err != nil || right == nil { + return right, err } var col collations.TypedCollation @@ -702,7 +694,6 @@ func (expr *LikeExpr) compile(c *compiler) (ctype, error) { }) } - c.asm.jumpDestination(skip1) - c.asm.jumpDestination(skip2) + c.asm.jumpDestination(skip1, skip2) return ctype{Type: sqltypes.Int64, Col: collationNumeric, Flag: flagIsBoolean | flagNullable}, nil } From bd67c876e0391189e5f13ba859066fcd7f756e64 Mon Sep 17 00:00:00 2001 From: Manan Gupta Date: Mon, 14 Oct 2024 12:07:11 +0530 Subject: [PATCH 14/15] feat: fix locate null check escaping Signed-off-by: Manan Gupta --- go/vt/vtgate/evalengine/compiler_test.go | 101 +++++++++++++---------- go/vt/vtgate/evalengine/fn_string.go | 28 ++++--- 2 files changed, 72 insertions(+), 57 deletions(-) diff --git a/go/vt/vtgate/evalengine/compiler_test.go b/go/vt/vtgate/evalengine/compiler_test.go index 67c9467de48..cb9b99e7776 100644 --- a/go/vt/vtgate/evalengine/compiler_test.go +++ b/go/vt/vtgate/evalengine/compiler_test.go @@ -96,6 +96,16 @@ 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() @@ -111,52 +121,10 @@ 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 - } - - var expected evalengine.EvalResult - var evalErr error - assert.NotPanics(t, func() { - expected, evalErr = env.EvaluateAST(converted) - }) total++ - - 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) + testCompilerCase(t, query, venv, tc.Schema, env) + if !t.Failed() { supported++ - 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) } }) @@ -167,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 diff --git a/go/vt/vtgate/evalengine/fn_string.go b/go/vt/vtgate/evalengine/fn_string.go index 6d83d36412d..1cca7a94c8d 100644 --- a/go/vt/vtgate/evalengine/fn_string.go +++ b/go/vt/vtgate/evalengine/fn_string.go @@ -1685,24 +1685,17 @@ func (call *builtinLocate) compile(c *compiler) (ctype, error) { return ctype{}, err } + skip1 := c.compileNullCheck1(substr) str, err := call.Arguments[1].compile(c) if err != nil { return ctype{}, err } - skip1 := c.compileNullCheck2(substr, str) - var skip2 *jump - if len(call.Arguments) > 2 { - l, err := call.Arguments[2].compile(c) - if err != nil { - return ctype{}, err - } - skip2 = c.compileNullCheck2(str, l) - _ = c.compileToInt64(l, 1) - } + skip2 := c.compileNullCheck1(str) + var skip3 *jump if !str.isTextual() { - c.asm.Convert_xce(len(call.Arguments)-1, sqltypes.VarChar, c.collation) + c.asm.Convert_xce(1, sqltypes.VarChar, c.collation) str.Col = collations.TypedCollation{ Collation: c.collation, Coercibility: collations.CoerceCoercible, @@ -1713,7 +1706,7 @@ func (call *builtinLocate) compile(c *compiler) (ctype, error) { fromCharset := colldata.Lookup(substr.Col.Collation).Charset() toCharset := colldata.Lookup(str.Col.Collation).Charset() if !substr.isTextual() || (fromCharset != toCharset && !toCharset.IsSuperset(fromCharset)) { - c.asm.Convert_xce(len(call.Arguments), sqltypes.VarChar, str.Col.Collation) + c.asm.Convert_xce(2, sqltypes.VarChar, str.Col.Collation) substr.Col = collations.TypedCollation{ Collation: str.Col.Collation, Coercibility: collations.CoerceCoercible, @@ -1721,6 +1714,15 @@ func (call *builtinLocate) compile(c *compiler) (ctype, error) { } } + if len(call.Arguments) > 2 { + l, err := call.Arguments[2].compile(c) + if err != nil { + return ctype{}, err + } + skip3 = c.compileNullCheck1(l) + _ = c.compileToInt64(l, 1) + } + var coll colldata.Collation if typeIsTextual(substr.Type) && typeIsTextual(str.Type) { coll = colldata.Lookup(str.Col.Collation) @@ -1734,7 +1736,7 @@ func (call *builtinLocate) compile(c *compiler) (ctype, error) { c.asm.Locate2(coll) } - c.asm.jumpDestination(skip1, skip2) + c.asm.jumpDestination(skip1, skip2, skip3) return ctype{Type: sqltypes.Int64, Col: collationNumeric, Flag: flagNullable}, nil } From 02fe1f2b0d1e651480e4d1691120bc2076431915 Mon Sep 17 00:00:00 2001 From: Andres Taylor Date: Mon, 14 Oct 2024 10:11:17 +0200 Subject: [PATCH 15/15] test: simplify code Signed-off-by: Andres Taylor --- go/vt/vtgate/evalengine/expr_compare.go | 14 +------------- 1 file changed, 1 insertion(+), 13 deletions(-) diff --git a/go/vt/vtgate/evalengine/expr_compare.go b/go/vt/vtgate/evalengine/expr_compare.go index 293f885197c..6e6c888ecf6 100644 --- a/go/vt/vtgate/evalengine/expr_compare.go +++ b/go/vt/vtgate/evalengine/expr_compare.go @@ -612,20 +612,8 @@ func (l *LikeExpr) eval(env *ExpressionEnv) (eval, error) { return nil, err } - lbytes, lok := left.(*evalBytes) - rbytes, rok := right.(*evalBytes) + matched := l.matchWildcard(left.ToRawBytes(), right.ToRawBytes(), col.Collation) - var matched bool - switch { - case lok && rok: - matched = l.matchWildcard(lbytes.bytes, rbytes.bytes, col.Collation) - case rok: - matched = l.matchWildcard(left.ToRawBytes(), rbytes.bytes, col.Collation) - case lok: - matched = l.matchWildcard(lbytes.bytes, right.ToRawBytes(), col.Collation) - default: - matched = l.matchWildcard(left.ToRawBytes(), right.ToRawBytes(), collations.CollationBinaryID) - } return newEvalBool(matched), nil }