diff --git a/go/test/endtoend/vtgate/queries/subquery/subquery_test.go b/go/test/endtoend/vtgate/queries/subquery/subquery_test.go index d955c4b2d06..e11b6fd58de 100644 --- a/go/test/endtoend/vtgate/queries/subquery/subquery_test.go +++ b/go/test/endtoend/vtgate/queries/subquery/subquery_test.go @@ -66,6 +66,7 @@ func TestSubqueriesExists(t *testing.T) { mcmp.Exec("insert into t1(id1, id2) values (0,1),(1,2),(2,3),(3,4),(4,5),(5,6)") mcmp.AssertMatches(`SELECT id2 FROM t1 WHERE EXISTS (SELECT id1 FROM t1 WHERE id1 > 0) ORDER BY id2`, `[[INT64(1)] [INT64(2)] [INT64(3)] [INT64(4)] [INT64(5)] [INT64(6)]]`) + mcmp.AssertMatches(`select * from (select 1) as tmp where exists(select 1 from t1 where id1 = 1)`, `[[INT32(1)]]`) } func TestQueryAndSubQWithLimit(t *testing.T) { diff --git a/go/vt/sqlparser/normalizer.go b/go/vt/sqlparser/normalizer.go index 69f440defdc..b827cf853c7 100644 --- a/go/vt/sqlparser/normalizer.go +++ b/go/vt/sqlparser/normalizer.go @@ -42,10 +42,11 @@ func Normalize(stmt Statement, reserved *ReservedVars, bindVars map[string]*quer } type normalizer struct { - bindVars map[string]*querypb.BindVariable - reserved *ReservedVars - vals map[string]string - err error + bindVars map[string]*querypb.BindVariable + reserved *ReservedVars + vals map[string]string + err error + inDerived bool } func newNormalizer(reserved *ReservedVars, bindVars map[string]*querypb.BindVariable) *normalizer { @@ -65,8 +66,12 @@ func (nz *normalizer) WalkStatement(cursor *Cursor) bool { case *Set, *Show, *Begin, *Commit, *Rollback, *Savepoint, DDLStatement, *SRollback, *Release, *OtherAdmin, *OtherRead: return false case *Select: + _, isDerived := cursor.Parent().(*DerivedTable) + var tmp bool + tmp, nz.inDerived = nz.inDerived, isDerived _ = Rewrite(node, nz.WalkSelect, nil) // Don't continue + nz.inDerived = tmp return false case *Literal: nz.convertLiteral(node, cursor) @@ -87,6 +92,19 @@ func (nz *normalizer) WalkStatement(cursor *Cursor) bool { // WalkSelect normalizes the AST in Select mode. func (nz *normalizer) WalkSelect(cursor *Cursor) bool { switch node := cursor.Node().(type) { + case *Select: + _, isDerived := cursor.Parent().(*DerivedTable) + if !isDerived { + return true + } + var tmp bool + tmp, nz.inDerived = nz.inDerived, isDerived + _ = Rewrite(node, nz.WalkSelect, nil) + // Don't continue + nz.inDerived = tmp + return false + case SelectExprs: + return !nz.inDerived case *Literal: parent := cursor.Parent() switch parent.(type) { diff --git a/go/vt/sqlparser/normalizer_test.go b/go/vt/sqlparser/normalizer_test.go index 0fee46b0344..2ca153f4ea1 100644 --- a/go/vt/sqlparser/normalizer_test.go +++ b/go/vt/sqlparser/normalizer_test.go @@ -320,7 +320,7 @@ func TestNormalize(t *testing.T) { "zipcode": sqltypes.ValueBindVariable(sqltypes.MakeTrusted(sqltypes.Int64, []byte("01001900"))), }, }, { - // Int leading with zero should also be normalized + // literals in limit and offset should not reuse bindvars in: `select * from t where id = 10 limit 10 offset 10`, outstmt: `select * from t where id = :id limit :bv1, :bv2`, outbv: map[string]*querypb.BindVariable{ @@ -328,6 +328,14 @@ func TestNormalize(t *testing.T) { "bv2": sqltypes.Int64BindVariable(10), "id": sqltypes.Int64BindVariable(10), }, + }, { + // we don't want to replace literals on the select expressions of a derived table + // these expressions can be referenced from the outside, + // and changing them to bindvars can change the meaning of the query + // example of problematic query: select tmp.`1` from (select 1) as tmp + in: `select * from (select 12) as t`, + outstmt: `select * from (select 12 from dual) as t`, + outbv: map[string]*querypb.BindVariable{}, }} for _, tc := range testcases { t.Run(tc.in, func(t *testing.T) { diff --git a/go/vt/vtgate/planbuilder/operators/derived.go b/go/vt/vtgate/planbuilder/operators/derived.go index 21c405611fc..39f3bb79432 100644 --- a/go/vt/vtgate/planbuilder/operators/derived.go +++ b/go/vt/vtgate/planbuilder/operators/derived.go @@ -116,8 +116,11 @@ func (d *Derived) AddPredicate(ctx *plancontext.PlanningContext, expr sqlparser. } tableInfo, err := ctx.SemTable.TableInfoForExpr(expr) if err != nil { - if err == semantics.ErrMultipleTables { - return nil, semantics.ProjError{Inner: vterrors.Errorf(vtrpcpb.Code_UNIMPLEMENTED, "unsupported: unable to split predicates to derived table: %s", sqlparser.String(expr))} + if err == semantics.ErrNotSingleTable { + return &Filter{ + Source: d, + Predicates: []sqlparser.Expr{expr}, + }, nil } return nil, err } diff --git a/go/vt/vtgate/planbuilder/projection_pushing.go b/go/vt/vtgate/planbuilder/projection_pushing.go index 31ad4d90f82..b67ab7164c9 100644 --- a/go/vt/vtgate/planbuilder/projection_pushing.go +++ b/go/vt/vtgate/planbuilder/projection_pushing.go @@ -322,7 +322,7 @@ func addExpressionToRoute(ctx *plancontext.PlanningContext, rb *routeGen4, expr func rewriteProjectionOfDerivedTable(expr *sqlparser.AliasedExpr, semTable *semantics.SemTable) error { ti, err := semTable.TableInfoForExpr(expr.Expr) - if err != nil && err != semantics.ErrMultipleTables { + if err != nil && err != semantics.ErrNotSingleTable { return err } _, isDerivedTable := ti.(*semantics.DerivedTable) diff --git a/go/vt/vtgate/planbuilder/testdata/dml_cases.json b/go/vt/vtgate/planbuilder/testdata/dml_cases.json index ca33db80c94..8d4886e8c39 100644 --- a/go/vt/vtgate/planbuilder/testdata/dml_cases.json +++ b/go/vt/vtgate/planbuilder/testdata/dml_cases.json @@ -3827,7 +3827,26 @@ "main.user_privacy_consents" ] }, - "gen4-plan": "unsupported: unable to split predicates to derived table: not :__sq_has_values1" + "gen4-plan": { + "QueryType": "INSERT", + "Original": "INSERT INTO main.user_privacy_consents (user_id, accepted_at) SELECT user_id, accepted_at FROM (SELECT 1 as user_id, 1629194864 as accepted_at) AS tmp WHERE NOT EXISTS (SELECT user_id FROM main.user_privacy_consents WHERE user_id = 1)", + "Instructions": { + "OperatorType": "Insert", + "Variant": "Unsharded", + "Keyspace": { + "Name": "main", + "Sharded": false + }, + "TargetTabletType": "PRIMARY", + "MultiShardAutocommit": false, + "Query": "insert into user_privacy_consents(user_id, accepted_at) select user_id, accepted_at from (select 1 as user_id, 1629194864 as accepted_at from dual) as tmp where not exists (select 1 from user_privacy_consents where user_id = 1 limit 1) for update", + "TableName": "user_privacy_consents" + }, + "TablesUsed": [ + "main.dual", + "main.user_privacy_consents" + ] + } }, { "comment": "Delete on backfilling unique lookup vindex should be a scatter", diff --git a/go/vt/vtgate/planbuilder/testdata/unsupported_cases.json b/go/vt/vtgate/planbuilder/testdata/unsupported_cases.json index 421d6900bbb..075f3545e9f 100644 --- a/go/vt/vtgate/planbuilder/testdata/unsupported_cases.json +++ b/go/vt/vtgate/planbuilder/testdata/unsupported_cases.json @@ -400,8 +400,7 @@ { "comment": "outer and inner subquery route reference the same \"uu.id\" name\n# but they refer to different things. The first reference is to the outermost query,\n# and the second reference is to the innermost 'from' subquery.\n# This query will never work as the inner derived table is only selecting one of the column", "query": "select id2 from user uu where id in (select id from user where id = uu.id and user.col in (select col from (select id from user_extra where user_id = 5) uu where uu.user_id = uu.id))", - "v3-plan": "unsupported: cross-shard correlated subquery", - "gen4-plan": "unsupported: unable to split predicates to derived table: uu.user_id = uu.id" + "plan": "unsupported: cross-shard correlated subquery" }, { "comment": "outer and inner subquery route reference the same \"uu.id\" name\n# but they refer to different things. The first reference is to the outermost query,\n# and the second reference is to the innermost 'from' subquery.\n# changed to project all the columns from the derived tables.", diff --git a/go/vt/vtgate/semantics/early_rewriter_test.go b/go/vt/vtgate/semantics/early_rewriter_test.go index f851f3ce1f4..9fec6e2e949 100644 --- a/go/vt/vtgate/semantics/early_rewriter_test.go +++ b/go/vt/vtgate/semantics/early_rewriter_test.go @@ -161,6 +161,9 @@ func TestExpandStar(t *testing.T) { }, { sql: "select 1 from t1 join t5 using (b) having b = 12", expSQL: "select 1 from t1 join t5 where t1.b = t5.b having t1.b = 12", + }, { + sql: "select * from (select 12) as t", + expSQL: "select t.`12` from (select 12 from dual) as t", }} for _, tcase := range tcases { t.Run(tcase.sql, func(t *testing.T) { diff --git a/go/vt/vtgate/semantics/semantic_state.go b/go/vt/vtgate/semantics/semantic_state.go index 1dec125bfe2..9b01191d8f5 100644 --- a/go/vt/vtgate/semantics/semantic_state.go +++ b/go/vt/vtgate/semantics/semantic_state.go @@ -121,8 +121,8 @@ type ( ) var ( - // ErrMultipleTables refers to an error happening when something should be used only for single tables - ErrMultipleTables = vterrors.Errorf(vtrpcpb.Code_INTERNAL, "[BUG] should only be used for single tables") + // ErrNotSingleTable refers to an error happening when something should be used only for single tables + ErrNotSingleTable = vterrors.Errorf(vtrpcpb.Code_INTERNAL, "[BUG] should only be used for single tables") ) // CopyDependencies copies the dependencies from one expression into the other @@ -174,7 +174,7 @@ func (st *SemTable) ReplaceTableSetFor(id TableSet, t *sqlparser.AliasedTableExp func (st *SemTable) TableInfoFor(id TableSet) (TableInfo, error) { offset := id.TableOffset() if offset < 0 { - return nil, ErrMultipleTables + return nil, ErrNotSingleTable } return st.Tables[offset], nil } diff --git a/go/vt/vtgate/semantics/table_collector.go b/go/vt/vtgate/semantics/table_collector.go index dc6d1369c80..aa8b4eebf99 100644 --- a/go/vt/vtgate/semantics/table_collector.go +++ b/go/vt/vtgate/semantics/table_collector.go @@ -138,7 +138,7 @@ func (tc *tableCollector) tableSetFor(t *sqlparser.AliasedTableExpr) TableSet { func (tc *tableCollector) tableInfoFor(id TableSet) (TableInfo, error) { offset := id.TableOffset() if offset < 0 { - return nil, ErrMultipleTables + return nil, ErrNotSingleTable } return tc.Tables[offset], nil }