Skip to content

Commit 192fa94

Browse files
vitess-bot[bot]harshit-gangal
vitess-bot[bot]
and
harshit-gangal
authored
[release-20.0] Reference Table DML Join Fix (#17414) (#17473)
Signed-off-by: Harshit Gangal <[email protected]> Co-authored-by: Harshit Gangal <[email protected]>
1 parent 6b3c47c commit 192fa94

File tree

11 files changed

+192
-22
lines changed

11 files changed

+192
-22
lines changed

‎go/vt/sqlparser/ast_funcs.go‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2285,6 +2285,25 @@ func RemoveKeyspace(in SQLNode) {
22852285
})
22862286
}
22872287

2288+
// RemoveKeyspaceIgnoreSysSchema removes the Qualifier.Qualifier on all ColNames and Qualifier on all TableNames in the AST
2289+
// except for the system schema.
2290+
func RemoveKeyspaceIgnoreSysSchema(in SQLNode) {
2291+
Rewrite(in, nil, func(cursor *Cursor) bool {
2292+
switch expr := cursor.Node().(type) {
2293+
case *ColName:
2294+
if expr.Qualifier.Qualifier.NotEmpty() && !SystemSchema(expr.Qualifier.Qualifier.String()) {
2295+
expr.Qualifier.Qualifier = NewIdentifierCS("")
2296+
}
2297+
case TableName:
2298+
if expr.Qualifier.NotEmpty() && !SystemSchema(expr.Qualifier.String()) {
2299+
expr.Qualifier = NewIdentifierCS("")
2300+
cursor.Replace(expr)
2301+
}
2302+
}
2303+
return true
2304+
})
2305+
}
2306+
22882307
func convertStringToInt(integer string) int {
22892308
val, _ := strconv.Atoi(integer)
22902309
return val

‎go/vt/vtgate/planbuilder/operators/delete.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -130,7 +130,7 @@ func createDeleteWithInputOp(ctx *plancontext.PlanningContext, del *sqlparser.De
130130
}
131131

132132
var delOps []dmlOp
133-
for _, target := range ctx.SemTable.Targets.Constituents() {
133+
for _, target := range ctx.SemTable.DMLTargets.Constituents() {
134134
op := createDeleteOpWithTarget(ctx, target, del.Ignore)
135135
delOps = append(delOps, op)
136136
}
@@ -336,7 +336,7 @@ func updateQueryGraphWithSource(ctx *plancontext.PlanningContext, input Operator
336336
return op, NoRewrite
337337
}
338338
if len(qg.Tables) > 1 {
339-
panic(vterrors.VT12001("DELETE on reference table with join"))
339+
panic(vterrors.VT12001("DML on reference table with join"))
340340
}
341341
for _, tbl := range qg.Tables {
342342
if tbl.ID != tblID {

‎go/vt/vtgate/planbuilder/operators/join_merging.go‎

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ import (
2929
// If they can be merged, a new operator with the merged routing is returned
3030
// If they cannot be merged, nil is returned.
3131
func mergeJoinInputs(ctx *plancontext.PlanningContext, lhs, rhs Operator, joinPredicates []sqlparser.Expr, m *joinMerger) *Route {
32-
lhsRoute, rhsRoute, routingA, routingB, a, b, sameKeyspace := prepareInputRoutes(lhs, rhs)
32+
lhsRoute, rhsRoute, routingA, routingB, a, b, sameKeyspace := prepareInputRoutes(ctx, lhs, rhs)
3333
if lhsRoute == nil {
3434
return nil
3535
}
@@ -91,13 +91,13 @@ func mergeAnyShardRoutings(ctx *plancontext.PlanningContext, a, b *AnyShardRouti
9191
}
9292
}
9393

94-
func prepareInputRoutes(lhs Operator, rhs Operator) (*Route, *Route, Routing, Routing, routingType, routingType, bool) {
94+
func prepareInputRoutes(ctx *plancontext.PlanningContext, lhs Operator, rhs Operator) (*Route, *Route, Routing, Routing, routingType, routingType, bool) {
9595
lhsRoute, rhsRoute := operatorsToRoutes(lhs, rhs)
9696
if lhsRoute == nil || rhsRoute == nil {
9797
return nil, nil, nil, nil, 0, 0, false
9898
}
9999

100-
lhsRoute, rhsRoute, routingA, routingB, sameKeyspace := getRoutesOrAlternates(lhsRoute, rhsRoute)
100+
lhsRoute, rhsRoute, routingA, routingB, sameKeyspace := getRoutesOrAlternates(ctx, lhsRoute, rhsRoute)
101101

102102
a, b := getRoutingType(routingA), getRoutingType(routingB)
103103
if getTypeName(routingA) < getTypeName(routingB) {
@@ -155,7 +155,7 @@ func (rt routingType) String() string {
155155

156156
// getRoutesOrAlternates gets the Routings from each Route. If they are from different keyspaces,
157157
// we check if this is a table with alternates in other keyspaces that we can use
158-
func getRoutesOrAlternates(lhsRoute, rhsRoute *Route) (*Route, *Route, Routing, Routing, bool) {
158+
func getRoutesOrAlternates(ctx *plancontext.PlanningContext, lhsRoute, rhsRoute *Route) (*Route, *Route, Routing, Routing, bool) {
159159
routingA := lhsRoute.Routing
160160
routingB := rhsRoute.Routing
161161
sameKeyspace := routingA.Keyspace() == routingB.Keyspace()
@@ -167,13 +167,17 @@ func getRoutesOrAlternates(lhsRoute, rhsRoute *Route) (*Route, *Route, Routing,
167167
return lhsRoute, rhsRoute, routingA, routingB, sameKeyspace
168168
}
169169

170-
if refA, ok := routingA.(*AnyShardRouting); ok {
170+
// If we have a reference route, we will try to find an alternate route in same keyspace as other routing keyspace.
171+
// If the reference route is part of DML table update target, alternate keyspace route cannot be considered.
172+
if refA, ok := routingA.(*AnyShardRouting); ok &&
173+
!TableID(lhsRoute).IsOverlapping(ctx.SemTable.DMLTargets) {
171174
if altARoute := refA.AlternateInKeyspace(routingB.Keyspace()); altARoute != nil {
172175
return altARoute, rhsRoute, altARoute.Routing, routingB, true
173176
}
174177
}
175178

176-
if refB, ok := routingB.(*AnyShardRouting); ok {
179+
if refB, ok := routingB.(*AnyShardRouting); ok &&
180+
!TableID(rhsRoute).IsOverlapping(ctx.SemTable.DMLTargets) {
177181
if altBRoute := refB.AlternateInKeyspace(routingA.Keyspace()); altBRoute != nil {
178182
return lhsRoute, altBRoute, routingA, altBRoute.Routing, true
179183
}

‎go/vt/vtgate/planbuilder/operators/subquery_planning.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -694,7 +694,7 @@ func mergeSubqueryInputs(ctx *plancontext.PlanningContext, in, out Operator, joi
694694
return nil
695695
}
696696

697-
inRoute, outRoute, inRouting, outRouting, sameKeyspace := getRoutesOrAlternates(inRoute, outRoute)
697+
inRoute, outRoute, inRouting, outRouting, sameKeyspace := getRoutesOrAlternates(ctx, inRoute, outRoute)
698698
inner, outer := getRoutingType(inRouting), getRoutingType(outRouting)
699699

700700
switch {

‎go/vt/vtgate/planbuilder/operators/union_merging.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -108,7 +108,7 @@ func mergeUnionInputs(
108108
lhsExprs, rhsExprs sqlparser.SelectExprs,
109109
distinct bool,
110110
) (Operator, sqlparser.SelectExprs) {
111-
lhsRoute, rhsRoute, routingA, routingB, a, b, sameKeyspace := prepareInputRoutes(lhs, rhs)
111+
lhsRoute, rhsRoute, routingA, routingB, a, b, sameKeyspace := prepareInputRoutes(ctx, lhs, rhs)
112112
if lhsRoute == nil {
113113
return nil, nil
114114
}

‎go/vt/vtgate/planbuilder/operators/update.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -171,7 +171,7 @@ func createUpdateWithInputOp(ctx *plancontext.PlanningContext, upd *sqlparser.Up
171171
ueMap := prepareUpdateExpressionList(ctx, upd)
172172

173173
var updOps []dmlOp
174-
for _, target := range ctx.SemTable.Targets.Constituents() {
174+
for _, target := range ctx.SemTable.DMLTargets.Constituents() {
175175
op := createUpdateOpWithTarget(ctx, upd, target, ueMap[target])
176176
updOps = append(updOps, op)
177177
}
@@ -318,7 +318,7 @@ func errIfUpdateNotSupported(ctx *plancontext.PlanningContext, stmt *sqlparser.U
318318
}
319319
}
320320

321-
// Now we check if any of the foreign key columns that are being udpated have dependencies on other updated columns.
321+
// Now we check if any of the foreign key columns that are being updated have dependencies on other updated columns.
322322
// This is unsafe, and we currently don't support this in Vitess.
323323
if err := ctx.SemTable.ErrIfFkDependentColumnUpdated(stmt.Exprs); err != nil {
324324
panic(err)

‎go/vt/vtgate/planbuilder/plan_test.go‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -85,6 +85,7 @@ func (s *planTestSuite) TestPlan() {
8585
s.addPKsProvided(vschemaWrapper.V, "user", []string{"user_extra"}, []string{"id", "user_id"})
8686
s.addPKsProvided(vschemaWrapper.V, "ordering", []string{"order"}, []string{"oid", "region_id"})
8787
s.addPKsProvided(vschemaWrapper.V, "ordering", []string{"order_event"}, []string{"oid", "ename"})
88+
s.addPKsProvided(vschemaWrapper.V, "main", []string{"source_of_ref"}, []string{"id"})
8889

8990
// You will notice that some tests expect user.Id instead of user.id.
9091
// This is because we now pre-create vindex columns in the symbol
@@ -304,6 +305,7 @@ func (s *planTestSuite) TestOne() {
304305
s.addPKsProvided(lv, "user", []string{"user_extra"}, []string{"id", "user_id"})
305306
s.addPKsProvided(lv, "ordering", []string{"order"}, []string{"oid", "region_id"})
306307
s.addPKsProvided(lv, "ordering", []string{"order_event"}, []string{"oid", "ename"})
308+
s.addPKsProvided(lv, "main", []string{"source_of_ref"}, []string{"id"})
307309
vschema := &vschemawrapper.VSchemaWrapper{
308310
V: lv,
309311
TestBuilder: TestBuilder,
@@ -684,7 +686,7 @@ func (s *planTestSuite) testFile(filename string, vschema *vschemawrapper.VSchem
684686
if tcase.Skip {
685687
t.Skip(message)
686688
} else {
687-
t.Errorf(message)
689+
t.Error(message)
688690
}
689691
} else if tcase.Skip {
690692
t.Errorf("query is correct even though it is skipped:\n %s", tcase.Query)

‎go/vt/vtgate/planbuilder/testdata/reference_cases.json‎

Lines changed: 140 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -771,5 +771,145 @@
771771
"user.user_extra"
772772
]
773773
}
774+
},
775+
{
776+
"comment": "update reference table with join on sharded table",
777+
"query": "update main.source_of_ref as sr join main.rerouted_ref as rr on sr.id = rr.id inner join user.music as m on sr.col = m.col set sr.tt = 5 where m.user_id = 1",
778+
"plan": {
779+
"QueryType": "UPDATE",
780+
"Original": "update main.source_of_ref as sr join main.rerouted_ref as rr on sr.id = rr.id inner join user.music as m on sr.col = m.col set sr.tt = 5 where m.user_id = 1",
781+
"Instructions": {
782+
"OperatorType": "DMLWithInput",
783+
"TargetTabletType": "PRIMARY",
784+
"Offset": [
785+
"0:[0]"
786+
],
787+
"Inputs": [
788+
{
789+
"OperatorType": "Join",
790+
"Variant": "Join",
791+
"JoinColumnIndexes": "R:0",
792+
"JoinVars": {
793+
"m_col": 0
794+
},
795+
"TableName": "music_rerouted_ref, source_of_ref",
796+
"Inputs": [
797+
{
798+
"OperatorType": "Route",
799+
"Variant": "EqualUnique",
800+
"Keyspace": {
801+
"Name": "user",
802+
"Sharded": true
803+
},
804+
"FieldQuery": "select m.col from music as m where 1 != 1",
805+
"Query": "select m.col from music as m where m.user_id = 1 lock in share mode",
806+
"Table": "music",
807+
"Values": [
808+
"1"
809+
],
810+
"Vindex": "user_index"
811+
},
812+
{
813+
"OperatorType": "Route",
814+
"Variant": "Unsharded",
815+
"Keyspace": {
816+
"Name": "main",
817+
"Sharded": false
818+
},
819+
"FieldQuery": "select sr.id from source_of_ref as sr, rerouted_ref as rr where 1 != 1",
820+
"Query": "select sr.id from source_of_ref as sr, rerouted_ref as rr where sr.col = :m_col and sr.id = rr.id lock in share mode",
821+
"Table": "rerouted_ref, source_of_ref"
822+
}
823+
]
824+
},
825+
{
826+
"OperatorType": "Update",
827+
"Variant": "Unsharded",
828+
"Keyspace": {
829+
"Name": "main",
830+
"Sharded": false
831+
},
832+
"TargetTabletType": "PRIMARY",
833+
"Query": "update source_of_ref as sr set sr.tt = 5 where sr.id in ::dml_vals",
834+
"Table": "source_of_ref"
835+
}
836+
]
837+
},
838+
"TablesUsed": [
839+
"main.rerouted_ref",
840+
"main.source_of_ref",
841+
"user.music"
842+
]
843+
}
844+
},
845+
{
846+
"comment": "delete from reference table with join on sharded table",
847+
"query": "delete sr from main.source_of_ref as sr join main.rerouted_ref as rr on sr.id = rr.id inner join user.music as m on sr.col = m.col where m.user_id = 1",
848+
"plan": {
849+
"QueryType": "DELETE",
850+
"Original": "delete sr from main.source_of_ref as sr join main.rerouted_ref as rr on sr.id = rr.id inner join user.music as m on sr.col = m.col where m.user_id = 1",
851+
"Instructions": {
852+
"OperatorType": "DMLWithInput",
853+
"TargetTabletType": "PRIMARY",
854+
"Offset": [
855+
"0:[0]"
856+
],
857+
"Inputs": [
858+
{
859+
"OperatorType": "Join",
860+
"Variant": "Join",
861+
"JoinColumnIndexes": "R:0",
862+
"JoinVars": {
863+
"m_col": 0
864+
},
865+
"TableName": "music_rerouted_ref, source_of_ref",
866+
"Inputs": [
867+
{
868+
"OperatorType": "Route",
869+
"Variant": "EqualUnique",
870+
"Keyspace": {
871+
"Name": "user",
872+
"Sharded": true
873+
},
874+
"FieldQuery": "select m.col from music as m where 1 != 1",
875+
"Query": "select m.col from music as m where m.user_id = 1",
876+
"Table": "music",
877+
"Values": [
878+
"1"
879+
],
880+
"Vindex": "user_index"
881+
},
882+
{
883+
"OperatorType": "Route",
884+
"Variant": "Unsharded",
885+
"Keyspace": {
886+
"Name": "main",
887+
"Sharded": false
888+
},
889+
"FieldQuery": "select sr.id from source_of_ref as sr, rerouted_ref as rr where 1 != 1",
890+
"Query": "select sr.id from source_of_ref as sr, rerouted_ref as rr where sr.col = :m_col and sr.id = rr.id",
891+
"Table": "rerouted_ref, source_of_ref"
892+
}
893+
]
894+
},
895+
{
896+
"OperatorType": "Delete",
897+
"Variant": "Unsharded",
898+
"Keyspace": {
899+
"Name": "main",
900+
"Sharded": false
901+
},
902+
"TargetTabletType": "PRIMARY",
903+
"Query": "delete from source_of_ref as sr where sr.id in ::dml_vals",
904+
"Table": "source_of_ref"
905+
}
906+
]
907+
},
908+
"TablesUsed": [
909+
"main.rerouted_ref",
910+
"main.source_of_ref",
911+
"user.music"
912+
]
913+
}
774914
}
775915
]

‎go/vt/vtgate/planbuilder/testdata/unsupported_cases.json‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -347,7 +347,12 @@
347347
{
348348
"comment": "reference table delete with join",
349349
"query": "delete r from user u join ref_with_source r on u.col = r.col",
350-
"plan": "VT12001: unsupported: DELETE on reference table with join"
350+
"plan": "VT12001: unsupported: DML on reference table with join"
351+
},
352+
{
353+
"comment": "reference table update with join",
354+
"query": "update user u join ref_with_source r on u.col = r.col set r.col = 5",
355+
"plan": "VT12001: unsupported: DML on reference table with join"
351356
},
352357
{
353358
"comment": "group_concat unsupported when needs full evaluation at vtgate with more than 1 column",

‎go/vt/vtgate/semantics/analyzer.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -173,7 +173,7 @@ func (a *analyzer) newSemTable(
173173
Direct: a.binder.direct,
174174
ExprTypes: a.typer.m,
175175
Tables: a.tables.Tables,
176-
Targets: a.binder.targets,
176+
DMLTargets: a.binder.targets,
177177
NotSingleRouteErr: a.projErr,
178178
NotUnshardedErr: a.unshardedErr,
179179
Warning: a.warning,

0 commit comments

Comments
 (0)