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 6fb93c5778d..2188b303eda 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, *SetTransaction, 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) @@ -85,6 +90,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: nz.convertLiteralDedup(node, cursor) case *ComparisonExpr: diff --git a/go/vt/sqlparser/normalizer_test.go b/go/vt/sqlparser/normalizer_test.go index aa47f1e5634..ab953084d71 100644 --- a/go/vt/sqlparser/normalizer_test.go +++ b/go/vt/sqlparser/normalizer_test.go @@ -284,6 +284,14 @@ func TestNormalize(t *testing.T) { outbv: map[string]*querypb.BindVariable{ "bv1": sqltypes.ValueBindVariable(sqltypes.MakeTrusted(sqltypes.Datetime, []byte("2022-08-06 17:05:12"))), }, + }, { + // 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/abstract/derived.go b/go/vt/vtgate/planbuilder/abstract/derived.go index 508d576f37d..afbfc18303d 100644 --- a/go/vt/vtgate/planbuilder/abstract/derived.go +++ b/go/vt/vtgate/planbuilder/abstract/derived.go @@ -17,9 +17,7 @@ limitations under the License. package abstract import ( - vtrpcpb "vitess.io/vitess/go/vt/proto/vtrpc" "vitess.io/vitess/go/vt/sqlparser" - "vitess.io/vitess/go/vt/vterrors" "vitess.io/vitess/go/vt/vtgate/semantics" ) @@ -44,8 +42,11 @@ func (d *Derived) TableID() semantics.TableSet { func (d *Derived) PushPredicate(expr sqlparser.Expr, semTable *semantics.SemTable) (LogicalOperator, error) { tableInfo, err := 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/physical/operator_funcs.go b/go/vt/vtgate/planbuilder/physical/operator_funcs.go index c843372ae2a..e4ded531efc 100644 --- a/go/vt/vtgate/planbuilder/physical/operator_funcs.go +++ b/go/vt/vtgate/planbuilder/physical/operator_funcs.go @@ -125,8 +125,11 @@ func PushPredicate(ctx *plancontext.PlanningContext, expr sqlparser.Expr, op abs case *Derived: 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: op, + 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 d602e590390..78898d9bd99 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 80fdce4e4a2..7e908b958f4 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 5ce9efe3afe..ff6411b19e7 100644 --- a/go/vt/vtgate/semantics/early_rewriter_test.go +++ b/go/vt/vtgate/semantics/early_rewriter_test.go @@ -149,6 +149,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 2436f626e7d..7a981f542bd 100644 --- a/go/vt/vtgate/semantics/semantic_state.go +++ b/go/vt/vtgate/semantics/semantic_state.go @@ -116,8 +116,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 @@ -169,7 +169,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 }