From d275da17659ec2cda8bfe385f3489b2dc07e184d Mon Sep 17 00:00:00 2001 From: Andres Taylor Date: Thu, 11 Nov 2021 10:27:34 +0100 Subject: [PATCH 1/2] remove keyspace from query before sending it on Signed-off-by: Andres Taylor --- go/vt/sqlparser/ast_funcs.go | 13 ++ go/vt/vtgate/planbuilder/abstract/operator.go | 6 +- .../planbuilder/abstract/operator_test.go | 7 +- .../abstract/operator_test_data.txt | 29 +++++ go/vt/vtgate/planbuilder/horizon_planning.go | 11 +- go/vt/vtgate/planbuilder/route_planning.go | 12 +- .../planbuilder/testdata/from_cases.txt | 117 ++++++++++++++++++ .../planbuilder/testdata/schema_test.json | 3 + .../testdata/systemtables_cases.txt | 16 ++- 9 files changed, 191 insertions(+), 23 deletions(-) diff --git a/go/vt/sqlparser/ast_funcs.go b/go/vt/sqlparser/ast_funcs.go index e1e0f713ce9..68ad3a5d487 100644 --- a/go/vt/sqlparser/ast_funcs.go +++ b/go/vt/sqlparser/ast_funcs.go @@ -1611,3 +1611,16 @@ func defaultRequiresParens(ct *ColumnType) bool { return true } + +// RemoveKeyspaceFromColName removes the Qualifier.Qualifier on all ColNames in the expression tree +func RemoveKeyspaceFromColName(expr Expr) Expr { + return Rewrite(expr, nil, func(cursor *Cursor) bool { + switch col := cursor.Node().(type) { + case *ColName: + if !col.Qualifier.Qualifier.IsEmpty() { + col.Qualifier.Qualifier = NewTableIdent("") + } + } + return true + }).(Expr) // This hard cast is safe because we do not change the type the input +} diff --git a/go/vt/vtgate/planbuilder/abstract/operator.go b/go/vt/vtgate/planbuilder/abstract/operator.go index 4894b96c9b7..88ad893a05e 100644 --- a/go/vt/vtgate/planbuilder/abstract/operator.go +++ b/go/vt/vtgate/planbuilder/abstract/operator.go @@ -90,7 +90,7 @@ func getOperatorFromTableExpr(tableExpr sqlparser.TableExpr, semTable *semantics } op := createJoin(lhs, rhs) if tableExpr.Condition.On != nil { - err = op.PushPredicate(tableExpr.Condition.On, semTable) + err = op.PushPredicate(sqlparser.RemoveKeyspaceFromColName(tableExpr.Condition.On), semTable) if err != nil { return nil, err } @@ -108,7 +108,7 @@ func getOperatorFromTableExpr(tableExpr sqlparser.TableExpr, semTable *semantics if tableExpr.Join == sqlparser.RightJoinType { lhs, rhs = rhs, lhs } - return &Join{LHS: lhs, RHS: rhs, LeftJoin: true, Predicate: tableExpr.Condition.On}, nil + return &Join{LHS: lhs, RHS: rhs, LeftJoin: true, Predicate: sqlparser.RemoveKeyspaceFromColName(tableExpr.Condition.On)}, nil default: return nil, vterrors.Errorf(vtrpcpb.Code_UNIMPLEMENTED, "unsupported: %s", tableExpr.Join.ToString()) } @@ -207,7 +207,7 @@ func createOperatorFromSelect(sel *sqlparser.Select, semTable *semantics.SemTabl if sel.Where != nil { exprs := sqlparser.SplitAndExpression(nil, sel.Where.Expr) for _, expr := range exprs { - err := op.PushPredicate(expr, semTable) + err := op.PushPredicate(sqlparser.RemoveKeyspaceFromColName(expr), semTable) if err != nil { return nil, err } diff --git a/go/vt/vtgate/planbuilder/abstract/operator_test.go b/go/vt/vtgate/planbuilder/abstract/operator_test.go index b86abfabd31..e1620c6dc60 100644 --- a/go/vt/vtgate/planbuilder/abstract/operator_test.go +++ b/go/vt/vtgate/planbuilder/abstract/operator_test.go @@ -50,7 +50,7 @@ func (lcr *lineCountingReader) nextLine() (string, error) { func readTestCase(lcr *lineCountingReader) (testCase, error) { query := "" var err error - for query == "" || query == "\n" { + for query == "" || query == "\n" || strings.HasPrefix(query, "#") { query, err = lcr.nextLine() if err != nil { return testCase{}, err @@ -63,9 +63,9 @@ func readTestCase(lcr *lineCountingReader) (testCase, error) { jsonPart, err := lcr.nextLine() if err != nil { if err == io.EOF { - return testCase{}, fmt.Errorf("test data is bad. expectation not finished") + return tc, fmt.Errorf("test data is bad. expectation not finished") } - return testCase{}, err + return tc, err } if jsonPart == "}\n" { tc.expected += "}" @@ -95,6 +95,7 @@ func TestOperator(t *testing.T) { break } t.Run(fmt.Sprintf("%d:%s", tc.line, tc.query), func(t *testing.T) { + require.NoError(t, err) tree, err := sqlparser.Parse(tc.query) require.NoError(t, err) stmt := tree.(sqlparser.SelectStatement) diff --git a/go/vt/vtgate/planbuilder/abstract/operator_test_data.txt b/go/vt/vtgate/planbuilder/abstract/operator_test_data.txt index 947c78c43cc..74a8639693b 100644 --- a/go/vt/vtgate/planbuilder/abstract/operator_test_data.txt +++ b/go/vt/vtgate/planbuilder/abstract/operator_test_data.txt @@ -415,3 +415,32 @@ SubQuery: { TableSet{0}:`user` where exists (select user_id from user_extra where user_id = 3 and user_id < `user`.id) } } + +# we should remove the keyspace from predicates +select ks.tbl.col from ks.tbl where ks.tbl.id = 1 +QueryGraph: { +Tables: + TableSet{0}:ks.tbl where tbl.id = 1 +} + +select 1 from ks.t join ks.y on ks.t.id = ks.y.t_id +QueryGraph: { +Tables: + TableSet{0}:ks.t + TableSet{1}:ks.y +JoinPredicates: + TableSet{0,1} - t.id = y.t_id +} + +select 1 from ks.t left join ks.y on ks.t.id = ks.y.t_id +OuterJoin: { + Inner: QueryGraph: { + Tables: + TableSet{0}:ks.t + } + Outer: QueryGraph: { + Tables: + TableSet{1}:ks.y + } + Predicate: t.id = y.t_id +} diff --git a/go/vt/vtgate/planbuilder/horizon_planning.go b/go/vt/vtgate/planbuilder/horizon_planning.go index 51380a9d322..0bfcab946ed 100644 --- a/go/vt/vtgate/planbuilder/horizon_planning.go +++ b/go/vt/vtgate/planbuilder/horizon_planning.go @@ -174,7 +174,7 @@ func pushProjection(expr *sqlparser.AliasedExpr, plan logicalPlan, semTable *sem return i, false, nil } } - expr = removeKeyspaceFromColName(expr) + expr.Expr = sqlparser.RemoveKeyspaceFromColName(expr.Expr) sel, isSel := node.Select.(*sqlparser.Select) if !isSel { return 0, false, vterrors.NewErrorf(vtrpcpb.Code_INVALID_ARGUMENT, vterrors.BadFieldError, "Unknown column '%s' in 'order clause'", sqlparser.String(expr)) @@ -348,15 +348,6 @@ func rewriteProjectionOfDerivedTable(expr *sqlparser.AliasedExpr, semTable *sema return nil } -func removeKeyspaceFromColName(expr *sqlparser.AliasedExpr) *sqlparser.AliasedExpr { - if _, ok := expr.Expr.(*sqlparser.ColName); ok { - expr = sqlparser.CloneRefOfAliasedExpr(expr) - col := expr.Expr.(*sqlparser.ColName) - col.Qualifier.Qualifier = sqlparser.NewTableIdent("") - } - return expr -} - func checkIfAlreadyExists(expr *sqlparser.AliasedExpr, node sqlparser.SelectStatement, semTable *semantics.SemTable) int { exprDep := semTable.RecursiveDeps(expr.Expr) // Here to find if the expr already exists in the SelectStatement, we have 3 cases diff --git a/go/vt/vtgate/planbuilder/route_planning.go b/go/vt/vtgate/planbuilder/route_planning.go index dcb104af3fc..9d8fa535f46 100644 --- a/go/vt/vtgate/planbuilder/route_planning.go +++ b/go/vt/vtgate/planbuilder/route_planning.go @@ -416,18 +416,18 @@ func planSingleShardRoutePlan(sel sqlparser.SelectStatement, rb *route) error { return err } sqlparser.Rewrite(rb.Select, func(cursor *sqlparser.Cursor) bool { - if aliasedExpr, ok := cursor.Node().(*sqlparser.AliasedExpr); ok { - cursor.Replace(removeKeyspaceFromColName(aliasedExpr)) + if aliasedExpr, ok := cursor.Node().(sqlparser.SelectExpr); ok { + removeKeyspaceFromSelectExpr(aliasedExpr) } return true }, nil) return nil } -func removeKeyspaceFromSelectExpr(expr sqlparser.SelectExpr, ast *sqlparser.Select, i int) { +func removeKeyspaceFromSelectExpr(expr sqlparser.SelectExpr) { switch expr := expr.(type) { case *sqlparser.AliasedExpr: - ast.SelectExprs[i] = removeKeyspaceFromColName(expr) + expr.Expr = sqlparser.RemoveKeyspaceFromColName(expr.Expr) case *sqlparser.StarExpr: expr.TableName.Qualifier = sqlparser.NewTableIdent("") } @@ -448,8 +448,8 @@ func stripDownQuery(from, to sqlparser.SelectStatement) error { toNode.OrderBy = node.OrderBy toNode.Comments = node.Comments toNode.SelectExprs = node.SelectExprs - for i, expr := range toNode.SelectExprs { - removeKeyspaceFromSelectExpr(expr, toNode, i) + for _, expr := range toNode.SelectExprs { + removeKeyspaceFromSelectExpr(expr) } case *sqlparser.Union: toNode, ok := to.(*sqlparser.Union) diff --git a/go/vt/vtgate/planbuilder/testdata/from_cases.txt b/go/vt/vtgate/planbuilder/testdata/from_cases.txt index 827523a672f..82380e2196c 100644 --- a/go/vt/vtgate/planbuilder/testdata/from_cases.txt +++ b/go/vt/vtgate/planbuilder/testdata/from_cases.txt @@ -377,6 +377,123 @@ Gen4 plan same as above "table disabled has been disabled" Gen4 plan same as above +"select second_user.foo.col from second_user.foo join user on second_user.foo.id = user.id where second_user.foo.col = 42" +{ + "QueryType": "SELECT", + "Original": "select second_user.foo.col from second_user.foo join user on second_user.foo.id = user.id where second_user.foo.col = 42", + "Instructions": { + "OperatorType": "Route", + "Variant": "SelectScatter", + "Keyspace": { + "Name": "user", + "Sharded": true + }, + "FieldQuery": "select foo.col from `user` as foo join `user` on foo.id = `user`.id where 1 != 1", + "Query": "select foo.col from `user` as foo join `user` on foo.id = `user`.id where foo.col = 42", + "Table": "`user`" + } +} +{ + "QueryType": "SELECT", + "Original": "select second_user.foo.col from second_user.foo join user on second_user.foo.id = user.id where second_user.foo.col = 42", + "Instructions": { + "OperatorType": "Route", + "Variant": "SelectScatter", + "Keyspace": { + "Name": "user", + "Sharded": true + }, + "FieldQuery": "select foo.col from `user` as foo, `user` where 1 != 1", + "Query": "select foo.col from `user` as foo, `user` where foo.col = 42 and foo.id = `user`.id", + "Table": "`user`" + } +} + +"select user.music.foo from user.music join user on user.music.id = user.id where user.music.col = 42" +{ + "QueryType": "SELECT", + "Original": "select user.music.foo from user.music join user on user.music.id = user.id where user.music.col = 42", + "Instructions": { + "OperatorType": "Join", + "Variant": "Join", + "JoinColumnIndexes": "-1", + "JoinVars": { + "music_id": 1 + }, + "TableName": "music_`user`", + "Inputs": [ + { + "OperatorType": "Route", + "Variant": "SelectScatter", + "Keyspace": { + "Name": "user", + "Sharded": true + }, + "FieldQuery": "select music.foo, music.id from music where 1 != 1", + "Query": "select music.foo, music.id from music where music.col = 42", + "Table": "music" + }, + { + "OperatorType": "Route", + "Variant": "SelectEqualUnique", + "Keyspace": { + "Name": "user", + "Sharded": true + }, + "FieldQuery": "select 1 from `user` where 1 != 1", + "Query": "select 1 from `user` where `user`.id = :music_id", + "Table": "`user`", + "Values": [ + ":music_id" + ], + "Vindex": "user_index" + } + ] + } +} +{ + "QueryType": "SELECT", + "Original": "select user.music.foo from user.music join user on user.music.id = user.id where user.music.col = 42", + "Instructions": { + "OperatorType": "Join", + "Variant": "Join", + "JoinColumnIndexes": "-2", + "JoinVars": { + "music_id": 0 + }, + "TableName": "music_`user`", + "Inputs": [ + { + "OperatorType": "Route", + "Variant": "SelectScatter", + "Keyspace": { + "Name": "user", + "Sharded": true + }, + "FieldQuery": "select music.id, music.foo from music where 1 != 1", + "Query": "select music.id, music.foo from music where music.col = 42", + "Table": "music" + }, + { + "OperatorType": "Route", + "Variant": "SelectEqualUnique", + "Keyspace": { + "Name": "user", + "Sharded": true + }, + "FieldQuery": "select 1 from `user` where 1 != 1", + "Query": "select 1 from `user` where `user`.id = :music_id", + "Table": "`user`", + "Values": [ + ":music_id" + ], + "Vindex": "user_index" + } + ] + } +} + + # ',' join "select music.col from user, music" { diff --git a/go/vt/vtgate/planbuilder/testdata/schema_test.json b/go/vt/vtgate/planbuilder/testdata/schema_test.json index b7e1fcdc0f3..68b3b9eb827 100644 --- a/go/vt/vtgate/planbuilder/testdata/schema_test.json +++ b/go/vt/vtgate/planbuilder/testdata/schema_test.json @@ -9,6 +9,9 @@ }, { "from_table": "second_user.user", "to_tables": ["user.user"] + }, { + "from_table": "second_user.foo", + "to_tables": ["user.user"] }, { "from_table": "primary_redirect@primary", "to_tables": ["user.user"] diff --git a/go/vt/vtgate/planbuilder/testdata/systemtables_cases.txt b/go/vt/vtgate/planbuilder/testdata/systemtables_cases.txt index 886b6535087..ca5fbb12c1e 100644 --- a/go/vt/vtgate/planbuilder/testdata/systemtables_cases.txt +++ b/go/vt/vtgate/planbuilder/testdata/systemtables_cases.txt @@ -124,7 +124,21 @@ Gen4 plan same as above "Table": "information_schema.a" } } -Gen4 plan same as above +{ + "QueryType": "SELECT", + "Original": "select * from information_schema.a where information_schema.a.b=10", + "Instructions": { + "OperatorType": "Route", + "Variant": "SelectDBA", + "Keyspace": { + "Name": "main", + "Sharded": false + }, + "FieldQuery": "select * from information_schema.a where 1 != 1", + "Query": "select * from information_schema.a where a.b = 10", + "Table": "information_schema.a" + } +} # union of information_schema "select * from information_schema.a union select * from information_schema.b" From 1389cbacdc20a4ea1119b643f8dff23b57724ad8 Mon Sep 17 00:00:00 2001 From: Andres Taylor Date: Thu, 11 Nov 2021 10:39:54 +0100 Subject: [PATCH 2/2] chore: clean up code after review Signed-off-by: Andres Taylor --- go/vt/sqlparser/ast_funcs.go | 6 ++++-- go/vt/vtgate/planbuilder/route_planning.go | 2 +- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/go/vt/sqlparser/ast_funcs.go b/go/vt/sqlparser/ast_funcs.go index 68ad3a5d487..eb172ea8f4c 100644 --- a/go/vt/sqlparser/ast_funcs.go +++ b/go/vt/sqlparser/ast_funcs.go @@ -1614,7 +1614,7 @@ func defaultRequiresParens(ct *ColumnType) bool { // RemoveKeyspaceFromColName removes the Qualifier.Qualifier on all ColNames in the expression tree func RemoveKeyspaceFromColName(expr Expr) Expr { - return Rewrite(expr, nil, func(cursor *Cursor) bool { + Rewrite(expr, nil, func(cursor *Cursor) bool { switch col := cursor.Node().(type) { case *ColName: if !col.Qualifier.Qualifier.IsEmpty() { @@ -1622,5 +1622,7 @@ func RemoveKeyspaceFromColName(expr Expr) Expr { } } return true - }).(Expr) // This hard cast is safe because we do not change the type the input + }) // This hard cast is safe because we do not change the type the input + + return expr } diff --git a/go/vt/vtgate/planbuilder/route_planning.go b/go/vt/vtgate/planbuilder/route_planning.go index 9d8fa535f46..836f52728c7 100644 --- a/go/vt/vtgate/planbuilder/route_planning.go +++ b/go/vt/vtgate/planbuilder/route_planning.go @@ -427,7 +427,7 @@ func planSingleShardRoutePlan(sel sqlparser.SelectStatement, rb *route) error { func removeKeyspaceFromSelectExpr(expr sqlparser.SelectExpr) { switch expr := expr.(type) { case *sqlparser.AliasedExpr: - expr.Expr = sqlparser.RemoveKeyspaceFromColName(expr.Expr) + sqlparser.RemoveKeyspaceFromColName(expr.Expr) case *sqlparser.StarExpr: expr.TableName.Qualifier = sqlparser.NewTableIdent("") }