From 5b86047c70f33a54f85aea6724617726e456e594 Mon Sep 17 00:00:00 2001 From: Harshit Gangal Date: Mon, 12 Feb 2024 19:12:34 +0530 Subject: [PATCH 01/12] add support for update with multi table reference Signed-off-by: Harshit Gangal --- go/vt/vterrors/code.go | 2 + go/vt/vtgate/planbuilder/operators/update.go | 45 ++++++++--- .../planbuilder/testdata/dml_cases.json | 79 +++++++++++++++++++ .../testdata/unsupported_cases.json | 30 +++---- go/vt/vtgate/semantics/check_invalid.go | 17 ---- 5 files changed, 130 insertions(+), 43 deletions(-) diff --git a/go/vt/vterrors/code.go b/go/vt/vterrors/code.go index eb2b1c4be6d..574ac7c2cdf 100644 --- a/go/vt/vterrors/code.go +++ b/go/vt/vterrors/code.go @@ -57,6 +57,7 @@ var ( VT03029 = errorWithState("VT03029", vtrpcpb.Code_INVALID_ARGUMENT, WrongValueCountOnRow, "column count does not match value count with the row for vindex '%s'", "The number of columns you want to insert do not match the number of columns of your SELECT query.") VT03030 = errorWithState("VT03030", vtrpcpb.Code_INVALID_ARGUMENT, WrongValueCountOnRow, "lookup column count does not match value count with the row (columns, count): (%v, %d)", "The number of columns you want to insert do not match the number of columns of your SELECT query.") VT03031 = errorWithoutState("VT03031", vtrpcpb.Code_INVALID_ARGUMENT, "EXPLAIN is only supported for single keyspace", "EXPLAIN has to be sent down as a single query to the underlying MySQL, and this is not possible if it uses tables from multiple keyspaces") + VT03032 = errorWithState("VT03031", vtrpcpb.Code_INVALID_ARGUMENT, NonUpdateableTable, "the target table %s of the UPDATE is not updatable", "You cannot update a table that is not a real MySQL table.") VT05001 = errorWithState("VT05001", vtrpcpb.Code_NOT_FOUND, DbDropExists, "cannot drop database '%s'; database does not exists", "The given database does not exist; Vitess cannot drop it.") VT05002 = errorWithState("VT05002", vtrpcpb.Code_NOT_FOUND, BadDb, "cannot alter database '%s'; unknown database", "The given database does not exist; Vitess cannot alter it.") @@ -141,6 +142,7 @@ var ( VT03029, VT03030, VT03031, + VT03032, VT05001, VT05002, VT05003, diff --git a/go/vt/vtgate/planbuilder/operators/update.go b/go/vt/vtgate/planbuilder/operators/update.go index a4aafa222fa..2390507ef23 100644 --- a/go/vt/vtgate/planbuilder/operators/update.go +++ b/go/vt/vtgate/planbuilder/operators/update.go @@ -103,6 +103,8 @@ func (u *Update) ShortDescription() string { } func createOperatorFromUpdate(ctx *plancontext.PlanningContext, updStmt *sqlparser.Update) (op Operator) { + errIfUpdateNotSupported(ctx, updStmt) + var updClone *sqlparser.Update var vTbl *vindexes.Table @@ -134,6 +136,31 @@ func createOperatorFromUpdate(ctx *plancontext.PlanningContext, updStmt *sqlpars return buildFkOperator(ctx, op, updClone, parentFks, childFks, vTbl) } +func errIfUpdateNotSupported(ctx *plancontext.PlanningContext, stmt *sqlparser.Update) { + var vTbl *vindexes.Table + for _, ue := range stmt.Exprs { + tblInfo, err := ctx.SemTable.TableInfoForExpr(ue.Name) + if err != nil { + panic(err) + } + if _, isATable := tblInfo.(*semantics.RealTable); !isATable { + var tblName string + ate := tblInfo.GetAliasedTableExpr() + if ate != nil { + tblName = sqlparser.String(ate) + } + panic(vterrors.VT03032(tblName)) + } + + if vTbl == nil { + vTbl = tblInfo.GetVindexTable() + } + if vTbl != tblInfo.GetVindexTable() { + panic(vterrors.VT12001("multi-table UPDATE statement with multi-target column update")) + } + } +} + func createUpdateOperator(ctx *plancontext.PlanningContext, updStmt *sqlparser.Update) (Operator, *vindexes.Table, *sqlparser.Update) { op := crossJoin(ctx, updStmt.TableExprs) @@ -148,6 +175,8 @@ func createUpdateOperator(ctx *plancontext.PlanningContext, updStmt *sqlparser.U // If we encounter subqueries, we want to fix the updClone to use the replaced expression, so that the pulled out subquery's // result is used everywhere instead of running the subquery multiple times, which is wasteful. updClone := sqlparser.CloneRefOfUpdate(updStmt) + var tblInfo semantics.TableInfo + var err error for idx, updExpr := range updStmt.Exprs { expr, subqs := sqc.pullOutValueSubqueries(ctx, updExpr.Expr, outerID, true) if len(subqs) == 0 { @@ -164,19 +193,13 @@ func createUpdateOperator(ctx *plancontext.PlanningContext, updStmt *sqlparser.U Name: updExpr.Name, Expr: proj, } + tblInfo, err = ctx.SemTable.TableInfoForExpr(updExpr.Name) + if err != nil { + panic(err) + } } - target := updStmt.TableExprs[0] - atbl, ok := target.(*sqlparser.AliasedTableExpr) - if !ok { - panic(vterrors.VT12001("multi table update")) - } - tblID := ctx.SemTable.TableSetFor(atbl) - tblInfo, err := ctx.SemTable.TableInfoFor(tblID) - if err != nil { - panic(err) - } - + tblID := ctx.SemTable.TableSetFor(tblInfo.GetAliasedTableExpr()) vTbl := tblInfo.GetVindexTable() // Reference table should update the source table. if vTbl.Type == vindexes.TypeReference && vTbl.Source != nil { diff --git a/go/vt/vtgate/planbuilder/testdata/dml_cases.json b/go/vt/vtgate/planbuilder/testdata/dml_cases.json index 5e4987be8c3..a6516f396e8 100644 --- a/go/vt/vtgate/planbuilder/testdata/dml_cases.json +++ b/go/vt/vtgate/planbuilder/testdata/dml_cases.json @@ -5660,5 +5660,84 @@ "user.user" ] } + }, + { + "comment": "update with multi table join with single target", + "query": "update user as u, user_extra as ue set u.name = 'foo' where u.id = ue.id", + "plan": { + "QueryType": "UPDATE", + "Original": "update user as u, user_extra as ue set u.name = 'foo' where u.id = ue.id", + "Instructions": { + "OperatorType": "DMLWithInput", + "TargetTabletType": "PRIMARY", + "Offset": [ + 0 + ], + "Inputs": [ + { + "OperatorType": "Join", + "Variant": "Join", + "JoinColumnIndexes": "R:0", + "JoinVars": { + "ue_id": 0 + }, + "TableName": "user_extra_`user`", + "Inputs": [ + { + "OperatorType": "Route", + "Variant": "Scatter", + "Keyspace": { + "Name": "user", + "Sharded": true + }, + "FieldQuery": "select ue.id from user_extra as ue where 1 != 1", + "Query": "select ue.id from user_extra as ue lock in share mode", + "Table": "user_extra" + }, + { + "OperatorType": "Route", + "Variant": "EqualUnique", + "Keyspace": { + "Name": "user", + "Sharded": true + }, + "FieldQuery": "select u.id from `user` as u where 1 != 1", + "Query": "select u.id from `user` as u where u.id = :ue_id lock in share mode", + "Table": "`user`", + "Values": [ + ":ue_id" + ], + "Vindex": "user_index" + } + ] + }, + { + "OperatorType": "Update", + "Variant": "IN", + "Keyspace": { + "Name": "user", + "Sharded": true + }, + "TargetTabletType": "PRIMARY", + "ChangedVindexValues": [ + "name_user_map:3" + ], + "KsidLength": 1, + "KsidVindex": "user_index", + "OwnedVindexQuery": "select Id, `Name`, Costly, u.`name` = 'foo' from `user` as u where u.id in ::dml_vals for update", + "Query": "update `user` as u set u.`name` = 'foo' where u.id in ::dml_vals", + "Table": "user", + "Values": [ + "::dml_vals" + ], + "Vindex": "user_index" + } + ] + }, + "TablesUsed": [ + "user.user", + "user.user_extra" + ] + } } ] diff --git a/go/vt/vtgate/planbuilder/testdata/unsupported_cases.json b/go/vt/vtgate/planbuilder/testdata/unsupported_cases.json index 59071c7e810..84d70fb24f3 100644 --- a/go/vt/vtgate/planbuilder/testdata/unsupported_cases.json +++ b/go/vt/vtgate/planbuilder/testdata/unsupported_cases.json @@ -64,21 +64,6 @@ "query": "update user_metadata set email = 'juan@vitess.io' where user_id = 1 limit 10", "plan": "VT12001: unsupported: Vindex update should have ORDER BY clause when using LIMIT" }, - { - "comment": "update with derived table", - "query": "update (select id from user) as u set id = 4", - "plan": "The target table u of the UPDATE is not updatable" - }, - { - "comment": "join in update tables", - "query": "update user join user_extra on user.id = user_extra.id set user.name = 'foo'", - "plan": "VT12001: unsupported: unaliased multiple tables in update" - }, - { - "comment": "multiple tables in update", - "query": "update user as u, user_extra as ue set u.name = 'foo' where u.id = ue.id", - "plan": "VT12001: unsupported: multiple (2) tables in update" - }, { "comment": "unsharded insert, col list does not match values", "query": "insert into unsharded_auto(id, val) values(1)", @@ -408,5 +393,20 @@ "comment": "count and sum distinct on different columns", "query": "SELECT COUNT(DISTINCT col), SUM(DISTINCT id) FROM user", "plan": "VT12001: unsupported: only one DISTINCT aggregation is allowed in a SELECT: sum(distinct id)" + }, + { + "comment": "update with derived table", + "query": "update (select id from user) as u set id = 4", + "plan": "VT03031: the target table (select id from `user`) as u of the UPDATE is not updatable" + }, + { + "comment": "Add your test case here for debugging and run go test -run=One.", + "query": "update ignore user u, music m set u.foo = 21, m.bar = 'abc' where u.col = m.col", + "plan": "VT12001: unsupported: multi-table UPDATE statement with multi-target column update" + }, + { + "comment": "Add your test case here for debugging and run go test -run=One.", + "query": "update ignore (select foo, col, bar from user) u, music m set u.foo = 21, u.bar = 'abc' where u.col = m.col", + "plan": "VT03031: the target table (select foo, col, bar from `user`) as u of the UPDATE is not updatable" } ] diff --git a/go/vt/vtgate/semantics/check_invalid.go b/go/vt/vtgate/semantics/check_invalid.go index 38dacefa1f0..45c160e93a2 100644 --- a/go/vt/vtgate/semantics/check_invalid.go +++ b/go/vt/vtgate/semantics/check_invalid.go @@ -24,8 +24,6 @@ import ( func (a *analyzer) checkForInvalidConstructs(cursor *sqlparser.Cursor) error { switch node := cursor.Node().(type) { - case *sqlparser.Update: - return checkUpdate(node) case *sqlparser.Select: return a.checkSelect(cursor, node) case *sqlparser.Nextval: @@ -179,21 +177,6 @@ func (a *analyzer) checkSelect(cursor *sqlparser.Cursor, node *sqlparser.Select) return nil } -func checkUpdate(node *sqlparser.Update) error { - if len(node.TableExprs) != 1 { - return ShardedError{Inner: &UnsupportedMultiTablesInUpdateError{ExprCount: len(node.TableExprs)}} - } - alias, isAlias := node.TableExprs[0].(*sqlparser.AliasedTableExpr) - if !isAlias { - return ShardedError{Inner: &UnsupportedMultiTablesInUpdateError{NotAlias: true}} - } - _, isDerived := alias.Expr.(*sqlparser.DerivedTable) - if isDerived { - return &TableNotUpdatableError{Table: alias.As.String()} - } - return nil -} - // checkAliasedTableExpr checks the validity of AliasedTableExpr. func checkAliasedTableExpr(node *sqlparser.AliasedTableExpr) error { if len(node.Hints) == 0 { From 29fc1981c656b695bdac3c602731092db00304e2 Mon Sep 17 00:00:00 2001 From: Harshit Gangal Date: Mon, 12 Feb 2024 19:58:01 +0530 Subject: [PATCH 02/12] changed the code to use update target than ast nodes after semantic analyzer Signed-off-by: Harshit Gangal --- .../planbuilder/operators/dml_planning.go | 7 +- go/vt/vtgate/planbuilder/operators/update.go | 5 +- .../planbuilder/testdata/dml_cases.json | 79 +++++++++++++++++++ 3 files changed, 84 insertions(+), 7 deletions(-) diff --git a/go/vt/vtgate/planbuilder/operators/dml_planning.go b/go/vt/vtgate/planbuilder/operators/dml_planning.go index 6dbe7a74ff3..ed777428381 100644 --- a/go/vt/vtgate/planbuilder/operators/dml_planning.go +++ b/go/vt/vtgate/planbuilder/operators/dml_planning.go @@ -109,6 +109,7 @@ func buildChangedVindexesValues( ctx *plancontext.PlanningContext, update *sqlparser.Update, table *vindexes.Table, + ate *sqlparser.AliasedTableExpr, ksidCols []sqlparser.IdentifierCI, assignments []SetExpr, ) (vv map[string]*engine.VindexValues, ownedVindexQuery *sqlparser.Select, subQueriesArgOnChangedVindex []string) { @@ -144,11 +145,7 @@ func buildChangedVindexesValues( return nil, nil, nil } // generate rest of the owned vindex query. - aTblExpr, ok := update.TableExprs[0].(*sqlparser.AliasedTableExpr) - if !ok { - panic(vterrors.VT12001("UPDATE on complex table expression")) - } - tblExpr := &sqlparser.AliasedTableExpr{Expr: sqlparser.TableName{Name: table.Name}, As: aTblExpr.As} + tblExpr := sqlparser.NewAliasedTableExpr(sqlparser.TableName{Name: table.Name}, ate.As.String()) ovq := &sqlparser.Select{ From: []sqlparser.TableExpr{tblExpr}, SelectExprs: selExprs, diff --git a/go/vt/vtgate/planbuilder/operators/update.go b/go/vt/vtgate/planbuilder/operators/update.go index 2390507ef23..5fc7dc0d4a7 100644 --- a/go/vt/vtgate/planbuilder/operators/update.go +++ b/go/vt/vtgate/planbuilder/operators/update.go @@ -217,7 +217,7 @@ func createUpdateOperator(ctx *plancontext.PlanningContext, updStmt *sqlparser.U Name: name, } - _, cvv, ovq, subQueriesArgOnChangedVindex := getUpdateVindexInformation(ctx, updStmt, targetTbl, assignments) + _, cvv, ovq, subQueriesArgOnChangedVindex := getUpdateVindexInformation(ctx, updStmt, targetTbl, tblInfo.GetAliasedTableExpr(), assignments) updOp := &Update{ DMLCommon: &DMLCommon{ @@ -249,6 +249,7 @@ func getUpdateVindexInformation( ctx *plancontext.PlanningContext, updStmt *sqlparser.Update, table TargetTable, + ate *sqlparser.AliasedTableExpr, assignments []SetExpr, ) ([]*VindexPlusPredicates, map[string]*engine.VindexValues, *sqlparser.Select, []string) { if !table.VTable.Keyspace.Sharded { @@ -256,7 +257,7 @@ func getUpdateVindexInformation( } primaryVindex, vindexAndPredicates := getVindexInformation(table.ID, table.VTable) - changedVindexValues, ownedVindexQuery, subQueriesArgOnChangedVindex := buildChangedVindexesValues(ctx, updStmt, table.VTable, primaryVindex.Columns, assignments) + changedVindexValues, ownedVindexQuery, subQueriesArgOnChangedVindex := buildChangedVindexesValues(ctx, updStmt, table.VTable, ate, primaryVindex.Columns, assignments) return vindexAndPredicates, changedVindexValues, ownedVindexQuery, subQueriesArgOnChangedVindex } diff --git a/go/vt/vtgate/planbuilder/testdata/dml_cases.json b/go/vt/vtgate/planbuilder/testdata/dml_cases.json index a6516f396e8..4eca7a57fb2 100644 --- a/go/vt/vtgate/planbuilder/testdata/dml_cases.json +++ b/go/vt/vtgate/planbuilder/testdata/dml_cases.json @@ -5739,5 +5739,84 @@ "user.user_extra" ] } + }, + { + "comment": "update with multi table join with single target modifying lookup vindex", + "query": "update user join user_extra on user.id = user_extra.id set user.name = 'foo'", + "plan": { + "QueryType": "UPDATE", + "Original": "update user join user_extra on user.id = user_extra.id set user.name = 'foo'", + "Instructions": { + "OperatorType": "DMLWithInput", + "TargetTabletType": "PRIMARY", + "Offset": [ + 0 + ], + "Inputs": [ + { + "OperatorType": "Join", + "Variant": "Join", + "JoinColumnIndexes": "R:0", + "JoinVars": { + "user_extra_id": 0 + }, + "TableName": "user_extra_`user`", + "Inputs": [ + { + "OperatorType": "Route", + "Variant": "Scatter", + "Keyspace": { + "Name": "user", + "Sharded": true + }, + "FieldQuery": "select user_extra.id from user_extra where 1 != 1", + "Query": "select user_extra.id from user_extra lock in share mode", + "Table": "user_extra" + }, + { + "OperatorType": "Route", + "Variant": "EqualUnique", + "Keyspace": { + "Name": "user", + "Sharded": true + }, + "FieldQuery": "select `user`.id from `user` where 1 != 1", + "Query": "select `user`.id from `user` where `user`.id = :user_extra_id lock in share mode", + "Table": "`user`", + "Values": [ + ":user_extra_id" + ], + "Vindex": "user_index" + } + ] + }, + { + "OperatorType": "Update", + "Variant": "IN", + "Keyspace": { + "Name": "user", + "Sharded": true + }, + "TargetTabletType": "PRIMARY", + "ChangedVindexValues": [ + "name_user_map:3" + ], + "KsidLength": 1, + "KsidVindex": "user_index", + "OwnedVindexQuery": "select Id, `Name`, Costly, `user`.`name` = 'foo' from `user` where `user`.id in ::dml_vals for update", + "Query": "update `user` set `user`.`name` = 'foo' where `user`.id in ::dml_vals", + "Table": "user", + "Values": [ + "::dml_vals" + ], + "Vindex": "user_index" + } + ] + }, + "TablesUsed": [ + "user.user", + "user.user_extra" + ] + } } ] From 4e7ea13def91ea3fa14d05fd365276a2d6ae272e Mon Sep 17 00:00:00 2001 From: Harshit Gangal Date: Mon, 12 Feb 2024 20:03:11 +0530 Subject: [PATCH 03/12] addressed review comments Signed-off-by: Harshit Gangal --- go/vt/vtgate/planbuilder/operators/delete.go | 4 ++-- go/vt/vtgate/planbuilder/operators/phases.go | 2 +- go/vt/vtgate/semantics/analyzer.go | 4 ++-- go/vt/vtgate/semantics/semantic_state.go | 2 +- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/go/vt/vtgate/planbuilder/operators/delete.go b/go/vt/vtgate/planbuilder/operators/delete.go index 22b8f1e4281..4bc2f48f4d8 100644 --- a/go/vt/vtgate/planbuilder/operators/delete.go +++ b/go/vt/vtgate/planbuilder/operators/delete.go @@ -220,12 +220,12 @@ func generateOwnedVindexQuery(tblExpr sqlparser.TableExpr, del *sqlparser.Delete var selExprs sqlparser.SelectExprs for _, col := range ksidCols { colName := makeColName(col, table, sqlparser.MultiTable(del.TableExprs)) - selExprs = append(selExprs, sqlparser.NewAliasedExpr(colName, "")) + selExprs = append(selExprs, aeWrap(colName)) } for _, cv := range table.VTable.Owned { for _, col := range cv.Columns { colName := makeColName(col, table, sqlparser.MultiTable(del.TableExprs)) - selExprs = append(selExprs, sqlparser.NewAliasedExpr(colName, "")) + selExprs = append(selExprs, aeWrap(colName)) } } sqlparser.RemoveKeyspaceInTables(tblExpr) diff --git a/go/vt/vtgate/planbuilder/operators/phases.go b/go/vt/vtgate/planbuilder/operators/phases.go index b45fbd5c9ad..3937105c494 100644 --- a/go/vt/vtgate/planbuilder/operators/phases.go +++ b/go/vt/vtgate/planbuilder/operators/phases.go @@ -79,7 +79,7 @@ func (p Phase) shouldRun(s semantics.QuerySignature) bool { case subquerySettling: return s.SubQueries case dmlWithInput: - return s.Dml + return s.DML default: return true } diff --git a/go/vt/vtgate/semantics/analyzer.go b/go/vt/vtgate/semantics/analyzer.go index 95d645d7d9d..f52bd983104 100644 --- a/go/vt/vtgate/semantics/analyzer.go +++ b/go/vt/vtgate/semantics/analyzer.go @@ -328,8 +328,8 @@ func (a *analyzer) noteQuerySignature(node sqlparser.SQLNode) { } case sqlparser.AggrFunc: a.sig.Aggregation = true - case *sqlparser.Delete, *sqlparser.Update: - a.sig.Dml = true + case *sqlparser.Delete, *sqlparser.Update, *sqlparser.Insert: + a.sig.DML = true } } diff --git a/go/vt/vtgate/semantics/semantic_state.go b/go/vt/vtgate/semantics/semantic_state.go index 949a1b72017..74bd9dd1d69 100644 --- a/go/vt/vtgate/semantics/semantic_state.go +++ b/go/vt/vtgate/semantics/semantic_state.go @@ -78,7 +78,7 @@ type ( // QuerySignature is used to identify shortcuts in the planning process QuerySignature struct { Aggregation bool - Dml bool + DML bool Distinct bool HashJoin bool SubQueries bool From 0a3c3176569c9fc89993d1f35860c3af9cff903c Mon Sep 17 00:00:00 2001 From: Andres Taylor Date: Mon, 12 Feb 2024 17:33:04 +0100 Subject: [PATCH 04/12] refactor: create and use a new interface Signed-off-by: Andres Taylor --- go/vt/sqlparser/ast.go | 14 +++++--- go/vt/sqlparser/ast_clone.go | 18 ++++++++++ go/vt/sqlparser/ast_copy_on_rewrite.go | 16 +++++++++ go/vt/sqlparser/ast_equals.go | 33 +++++++++++++++++ go/vt/sqlparser/ast_funcs.go | 17 +++++++++ go/vt/sqlparser/ast_rewrite.go | 16 +++++++++ go/vt/sqlparser/ast_visit.go | 16 +++++++++ .../planbuilder/operators/SQL_builder.go | 35 +++++++++++-------- 8 files changed, 146 insertions(+), 19 deletions(-) diff --git a/go/vt/sqlparser/ast.go b/go/vt/sqlparser/ast.go index 3dd376ff228..dd96a91aea8 100644 --- a/go/vt/sqlparser/ast.go +++ b/go/vt/sqlparser/ast.go @@ -53,16 +53,20 @@ type ( Commented } - // SelectStatement any SELECT statement. - SelectStatement interface { - Statement - InsertRows - iSelectStatement() + OrderAndLimit interface { AddOrder(*Order) SetOrderBy(OrderBy) GetOrderBy() OrderBy GetLimit() *Limit SetLimit(*Limit) + } + + // SelectStatement any SELECT statement. + SelectStatement interface { + Statement + InsertRows + OrderAndLimit + iSelectStatement() GetLock() Lock SetLock(lock Lock) SetInto(into *SelectInto) diff --git a/go/vt/sqlparser/ast_clone.go b/go/vt/sqlparser/ast_clone.go index ef46a348df1..be3ba8c9a53 100644 --- a/go/vt/sqlparser/ast_clone.go +++ b/go/vt/sqlparser/ast_clone.go @@ -3410,6 +3410,24 @@ func CloneRefOfXorExpr(n *XorExpr) *XorExpr { return &out } +// CloneAddToFrom creates a deep clone of the input. +func CloneAddToFrom(in AddToFrom) AddToFrom { + if in == nil { + return nil + } + switch in := in.(type) { + case *Delete: + return CloneRefOfDelete(in) + case *Select: + return CloneRefOfSelect(in) + case *Update: + return CloneRefOfUpdate(in) + default: + // this should never happen + return nil + } +} + // CloneAggrFunc creates a deep clone of the input. func CloneAggrFunc(in AggrFunc) AggrFunc { if in == nil { diff --git a/go/vt/sqlparser/ast_copy_on_rewrite.go b/go/vt/sqlparser/ast_copy_on_rewrite.go index fa90169355a..e1cc1071cd3 100644 --- a/go/vt/sqlparser/ast_copy_on_rewrite.go +++ b/go/vt/sqlparser/ast_copy_on_rewrite.go @@ -6634,6 +6634,22 @@ func (c *cow) copyOnRewriteRefOfXorExpr(n *XorExpr, parent SQLNode) (out SQLNode } return } +func (c *cow) copyOnRewriteAddToFrom(n AddToFrom, parent SQLNode) (out SQLNode, changed bool) { + if n == nil || c.cursor.stop { + return n, false + } + switch n := n.(type) { + case *Delete: + return c.copyOnRewriteRefOfDelete(n, parent) + case *Select: + return c.copyOnRewriteRefOfSelect(n, parent) + case *Update: + return c.copyOnRewriteRefOfUpdate(n, parent) + default: + // this should never happen + return nil, false + } +} func (c *cow) copyOnRewriteAggrFunc(n AggrFunc, parent SQLNode) (out SQLNode, changed bool) { if n == nil || c.cursor.stop { return n, false diff --git a/go/vt/sqlparser/ast_equals.go b/go/vt/sqlparser/ast_equals.go index c3cc5dad18d..2e45d23e4d9 100644 --- a/go/vt/sqlparser/ast_equals.go +++ b/go/vt/sqlparser/ast_equals.go @@ -4916,6 +4916,39 @@ func (cmp *Comparator) RefOfXorExpr(a, b *XorExpr) bool { cmp.Expr(a.Right, b.Right) } +// AddToFrom does deep equals between the two objects. +func (cmp *Comparator) AddToFrom(inA, inB AddToFrom) bool { + if inA == nil && inB == nil { + return true + } + if inA == nil || inB == nil { + return false + } + switch a := inA.(type) { + case *Delete: + b, ok := inB.(*Delete) + if !ok { + return false + } + return cmp.RefOfDelete(a, b) + case *Select: + b, ok := inB.(*Select) + if !ok { + return false + } + return cmp.RefOfSelect(a, b) + case *Update: + b, ok := inB.(*Update) + if !ok { + return false + } + return cmp.RefOfUpdate(a, b) + default: + // this should never happen + return false + } +} + // AggrFunc does deep equals between the two objects. func (cmp *Comparator) AggrFunc(inA, inB AggrFunc) bool { if inA == nil && inB == nil { diff --git a/go/vt/sqlparser/ast_funcs.go b/go/vt/sqlparser/ast_funcs.go index ecbfef2ff66..b723ae955db 100644 --- a/go/vt/sqlparser/ast_funcs.go +++ b/go/vt/sqlparser/ast_funcs.go @@ -2605,3 +2605,20 @@ func MultiTable(node []TableExpr) bool { _, singleTbl := node[0].(*AliasedTableExpr) return !singleTbl } + +type AddToFrom interface { + Statement + AddFrom(tbl TableExpr) +} + +func (node *Select) AddFrom(tbl TableExpr) { + node.From = append(node.From, tbl) +} + +func (node *Update) AddFrom(tbl TableExpr) { + node.TableExprs = append(node.TableExprs, tbl) +} + +func (node *Delete) AddFrom(tbl TableExpr) { + node.TableExprs = append(node.TableExprs, tbl) +} diff --git a/go/vt/sqlparser/ast_rewrite.go b/go/vt/sqlparser/ast_rewrite.go index 87196ee0b2f..f09c3f874a9 100644 --- a/go/vt/sqlparser/ast_rewrite.go +++ b/go/vt/sqlparser/ast_rewrite.go @@ -9538,6 +9538,22 @@ func (a *application) rewriteRefOfXorExpr(parent SQLNode, node *XorExpr, replace } return true } +func (a *application) rewriteAddToFrom(parent SQLNode, node AddToFrom, replacer replacerFunc) bool { + if node == nil { + return true + } + switch node := node.(type) { + case *Delete: + return a.rewriteRefOfDelete(parent, node, replacer) + case *Select: + return a.rewriteRefOfSelect(parent, node, replacer) + case *Update: + return a.rewriteRefOfUpdate(parent, node, replacer) + default: + // this should never happen + return true + } +} func (a *application) rewriteAggrFunc(parent SQLNode, node AggrFunc, replacer replacerFunc) bool { if node == nil { return true diff --git a/go/vt/sqlparser/ast_visit.go b/go/vt/sqlparser/ast_visit.go index 2133bc05330..b979372ee50 100644 --- a/go/vt/sqlparser/ast_visit.go +++ b/go/vt/sqlparser/ast_visit.go @@ -4380,6 +4380,22 @@ func VisitRefOfXorExpr(in *XorExpr, f Visit) error { } return nil } +func VisitAddToFrom(in AddToFrom, f Visit) error { + if in == nil { + return nil + } + switch in := in.(type) { + case *Delete: + return VisitRefOfDelete(in, f) + case *Select: + return VisitRefOfSelect(in, f) + case *Update: + return VisitRefOfUpdate(in, f) + default: + // this should never happen + return nil + } +} func VisitAggrFunc(in AggrFunc, f Visit) error { if in == nil { return nil diff --git a/go/vt/vtgate/planbuilder/operators/SQL_builder.go b/go/vt/vtgate/planbuilder/operators/SQL_builder.go index aeb60cc8d01..9caf05e15fe 100644 --- a/go/vt/vtgate/planbuilder/operators/SQL_builder.go +++ b/go/vt/vtgate/planbuilder/operators/SQL_builder.go @@ -40,6 +40,9 @@ type ( func (qb *queryBuilder) asSelectStatement() sqlparser.SelectStatement { return qb.stmt.(sqlparser.SelectStatement) } +func (qb *queryBuilder) asOrderAndLimit() sqlparser.OrderAndLimit { + return qb.stmt.(sqlparser.OrderAndLimit) +} func ToSQL(ctx *plancontext.PlanningContext, op Operator) (_ sqlparser.Statement, _ Operator, err error) { defer PanicHandler(&err) @@ -70,17 +73,16 @@ func (qb *queryBuilder) addTableExpr( if qb.stmt == nil { qb.stmt = &sqlparser.Select{} } - sel := qb.stmt.(*sqlparser.Select) - elems := &sqlparser.AliasedTableExpr{ + stmt := qb.stmt.(sqlparser.AddToFrom) + tbl := &sqlparser.AliasedTableExpr{ Expr: tblExpr, Partitions: nil, As: sqlparser.NewIdentifierCS(alias), Hints: hints, Columns: columnAliases, } - qb.ctx.SemTable.ReplaceTableSetFor(tableID, elems) - sel.From = append(sel.From, elems) - qb.stmt = sel + qb.ctx.SemTable.ReplaceTableSetFor(tableID, tbl) + stmt.AddFrom(tbl) qb.tableNames = append(qb.tableNames, tableName) } @@ -408,13 +410,7 @@ func buildDelete(op *Delete, qb *queryBuilder) { } func buildUpdate(op *Update, qb *queryBuilder) { - updExprs := make(sqlparser.UpdateExprs, 0, len(op.Assignments)) - for _, se := range op.Assignments { - updExprs = append(updExprs, &sqlparser.UpdateExpr{ - Name: se.Name, - Expr: se.Expr.EvalExpr, - }) - } + updExprs := getUpdateExprs(op) buildQuery(op.Source, qb) // currently the qb builds a select query underneath. @@ -437,6 +433,17 @@ func buildUpdate(op *Update, qb *queryBuilder) { } } +func getUpdateExprs(op *Update) sqlparser.UpdateExprs { + updExprs := make(sqlparser.UpdateExprs, 0, len(op.Assignments)) + for _, se := range op.Assignments { + updExprs = append(updExprs, &sqlparser.UpdateExpr{ + Name: se.Name, + Expr: se.Expr.EvalExpr, + }) + } + return updExprs +} + type OpWithAST interface { Operator Statement() sqlparser.Statement @@ -470,13 +477,13 @@ func buildOrdering(op *Ordering, qb *queryBuilder) { buildQuery(op.Source, qb) for _, order := range op.Order { - qb.asSelectStatement().AddOrder(order.Inner) + qb.asOrderAndLimit().AddOrder(order.Inner) } } func buildLimit(op *Limit, qb *queryBuilder) { buildQuery(op.Source, qb) - qb.asSelectStatement().SetLimit(op.AST) + qb.asOrderAndLimit().SetLimit(op.AST) } func buildTable(op *Table, qb *queryBuilder) { From 0775b6a6c5edc6e40f4531df51ad523d8d76ec4f Mon Sep 17 00:00:00 2001 From: Andres Taylor Date: Mon, 12 Feb 2024 17:45:22 +0100 Subject: [PATCH 05/12] build update using interfaces Signed-off-by: Andres Taylor --- go/vt/sqlparser/ast.go | 4 ++ go/vt/sqlparser/ast_funcs.go | 40 +++++++++++++++++++ .../planbuilder/operators/SQL_builder.go | 23 +++-------- 3 files changed, 49 insertions(+), 18 deletions(-) diff --git a/go/vt/sqlparser/ast.go b/go/vt/sqlparser/ast.go index dd96a91aea8..462fb227890 100644 --- a/go/vt/sqlparser/ast.go +++ b/go/vt/sqlparser/ast.go @@ -716,6 +716,10 @@ type ( IndexType int8 ) +var _ OrderAndLimit = (*Select)(nil) +var _ OrderAndLimit = (*Update)(nil) +var _ OrderAndLimit = (*Delete)(nil) + func (*Union) iStatement() {} func (*Select) iStatement() {} func (*Stream) iStatement() {} diff --git a/go/vt/sqlparser/ast_funcs.go b/go/vt/sqlparser/ast_funcs.go index b723ae955db..06f02ecb637 100644 --- a/go/vt/sqlparser/ast_funcs.go +++ b/go/vt/sqlparser/ast_funcs.go @@ -2622,3 +2622,43 @@ func (node *Update) AddFrom(tbl TableExpr) { func (node *Delete) AddFrom(tbl TableExpr) { node.TableExprs = append(node.TableExprs, tbl) } + +func (node *Update) AddOrder(order *Order) { + node.OrderBy = append(node.OrderBy, order) +} + +func (node *Update) SetOrderBy(by OrderBy) { + node.OrderBy = by +} + +func (node *Update) GetOrderBy() OrderBy { + return node.OrderBy +} + +func (node *Update) GetLimit() *Limit { + return node.Limit +} + +func (node *Update) SetLimit(limit *Limit) { + node.Limit = limit +} + +func (node *Delete) AddOrder(order *Order) { + node.OrderBy = append(node.OrderBy, order) +} + +func (node *Delete) SetOrderBy(by OrderBy) { + node.OrderBy = by +} + +func (node *Delete) GetOrderBy() OrderBy { + return node.OrderBy +} + +func (node *Delete) GetLimit() *Limit { + return node.Limit +} + +func (node *Delete) SetLimit(limit *Limit) { + node.Limit = limit +} diff --git a/go/vt/vtgate/planbuilder/operators/SQL_builder.go b/go/vt/vtgate/planbuilder/operators/SQL_builder.go index 9caf05e15fe..333ec6160d1 100644 --- a/go/vt/vtgate/planbuilder/operators/SQL_builder.go +++ b/go/vt/vtgate/planbuilder/operators/SQL_builder.go @@ -411,26 +411,13 @@ func buildDelete(op *Delete, qb *queryBuilder) { func buildUpdate(op *Update, qb *queryBuilder) { updExprs := getUpdateExprs(op) - - buildQuery(op.Source, qb) - // currently the qb builds a select query underneath. - // Will take the `From` and `Where` from this select - // and create a update statement. - // TODO: change it to directly produce `update` statement. - sel, ok := qb.stmt.(*sqlparser.Select) - if !ok { - panic(vterrors.VT13001("expected a select here")) + upd := &sqlparser.Update{ + Ignore: op.Ignore, + Exprs: updExprs, } - + qb.stmt = upd qb.dmlOperator = op - qb.stmt = &sqlparser.Update{ - Ignore: op.Ignore, - TableExprs: sel.From, - Exprs: updExprs, - Where: sel.Where, - Limit: sel.Limit, - OrderBy: sel.OrderBy, - } + buildQuery(op.Source, qb) } func getUpdateExprs(op *Update) sqlparser.UpdateExprs { From 3e91727e319fbe56e7f0395cfd6ebc60784451ec Mon Sep 17 00:00:00 2001 From: Andres Taylor Date: Mon, 12 Feb 2024 18:15:22 +0100 Subject: [PATCH 06/12] refactor: make delete work well with SQL builder Signed-off-by: Andres Taylor --- go/vt/sqlparser/ast_funcs.go | 65 +++++++++++++++++++ .../planbuilder/operators/SQL_builder.go | 52 +++++++-------- 2 files changed, 91 insertions(+), 26 deletions(-) diff --git a/go/vt/sqlparser/ast_funcs.go b/go/vt/sqlparser/ast_funcs.go index 06f02ecb637..b482baa1321 100644 --- a/go/vt/sqlparser/ast_funcs.go +++ b/go/vt/sqlparser/ast_funcs.go @@ -2662,3 +2662,68 @@ func (node *Delete) GetLimit() *Limit { func (node *Delete) SetLimit(limit *Limit) { node.Limit = limit } + +func (node *Select) GetFrom() []TableExpr { + return node.From +} + +func (node *Select) SetFrom(exprs []TableExpr) { + node.From = exprs +} + +func (node *Select) GetWherePredicate() Expr { + if node.Where == nil { + return nil + } + return node.Where.Expr +} + +func (node *Select) SetWherePredicate(expr Expr) { + node.Where = &Where{ + Type: WhereClause, + Expr: expr, + } +} +func (node *Delete) GetFrom() []TableExpr { + return node.TableExprs +} + +func (node *Delete) SetFrom(exprs []TableExpr) { + node.TableExprs = exprs +} + +func (node *Delete) GetWherePredicate() Expr { + if node.Where == nil { + return nil + } + return node.Where.Expr +} + +func (node *Delete) SetWherePredicate(expr Expr) { + node.Where = &Where{ + Type: WhereClause, + Expr: expr, + } +} + +func (node *Update) GetFrom() []TableExpr { + return node.TableExprs +} + +func (node *Update) SetFrom(exprs []TableExpr) { + node.TableExprs = exprs +} + +func (node *Update) GetWherePredicate() Expr { + if node.Where == nil { + return nil + } + return node.Where.Expr +} + +func (node *Update) SetWherePredicate(expr Expr) { + node.Where = &Where{ + Type: WhereClause, + Expr: expr, + } +} diff --git a/go/vt/vtgate/planbuilder/operators/SQL_builder.go b/go/vt/vtgate/planbuilder/operators/SQL_builder.go index 333ec6160d1..5da26a99616 100644 --- a/go/vt/vtgate/planbuilder/operators/SQL_builder.go +++ b/go/vt/vtgate/planbuilder/operators/SQL_builder.go @@ -203,23 +203,35 @@ func (qb *queryBuilder) unionWith(other *queryBuilder, distinct bool) { } } +type FromStatement interface { + GetFrom() []sqlparser.TableExpr + SetFrom([]sqlparser.TableExpr) + GetWherePredicate() sqlparser.Expr + SetWherePredicate(sqlparser.Expr) +} + +var _ FromStatement = (*sqlparser.Select)(nil) +var _ FromStatement = (*sqlparser.Update)(nil) +var _ FromStatement = (*sqlparser.Delete)(nil) + func (qb *queryBuilder) joinInnerWith(other *queryBuilder, onCondition sqlparser.Expr) { - sel := qb.stmt.(*sqlparser.Select) - otherSel := other.stmt.(*sqlparser.Select) - sel.From = append(sel.From, otherSel.From...) - sel.SelectExprs = append(sel.SelectExprs, otherSel.SelectExprs...) + stmt := qb.stmt.(FromStatement) + otherStmt := other.stmt.(FromStatement) + stmt.SetFrom(append(stmt.GetFrom(), otherStmt.GetFrom()...)) - var predicate sqlparser.Expr - if sel.Where != nil { - predicate = sel.Where.Expr + if sel, isSel := stmt.(*sqlparser.Select); isSel { + otherSel := otherStmt.(*sqlparser.Select) + sel.SelectExprs = append(sel.SelectExprs, otherSel.SelectExprs...) } - if otherSel.Where != nil { + + predicate := stmt.GetWherePredicate() + if otherPredicate := otherStmt.GetWherePredicate(); otherPredicate != nil { predExprs := sqlparser.SplitAndExpression(nil, predicate) - otherExprs := sqlparser.SplitAndExpression(nil, otherSel.Where.Expr) + otherExprs := sqlparser.SplitAndExpression(nil, otherPredicate) predicate = qb.ctx.SemTable.AndExpressions(append(predExprs, otherExprs...)...) } if predicate != nil { - sel.Where = &sqlparser.Where{Type: sqlparser.WhereClause, Expr: predicate} + stmt.SetWherePredicate(predicate) } qb.addPredicate(onCondition) @@ -388,25 +400,13 @@ func buildQuery(op Operator, qb *queryBuilder) { } func buildDelete(op *Delete, qb *queryBuilder) { - buildQuery(op.Source, qb) - // currently the qb builds a select query underneath. - // Will take the `From` and `Where` from this select - // and create a delete statement. - // TODO: change it to directly produce `delete` statement. - sel, ok := qb.stmt.(*sqlparser.Select) - if !ok { - panic(vterrors.VT13001("expected a select here")) + qb.stmt = &sqlparser.Delete{ + Ignore: op.Ignore, + Targets: sqlparser.TableNames{op.Target.Name}, } + buildQuery(op.Source, qb) qb.dmlOperator = op - qb.stmt = &sqlparser.Delete{ - Ignore: op.Ignore, - Targets: sqlparser.TableNames{op.Target.Name}, - TableExprs: sel.From, - Where: sel.Where, - Limit: sel.Limit, - OrderBy: sel.OrderBy, - } } func buildUpdate(op *Update, qb *queryBuilder) { From 8bca5f9139cec957444cee6ddaaade12cc51c208 Mon Sep 17 00:00:00 2001 From: Andres Taylor Date: Mon, 12 Feb 2024 18:19:44 +0100 Subject: [PATCH 07/12] refactor: remove unused interface Signed-off-by: Andres Taylor --- go/vt/sqlparser/ast_clone.go | 18 ---------- go/vt/sqlparser/ast_copy_on_rewrite.go | 16 --------- go/vt/sqlparser/ast_equals.go | 33 ------------------- go/vt/sqlparser/ast_funcs.go | 5 --- go/vt/sqlparser/ast_rewrite.go | 16 --------- go/vt/sqlparser/ast_visit.go | 16 --------- .../planbuilder/operators/SQL_builder.go | 4 +-- 7 files changed, 2 insertions(+), 106 deletions(-) diff --git a/go/vt/sqlparser/ast_clone.go b/go/vt/sqlparser/ast_clone.go index be3ba8c9a53..ef46a348df1 100644 --- a/go/vt/sqlparser/ast_clone.go +++ b/go/vt/sqlparser/ast_clone.go @@ -3410,24 +3410,6 @@ func CloneRefOfXorExpr(n *XorExpr) *XorExpr { return &out } -// CloneAddToFrom creates a deep clone of the input. -func CloneAddToFrom(in AddToFrom) AddToFrom { - if in == nil { - return nil - } - switch in := in.(type) { - case *Delete: - return CloneRefOfDelete(in) - case *Select: - return CloneRefOfSelect(in) - case *Update: - return CloneRefOfUpdate(in) - default: - // this should never happen - return nil - } -} - // CloneAggrFunc creates a deep clone of the input. func CloneAggrFunc(in AggrFunc) AggrFunc { if in == nil { diff --git a/go/vt/sqlparser/ast_copy_on_rewrite.go b/go/vt/sqlparser/ast_copy_on_rewrite.go index e1cc1071cd3..fa90169355a 100644 --- a/go/vt/sqlparser/ast_copy_on_rewrite.go +++ b/go/vt/sqlparser/ast_copy_on_rewrite.go @@ -6634,22 +6634,6 @@ func (c *cow) copyOnRewriteRefOfXorExpr(n *XorExpr, parent SQLNode) (out SQLNode } return } -func (c *cow) copyOnRewriteAddToFrom(n AddToFrom, parent SQLNode) (out SQLNode, changed bool) { - if n == nil || c.cursor.stop { - return n, false - } - switch n := n.(type) { - case *Delete: - return c.copyOnRewriteRefOfDelete(n, parent) - case *Select: - return c.copyOnRewriteRefOfSelect(n, parent) - case *Update: - return c.copyOnRewriteRefOfUpdate(n, parent) - default: - // this should never happen - return nil, false - } -} func (c *cow) copyOnRewriteAggrFunc(n AggrFunc, parent SQLNode) (out SQLNode, changed bool) { if n == nil || c.cursor.stop { return n, false diff --git a/go/vt/sqlparser/ast_equals.go b/go/vt/sqlparser/ast_equals.go index 2e45d23e4d9..c3cc5dad18d 100644 --- a/go/vt/sqlparser/ast_equals.go +++ b/go/vt/sqlparser/ast_equals.go @@ -4916,39 +4916,6 @@ func (cmp *Comparator) RefOfXorExpr(a, b *XorExpr) bool { cmp.Expr(a.Right, b.Right) } -// AddToFrom does deep equals between the two objects. -func (cmp *Comparator) AddToFrom(inA, inB AddToFrom) bool { - if inA == nil && inB == nil { - return true - } - if inA == nil || inB == nil { - return false - } - switch a := inA.(type) { - case *Delete: - b, ok := inB.(*Delete) - if !ok { - return false - } - return cmp.RefOfDelete(a, b) - case *Select: - b, ok := inB.(*Select) - if !ok { - return false - } - return cmp.RefOfSelect(a, b) - case *Update: - b, ok := inB.(*Update) - if !ok { - return false - } - return cmp.RefOfUpdate(a, b) - default: - // this should never happen - return false - } -} - // AggrFunc does deep equals between the two objects. func (cmp *Comparator) AggrFunc(inA, inB AggrFunc) bool { if inA == nil && inB == nil { diff --git a/go/vt/sqlparser/ast_funcs.go b/go/vt/sqlparser/ast_funcs.go index b482baa1321..276888e3da3 100644 --- a/go/vt/sqlparser/ast_funcs.go +++ b/go/vt/sqlparser/ast_funcs.go @@ -2606,11 +2606,6 @@ func MultiTable(node []TableExpr) bool { return !singleTbl } -type AddToFrom interface { - Statement - AddFrom(tbl TableExpr) -} - func (node *Select) AddFrom(tbl TableExpr) { node.From = append(node.From, tbl) } diff --git a/go/vt/sqlparser/ast_rewrite.go b/go/vt/sqlparser/ast_rewrite.go index f09c3f874a9..87196ee0b2f 100644 --- a/go/vt/sqlparser/ast_rewrite.go +++ b/go/vt/sqlparser/ast_rewrite.go @@ -9538,22 +9538,6 @@ func (a *application) rewriteRefOfXorExpr(parent SQLNode, node *XorExpr, replace } return true } -func (a *application) rewriteAddToFrom(parent SQLNode, node AddToFrom, replacer replacerFunc) bool { - if node == nil { - return true - } - switch node := node.(type) { - case *Delete: - return a.rewriteRefOfDelete(parent, node, replacer) - case *Select: - return a.rewriteRefOfSelect(parent, node, replacer) - case *Update: - return a.rewriteRefOfUpdate(parent, node, replacer) - default: - // this should never happen - return true - } -} func (a *application) rewriteAggrFunc(parent SQLNode, node AggrFunc, replacer replacerFunc) bool { if node == nil { return true diff --git a/go/vt/sqlparser/ast_visit.go b/go/vt/sqlparser/ast_visit.go index b979372ee50..2133bc05330 100644 --- a/go/vt/sqlparser/ast_visit.go +++ b/go/vt/sqlparser/ast_visit.go @@ -4380,22 +4380,6 @@ func VisitRefOfXorExpr(in *XorExpr, f Visit) error { } return nil } -func VisitAddToFrom(in AddToFrom, f Visit) error { - if in == nil { - return nil - } - switch in := in.(type) { - case *Delete: - return VisitRefOfDelete(in, f) - case *Select: - return VisitRefOfSelect(in, f) - case *Update: - return VisitRefOfUpdate(in, f) - default: - // this should never happen - return nil - } -} func VisitAggrFunc(in AggrFunc, f Visit) error { if in == nil { return nil diff --git a/go/vt/vtgate/planbuilder/operators/SQL_builder.go b/go/vt/vtgate/planbuilder/operators/SQL_builder.go index 5da26a99616..307bb5e2298 100644 --- a/go/vt/vtgate/planbuilder/operators/SQL_builder.go +++ b/go/vt/vtgate/planbuilder/operators/SQL_builder.go @@ -73,7 +73,7 @@ func (qb *queryBuilder) addTableExpr( if qb.stmt == nil { qb.stmt = &sqlparser.Select{} } - stmt := qb.stmt.(sqlparser.AddToFrom) + stmt := qb.stmt.(FromStatement) tbl := &sqlparser.AliasedTableExpr{ Expr: tblExpr, Partitions: nil, @@ -82,7 +82,7 @@ func (qb *queryBuilder) addTableExpr( Columns: columnAliases, } qb.ctx.SemTable.ReplaceTableSetFor(tableID, tbl) - stmt.AddFrom(tbl) + stmt.SetFrom(append(stmt.GetFrom(), tbl)) qb.tableNames = append(qb.tableNames, tableName) } From c3176fd25f587eb5a2b2684a21170aa9540b4916 Mon Sep 17 00:00:00 2001 From: Andres Taylor Date: Mon, 12 Feb 2024 18:29:12 +0100 Subject: [PATCH 08/12] refactor: use FromStatement for outer joins Signed-off-by: Andres Taylor --- .../planbuilder/operators/SQL_builder.go | 41 ++++++++----------- 1 file changed, 18 insertions(+), 23 deletions(-) diff --git a/go/vt/vtgate/planbuilder/operators/SQL_builder.go b/go/vt/vtgate/planbuilder/operators/SQL_builder.go index 307bb5e2298..48b12517f2a 100644 --- a/go/vt/vtgate/planbuilder/operators/SQL_builder.go +++ b/go/vt/vtgate/planbuilder/operators/SQL_builder.go @@ -224,6 +224,11 @@ func (qb *queryBuilder) joinInnerWith(other *queryBuilder, onCondition sqlparser sel.SelectExprs = append(sel.SelectExprs, otherSel.SelectExprs...) } + qb.mergeWhereClauses(stmt, otherStmt) + qb.addPredicate(onCondition) +} + +func (qb *queryBuilder) mergeWhereClauses(stmt, otherStmt FromStatement) { predicate := stmt.GetWherePredicate() if otherPredicate := otherStmt.GetWherePredicate(); otherPredicate != nil { predExprs := sqlparser.SplitAndExpression(nil, predicate) @@ -233,45 +238,35 @@ func (qb *queryBuilder) joinInnerWith(other *queryBuilder, onCondition sqlparser if predicate != nil { stmt.SetWherePredicate(predicate) } - - qb.addPredicate(onCondition) } func (qb *queryBuilder) joinOuterWith(other *queryBuilder, onCondition sqlparser.Expr) { - sel := qb.stmt.(*sqlparser.Select) - otherSel := other.stmt.(*sqlparser.Select) + stmt := qb.stmt.(FromStatement) + otherStmt := other.stmt.(FromStatement) var lhs sqlparser.TableExpr - if len(sel.From) == 1 { - lhs = sel.From[0] + fromClause := stmt.GetFrom() + if len(fromClause) == 1 { + lhs = fromClause[0] } else { - lhs = &sqlparser.ParenTableExpr{Exprs: sel.From} + lhs = &sqlparser.ParenTableExpr{Exprs: fromClause} } var rhs sqlparser.TableExpr - if len(otherSel.From) == 1 { - rhs = otherSel.From[0] + otherFromClause := otherStmt.GetFrom() + if len(otherFromClause) == 1 { + rhs = otherFromClause[0] } else { - rhs = &sqlparser.ParenTableExpr{Exprs: otherSel.From} + rhs = &sqlparser.ParenTableExpr{Exprs: otherFromClause} } - sel.From = []sqlparser.TableExpr{&sqlparser.JoinTableExpr{ + stmt.SetFrom([]sqlparser.TableExpr{&sqlparser.JoinTableExpr{ LeftExpr: lhs, RightExpr: rhs, Join: sqlparser.LeftJoinType, Condition: &sqlparser.JoinCondition{ On: onCondition, }, - }} + }}) - sel.SelectExprs = append(sel.SelectExprs, otherSel.SelectExprs...) - var predicate sqlparser.Expr - if sel.Where != nil { - predicate = sel.Where.Expr - } - if otherSel.Where != nil { - predicate = qb.ctx.SemTable.AndExpressions(predicate, otherSel.Where.Expr) - } - if predicate != nil { - sel.Where = &sqlparser.Where{Type: sqlparser.WhereClause, Expr: predicate} - } + qb.mergeWhereClauses(stmt, otherStmt) } func (qb *queryBuilder) sortTables() { From 529cc7a772ed6d272dffeac9bc7790a25033559e Mon Sep 17 00:00:00 2001 From: Andres Taylor Date: Mon, 12 Feb 2024 18:35:08 +0100 Subject: [PATCH 09/12] refactor: make the sql builder handle inner and outer joins similarly Signed-off-by: Andres Taylor --- .../planbuilder/operators/SQL_builder.go | 31 +++++++++++++------ 1 file changed, 21 insertions(+), 10 deletions(-) diff --git a/go/vt/vtgate/planbuilder/operators/SQL_builder.go b/go/vt/vtgate/planbuilder/operators/SQL_builder.go index 48b12517f2a..88f9d985d81 100644 --- a/go/vt/vtgate/planbuilder/operators/SQL_builder.go +++ b/go/vt/vtgate/planbuilder/operators/SQL_builder.go @@ -73,7 +73,6 @@ func (qb *queryBuilder) addTableExpr( if qb.stmt == nil { qb.stmt = &sqlparser.Select{} } - stmt := qb.stmt.(FromStatement) tbl := &sqlparser.AliasedTableExpr{ Expr: tblExpr, Partitions: nil, @@ -82,7 +81,7 @@ func (qb *queryBuilder) addTableExpr( Columns: columnAliases, } qb.ctx.SemTable.ReplaceTableSetFor(tableID, tbl) - stmt.SetFrom(append(stmt.GetFrom(), tbl)) + qb.stmt.(FromStatement).SetFrom(append(qb.stmt.(FromStatement).GetFrom(), tbl)) qb.tableNames = append(qb.tableNames, tableName) } @@ -217,17 +216,32 @@ var _ FromStatement = (*sqlparser.Delete)(nil) func (qb *queryBuilder) joinInnerWith(other *queryBuilder, onCondition sqlparser.Expr) { stmt := qb.stmt.(FromStatement) otherStmt := other.stmt.(FromStatement) - stmt.SetFrom(append(stmt.GetFrom(), otherStmt.GetFrom()...)) if sel, isSel := stmt.(*sqlparser.Select); isSel { otherSel := otherStmt.(*sqlparser.Select) sel.SelectExprs = append(sel.SelectExprs, otherSel.SelectExprs...) } + newFromClause := append(stmt.GetFrom(), otherStmt.GetFrom()...) + stmt.SetFrom(newFromClause) qb.mergeWhereClauses(stmt, otherStmt) qb.addPredicate(onCondition) } +func (qb *queryBuilder) joinOuterWith(other *queryBuilder, onCondition sqlparser.Expr) { + stmt := qb.stmt.(FromStatement) + otherStmt := other.stmt.(FromStatement) + + if sel, isSel := stmt.(*sqlparser.Select); isSel { + otherSel := otherStmt.(*sqlparser.Select) + sel.SelectExprs = append(sel.SelectExprs, otherSel.SelectExprs...) + } + + newFromClause := []sqlparser.TableExpr{buildOuterJoin(stmt, otherStmt, onCondition)} + stmt.SetFrom(newFromClause) + qb.mergeWhereClauses(stmt, otherStmt) +} + func (qb *queryBuilder) mergeWhereClauses(stmt, otherStmt FromStatement) { predicate := stmt.GetWherePredicate() if otherPredicate := otherStmt.GetWherePredicate(); otherPredicate != nil { @@ -240,9 +254,7 @@ func (qb *queryBuilder) mergeWhereClauses(stmt, otherStmt FromStatement) { } } -func (qb *queryBuilder) joinOuterWith(other *queryBuilder, onCondition sqlparser.Expr) { - stmt := qb.stmt.(FromStatement) - otherStmt := other.stmt.(FromStatement) +func buildOuterJoin(stmt FromStatement, otherStmt FromStatement, onCondition sqlparser.Expr) *sqlparser.JoinTableExpr { var lhs sqlparser.TableExpr fromClause := stmt.GetFrom() if len(fromClause) == 1 { @@ -257,16 +269,15 @@ func (qb *queryBuilder) joinOuterWith(other *queryBuilder, onCondition sqlparser } else { rhs = &sqlparser.ParenTableExpr{Exprs: otherFromClause} } - stmt.SetFrom([]sqlparser.TableExpr{&sqlparser.JoinTableExpr{ + + return &sqlparser.JoinTableExpr{ LeftExpr: lhs, RightExpr: rhs, Join: sqlparser.LeftJoinType, Condition: &sqlparser.JoinCondition{ On: onCondition, }, - }}) - - qb.mergeWhereClauses(stmt, otherStmt) + } } func (qb *queryBuilder) sortTables() { From c33b5120da5d6cf4bb3c46a508e3d696ee2cb4df Mon Sep 17 00:00:00 2001 From: Harshit Gangal Date: Tue, 13 Feb 2024 15:23:29 +0530 Subject: [PATCH 10/12] added release notes Signed-off-by: Harshit Gangal --- changelog/20.0/20.0.0/summary.md | 23 +++++++++++++++++++++-- go/vt/vtgate/semantics/analyzer_test.go | 25 ------------------------- 2 files changed, 21 insertions(+), 27 deletions(-) diff --git a/changelog/20.0/20.0.0/summary.md b/changelog/20.0/20.0.0/summary.md index c947ea2a0aa..41fa729e165 100644 --- a/changelog/20.0/20.0.0/summary.md +++ b/changelog/20.0/20.0.0/summary.md @@ -3,15 +3,17 @@ ### Table of Contents - **[Major Changes](#major-changes)** - - **[Query Serving](#query-serving)** + - **[Query Compatibility](#query-compatibility)** - [Vindex Hints](#vindex-hints) + - [Update with Limit Support](#update-limit) + - [Update with Multi Table Support](#multi-table-update) - **[Minor Changes](#minor-changes)** ## Major Changes -### Query Serving +### Query Serving #### Vindex Hints @@ -25,5 +27,22 @@ Example: For more information about Vindex hints and its usage, please consult the documentation. +#### Update with Limit Support + +Support is added for sharded update with limit. + +Example: `update t1 set t1.foo = 'abc', t1.bar = 23 where t1.baz > 5 limit 1` + +More details about how it works is available in [MySQL Docs](https://dev.mysql.com/doc/refman/8.0/en/update.html) + +#### Update with Multi Table Support + +Support is added for sharded multi-table update with column update on single target table using multiple table join. + +Example: `update t1 join t2 on t1.id = t2.id join t3 on t1.col = t3.col set t1.baz = 'abc', t1.apa = 23 where t3.foo = 5 and t2.bar = 7` + +More details about how it works is available in [MySQL Docs](https://dev.mysql.com/doc/refman/8.0/en/update.html) + + ## Minor Changes diff --git a/go/vt/vtgate/semantics/analyzer_test.go b/go/vt/vtgate/semantics/analyzer_test.go index 10f85326751..dd8db6f5cd1 100644 --- a/go/vt/vtgate/semantics/analyzer_test.go +++ b/go/vt/vtgate/semantics/analyzer_test.go @@ -1589,31 +1589,6 @@ func TestNextErrors(t *testing.T) { } } -func TestUpdateErrors(t *testing.T) { - tests := []struct { - query, expectedError string - }{ - { - query: "update t1, t2 set id = 12", - expectedError: "VT12001: unsupported: multiple (2) tables in update", - }, { - query: "update (select 1 from dual) dt set id = 1", - expectedError: "The target table dt of the UPDATE is not updatable", - }, - } - - for _, test := range tests { - t.Run(test.query, func(t *testing.T) { - parse, err := sqlparser.NewTestParser().Parse(test.query) - require.NoError(t, err) - - _, err = AnalyzeStrict(parse, "d", fakeSchemaInfo()) - - assert.EqualError(t, err, test.expectedError) - }) - } -} - // TestScopingSubQueryJoinClause tests the scoping behavior of a subquery containing a join clause. // The test ensures that the scoping analysis correctly identifies and handles the relationships // between the tables involved in the join operation with the outer query. From 3d456e8b7ddcf65650daad47ca4bb3e354575aca Mon Sep 17 00:00:00 2001 From: Harshit Gangal Date: Tue, 13 Feb 2024 17:17:23 +0530 Subject: [PATCH 11/12] addressed review comments Signed-off-by: Harshit Gangal --- go/vt/vtgate/planbuilder/testdata/dml_cases.json | 10 ++++++++++ .../planbuilder/testdata/unsupported_cases.json | 12 +----------- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/go/vt/vtgate/planbuilder/testdata/dml_cases.json b/go/vt/vtgate/planbuilder/testdata/dml_cases.json index 4eca7a57fb2..995795fc209 100644 --- a/go/vt/vtgate/planbuilder/testdata/dml_cases.json +++ b/go/vt/vtgate/planbuilder/testdata/dml_cases.json @@ -5818,5 +5818,15 @@ "user.user_extra" ] } + }, + { + "comment": "update with multi table reference with multi target update on a derived table", + "query": "update ignore (select foo, col, bar from user) u, music m set u.foo = 21, u.bar = 'abc' where u.col = m.col", + "plan": "VT03031: the target table (select foo, col, bar from `user`) as u of the UPDATE is not updatable" + }, + { + "comment": "update with derived table", + "query": "update (select id from user) as u set id = 4", + "plan": "VT03031: the target table (select id from `user`) as u of the UPDATE is not updatable" } ] diff --git a/go/vt/vtgate/planbuilder/testdata/unsupported_cases.json b/go/vt/vtgate/planbuilder/testdata/unsupported_cases.json index 84d70fb24f3..4214a396499 100644 --- a/go/vt/vtgate/planbuilder/testdata/unsupported_cases.json +++ b/go/vt/vtgate/planbuilder/testdata/unsupported_cases.json @@ -395,18 +395,8 @@ "plan": "VT12001: unsupported: only one DISTINCT aggregation is allowed in a SELECT: sum(distinct id)" }, { - "comment": "update with derived table", - "query": "update (select id from user) as u set id = 4", - "plan": "VT03031: the target table (select id from `user`) as u of the UPDATE is not updatable" - }, - { - "comment": "Add your test case here for debugging and run go test -run=One.", + "comment": "update with multi table reference with multi target update", "query": "update ignore user u, music m set u.foo = 21, m.bar = 'abc' where u.col = m.col", "plan": "VT12001: unsupported: multi-table UPDATE statement with multi-target column update" - }, - { - "comment": "Add your test case here for debugging and run go test -run=One.", - "query": "update ignore (select foo, col, bar from user) u, music m set u.foo = 21, u.bar = 'abc' where u.col = m.col", - "plan": "VT03031: the target table (select foo, col, bar from `user`) as u of the UPDATE is not updatable" } ] From a52c54ab0d3cb13027b027966ab3b1cfdb55dd98 Mon Sep 17 00:00:00 2001 From: Harshit Gangal Date: Tue, 13 Feb 2024 20:43:56 +0530 Subject: [PATCH 12/12] fix typo and some refactor Signed-off-by: Harshit Gangal --- changelog/20.0/20.0.0/summary.md | 2 +- go/vt/sqlparser/ast.go | 6 +++--- go/vt/sqlparser/ast_funcs.go | 36 -------------------------------- 3 files changed, 4 insertions(+), 40 deletions(-) diff --git a/changelog/20.0/20.0.0/summary.md b/changelog/20.0/20.0.0/summary.md index 41fa729e165..f1280ba7e76 100644 --- a/changelog/20.0/20.0.0/summary.md +++ b/changelog/20.0/20.0.0/summary.md @@ -13,7 +13,7 @@ ## Major Changes -### Query Serving +### Query Compatibility #### Vindex Hints diff --git a/go/vt/sqlparser/ast.go b/go/vt/sqlparser/ast.go index 462fb227890..0e50d71b77c 100644 --- a/go/vt/sqlparser/ast.go +++ b/go/vt/sqlparser/ast.go @@ -55,9 +55,6 @@ type ( OrderAndLimit interface { AddOrder(*Order) - SetOrderBy(OrderBy) - GetOrderBy() OrderBy - GetLimit() *Limit SetLimit(*Limit) } @@ -76,6 +73,9 @@ type ( GetColumns() SelectExprs Commented IsDistinct() bool + GetOrderBy() OrderBy + SetOrderBy(OrderBy) + GetLimit() *Limit } // DDLStatement represents any DDL Statement diff --git a/go/vt/sqlparser/ast_funcs.go b/go/vt/sqlparser/ast_funcs.go index 276888e3da3..a8d231d6aa1 100644 --- a/go/vt/sqlparser/ast_funcs.go +++ b/go/vt/sqlparser/ast_funcs.go @@ -2606,34 +2606,10 @@ func MultiTable(node []TableExpr) bool { return !singleTbl } -func (node *Select) AddFrom(tbl TableExpr) { - node.From = append(node.From, tbl) -} - -func (node *Update) AddFrom(tbl TableExpr) { - node.TableExprs = append(node.TableExprs, tbl) -} - -func (node *Delete) AddFrom(tbl TableExpr) { - node.TableExprs = append(node.TableExprs, tbl) -} - func (node *Update) AddOrder(order *Order) { node.OrderBy = append(node.OrderBy, order) } -func (node *Update) SetOrderBy(by OrderBy) { - node.OrderBy = by -} - -func (node *Update) GetOrderBy() OrderBy { - return node.OrderBy -} - -func (node *Update) GetLimit() *Limit { - return node.Limit -} - func (node *Update) SetLimit(limit *Limit) { node.Limit = limit } @@ -2642,18 +2618,6 @@ func (node *Delete) AddOrder(order *Order) { node.OrderBy = append(node.OrderBy, order) } -func (node *Delete) SetOrderBy(by OrderBy) { - node.OrderBy = by -} - -func (node *Delete) GetOrderBy() OrderBy { - return node.OrderBy -} - -func (node *Delete) GetLimit() *Limit { - return node.Limit -} - func (node *Delete) SetLimit(limit *Limit) { node.Limit = limit }