bugfix: allow predicates without dependencies with derived tables to …

…be handled correctly (vitessio#11911)

* bugfix: allow predicates on top of derived tables to be handled without dependencies

Signed-off-by: Andres Taylor <andres@planetscale.com>

* stop normalizer from changing literals into bindvars

Signed-off-by: Andres Taylor <andres@planetscale.com>

Signed-off-by: Andres Taylor <andres@planetscale.com>
Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr>

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) {

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:

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) {

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
  }

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
  }

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)

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",

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.",

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) {

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
 }

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
 }