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 <[email protected]>

* stop normalizer from changing literals into bindvars

Signed-off-by: Andres Taylor <[email protected]>

Signed-off-by: Andres Taylor <[email protected]>

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

go/vt/sqlparser/normalizer_test.go

@@ -320,14 +320,22 @@ 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{
 			"bv1": sqltypes.Int64BindVariable(10),
 			"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) {

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
 	}

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

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

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
 }

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
 }