vtgate: validate the declared column list length of CTEs and derived tables

go/vt/vtgate/planbuilder/testdata/select_cases.json

@@ -357,6 +357,28 @@
       ]
     }
   },
+  {
+    "comment": "qualified star after join using keeps the join column",
+    "query": "select b.* from authoritative a join authoritative b using (user_id)",
+    "plan": {
+      "Type": "Scatter",
+      "QueryType": "SELECT",
+      "Original": "select b.* from authoritative a join authoritative b using (user_id)",
+      "Instructions": {
+        "OperatorType": "Route",
+        "Variant": "Scatter",
+        "Keyspace": {
+          "Name": "user",
+          "Sharded": true
+        },
+        "FieldQuery": "select b.user_id, b.col1, b.col2 from authoritative as a, authoritative as b where 1 != 1",
+        "Query": "select b.user_id, b.col1, b.col2 from authoritative as a, authoritative as b where a.user_id = b.user_id"
+      },
+      "TablesUsed": [
+        "user.authoritative"
+      ]
+    }
+  },
   {
     "comment": "select * from authoritative table",
     "query": "select * from authoritative",

go/vt/vtgate/semantics/analyzer_test.go

@@ -248,6 +248,141 @@ func TestRecursiveCTEChecking(t *testing.T) {
 	}
 }
 
+func TestColumnListLengthChecking(t *testing.T) {
+	const mismatch = "VT03033: In definition of view, derived table or common table expression, SELECT list and column names list have different column counts"
+	type testCase struct {
+		name, query, err string
+	}
+	queries := []testCase{{
+		// MySQL reports these three as unknown columns (1054): an unpairable
+		// declared list leaves the recursive reference on the seed select
+		// names, so the term's declared-name reference never resolves. The
+		// unresolved column parks as a sharded error, which strict analysis
+		// only surfaces if no hard error follows, so the count check answers
+		// here; unsharded pass-through still gets MySQL's own error
+		name:  "recursive cte with a declared column list shorter than the seed select list",
+		query: "with recursive x(a) as (select 1, 2 union select a + 1, 2 from x where a < 10) select a from x",
+		err:   mismatch,
+	}, {
+		name:  "recursive cte with a declared column list longer than the seed select list",
+		query: "with recursive x(a, b, c) as (select 1, 2 union select a + 1, b from x where a < 10) select a from x",
+		err:   mismatch,
+	}, {
+		name:  "recursive cte with a declared column list longer than an expanded star seed",
+		query: "with recursive x(a, b) as (select * from t1 union select id + 1 from x where id < 10) select a from x",
+		err:   mismatch,
+	}, {
+		name:  "recursive cte term resolves via seed names under a mismatched column list",
+		query: "with recursive x(a) as (select 1 as b, 2 as c union select b + 1, c from x where b < 3) select a from x",
+		err:   mismatch,
+	}, {
+		name:  "outer query cannot bypass the count check with a seed name",
+		query: "with recursive x(a) as (select 1 as b, 2 as c union select b + 1, c from x where b < 3) select b from x",
+		err:   mismatch,
+	}, {
+		name:  "matched column list hides seed names from the term",
+		query: "with recursive x(a, b) as (select 1 as s, 2 as t union select s + 1, t from x where s < 3) select a from x",
+		err:   "column 's' not found",
+	}, {
+		name:  "matched column list hides seed names from the outer query",
+		query: "with recursive x(a, b) as (select 1 as s, 2 as t union select a + 1, b from x where a < 3) select s from x",
+		err:   "column 's' not found in table 'x'",
+	}, {
+		name:  "recursive keyword without a self-reference",
+		query: "with recursive x(a) as (select 1, 2) select a from x",
+		err:   mismatch,
+	}, {
+		name:  "plain cte with a declared column list shorter than the select list",
+		query: "with x(a) as (select 1, 2) select a from x",
+		err:   mismatch,
+	}, {
+		name:  "plain cte with a declared column list longer than the select list",
+		query: "with x(a, b, c) as (select 1, 2) select a from x",
+		err:   mismatch,
+	}, {
+		name:  "plain cte defined by a union",
+		query: "with x(a) as (select 1, 2 union select 3, 4) select a from x",
+		err:   mismatch,
+	}, {
+		name:  "derived table with a declared column list shorter than the select list",
+		query: "select a from (select 1, 2) as x(a)",
+		err:   mismatch,
+	}, {
+		name:  "derived table with a declared column list longer than the select list",
+		query: "select a from (select 1) as x(a, b)",
+		err:   mismatch,
+	}, {
+		name:  "derived table defined by a union",
+		query: "select a from (select 1, 2 union select 3, 4) as x(a)",
+		err:   mismatch,
+	}, {
+		name:  "derived table with a declared column list shorter than an expanded star",
+		query: "select a from (select * from t2) as x(a)",
+		err:   mismatch,
+	}, {
+		name:  "unexpanded star with more select expressions than declared columns",
+		query: "select a from (select t.*, 1 from t) as x(a)",
+		err:   mismatch,
+	}, {
+		name:  "unexpanded star alone cannot be validated",
+		query: "select a from (select * from t) as x(a)",
+	}, {
+		name:  "derived table with a matching declared column list",
+		query: "select a from (select 1, 2) as x(a, b)",
+	}, {
+		name:  "derived table with a declared column list matching an expanded star",
+		query: "select a from (select * from t2) as x(a, b, c)",
+	}, {
+		name:  "plain cte with a matching declared column list",
+		query: "with x(a, b) as (select uid, name from t2) select a from x",
+	}, {
+		name:  "recursive cte with a matching declared column list",
+		query: "with recursive x(a, b) as (select 1, 2 union select a + 1, b from x where a < 5) select a from x",
+	}, {
+		// MySQL does not validate definitions that are never referenced
+		name:  "unused plain cte with a mismatched column list",
+		query: "with x(a) as (select 1, 2) select 1",
+	}, {
+		name:  "unused recursive cte with a mismatched column list",
+		query: "with recursive x(a) as (select 1, 2) select 1",
+	}, {
+		// a use inside a definition only counts if that definition is used
+		name:  "mismatched cte referenced only by an unused cte",
+		query: "with recursive x(a) as (select 1, 2), y as (select 1 from x) select 1",
+	}, {
+		name:  "mismatched cte referenced through two unused definitions",
+		query: "with recursive x(a) as (select 1, 2), y as (select 1 from x), z as (select 1 from y) select 1",
+	}, {
+		name:  "mismatched cte referenced through a used cte",
+		query: "with recursive x(a) as (select 1, 2), y as (select 1 from x) select * from y",
+		err:   mismatch,
+	}, {
+		name:  "mismatched cte referenced through a two-level used chain",
+		query: "with recursive x(a) as (select 1, 2), y as (select 1 from x), z as (select 1 from y) select * from z",
+		err:   mismatch,
+	}, {
+		name:  "derived table with a declared column list matching a qualified star after join using",
+		query: "select a from (select r.* from authoritative l join authoritative r using (col1)) x(a, b, c)",
+	}, {
+		name:  "derived table with a declared column list shorter than a qualified star after join using",
+		query: "select a from (select r.* from authoritative l join authoritative r using (col1)) x(a, b)",
+		err:   mismatch,
+	}}
+	for _, tc := range queries {
+		t.Run(tc.name, func(t *testing.T) {
+			parse, err := sqlparser.NewTestParser().Parse(tc.query)
+			require.NoError(t, err)
+
+			_, err = AnalyzeStrict(parse, "user", fakeSchemaInfo())
+			if tc.err == "" {
+				require.NoError(t, err)
+				return
+			}
+			require.EqualError(t, err, tc.err)
+		})
+	}
+}
+
 func TestBindingMultiAliasedTablePositive(t *testing.T) {
 	type testCase struct {
 		query          string
@@ -1481,18 +1616,38 @@ var ks3 = &vindexes.Keyspace{
 // create table t(<no column info>)
 // create table t1(id bigint)
 // create table t2(uid bigint, name varchar(255))
+// create table authoritative(col1 bigint, col2 bigint, col3 bigint)
 func fakeSchemaInfo() *FakeSI {
 	si := &FakeSI{
 		Tables: map[string]*vindexes.BaseTable{
-			"t":  tableT(),
-			"t1": tableT1(),
-			"t2": tableT2(),
-			"t3": tableT3(),
+			"t":             tableT(),
+			"t1":            tableT1(),
+			"t2":            tableT2(),
+			"t3":            tableT3(),
+			"authoritative": tableAuthoritative(),
 		},
 	}
 	return si
 }
 
+func tableAuthoritative() *vindexes.BaseTable {
+	return &vindexes.BaseTable{
+		Name: sqlparser.NewIdentifierCS("authoritative"),
+		Columns: []vindexes.Column{{
+			Name: sqlparser.NewIdentifierCI("col1"),
+			Type: querypb.Type_INT64,
+		}, {
+			Name: sqlparser.NewIdentifierCI("col2"),
+			Type: querypb.Type_INT64,
+		}, {
+			Name: sqlparser.NewIdentifierCI("col3"),
+			Type: querypb.Type_INT64,
+		}},
+		ColumnListAuthoritative: true,
+		Keyspace:                unsharded,
+	}
+}
+
 func tableT() *vindexes.BaseTable {
 	return &vindexes.BaseTable{
 		Name:     sqlparser.NewIdentifierCS("t"),

go/vt/vtgate/semantics/cte_table.go

@@ -94,18 +94,21 @@ func (cte *CTETable) canShortCut() shortCut {
 func (cte *CTETable) getColumns(bool) []ColumnInfo {
 	selExprs := cte.Query.GetColumns()
 	cols := make([]ColumnInfo, 0, len(selExprs))
+	// a declared column list renames the columns only when it pairs with the
+	// select list; on a length mismatch MySQL resolves the recursive reference
+	// against the select list names, and rejects the mismatch where the CTE
+	// is used
+	useDeclared := len(cte.Columns) == len(selExprs)
 	for i, selExpr := range selExprs {
 		ae, isAe := selExpr.(*sqlparser.AliasedExpr)
 		if !isAe {
 			panic(vterrors.VT12001("should not be called"))
 		}
-		if len(cte.Columns) == 0 {
-			cols = append(cols, ColumnInfo{Name: ae.ColumnName()})
+		if useDeclared {
+			cols = append(cols, ColumnInfo{Name: cte.Columns[i].String()})
 			continue
 		}
-
-		// We have column aliases defined on the CTE
-		cols = append(cols, ColumnInfo{Name: cte.Columns[i].String()})
+		cols = append(cols, ColumnInfo{Name: ae.ColumnName()})
 	}
 	return cols
 }

go/vt/vtgate/semantics/early_rewriter.go

@@ -1075,6 +1075,11 @@ func (r *earlyRewriter) expandTableColumns(
 		org:             org,
 		expandedColumns: map[sqlparser.TableName][]*sqlparser.ColName{},
 	}
+	if !starExpr.TableName.IsEmpty() {
+		// USING coalescing applies to an unqualified * only: a qualified star
+		// returns every column of its table, join columns included
+		state.joinUsing = nil
+	}
 
 	for _, tbl := range tables {
 		if !starExpr.TableName.IsEmpty() && !tbl.matches(starExpr.TableName) {

go/vt/vtgate/semantics/early_rewriter_test.go

@@ -174,6 +174,20 @@ func TestExpandStar(t *testing.T) {
 	}, {
 		sql:    "select 1 from t1 join t5 using (b) where b = 12",
 		expSQL: "select 1 from t1 join t5 on t1.b = t5.b where t1.b = 12",
+	}, {
+		// USING coalescing applies to an unqualified * only: a qualified star
+		// returns every column of its table, join columns included
+		sql:    "select t2.* from t2 join t4 using (c1)",
+		expSQL: "select t2.c1, t2.c2 from t2 join t4 on t2.c1 = t4.c1",
+	}, {
+		sql:    "select t4.* from t2 join t4 using (c1)",
+		expSQL: "select t4.c1, t4.c4 from t2 join t4 on t2.c1 = t4.c1",
+	}, {
+		sql:    "select t2.*, t4.* from t2 join t4 using (c1)",
+		expSQL: "select t2.c1, t2.c2, t4.c1, t4.c4 from t2 join t4 on t2.c1 = t4.c1",
+	}, {
+		sql:    "select t1.*, t5.* from t1 join t5 using (b)",
+		expSQL: "select t1.a, t1.b, t1.c, t5.a, t5.b from t1 join t5 on t1.b = t5.b",
 	}, {
 		sql:    "select * from (select 12) as t",
 		expSQL: "select `12` from (select 12 from dual) as t",

go/vt/vtgate/semantics/table_collector.go

@@ -47,6 +47,13 @@ type (
 		// cte is a map of CTE definitions that are used in the query
 		cte map[string]*CTE
 
+		// deferredMismatch holds column-list violations found while analyzing
+		// a definition's body, keyed by the enclosing definition. MySQL only
+		// validates definitions the executed statement uses, so a violation
+		// surfaces when a use of the recording definition resolves, and stays
+		// silent while it remains unused.
+		deferredMismatch map[*CTE]error
+
 		// lastInsertIdWithArgument is used to signal to later stages that we
 		// need to do special handling of the engine primitive
 		lastInsertIdWithArgument bool
@@ -55,10 +62,11 @@ type (
 
 func newEarlyTableCollector(si SchemaInformation, currentDb string) *earlyTableCollector {
 	return &earlyTableCollector{
-		si:        si,
-		currentDb: currentDb,
-		done:      map[*sqlparser.AliasedTableExpr]TableInfo{},
-		cte:       map[string]*CTE{},
+		si:               si,
+		currentDb:        currentDb,
+		done:             map[*sqlparser.AliasedTableExpr]TableInfo{},
+		cte:              map[string]*CTE{},
+		deferredMismatch: map[*CTE]error{},
 	}
 }
 
@@ -400,6 +408,23 @@ func (etc *earlyTableCollector) buildRecursiveCTE(node *sqlparser.AliasedTableEx
 			return cteTable, nil
 		}
 	}
+	// MySQL validates a declared column list only where the CTE is used from
+	// the executed statement: the recursive reference inside the definition
+	// resolves against the select list names instead when the declared list
+	// cannot be paired with it, and a use inside another definition only
+	// counts if that definition is itself used, so the violation is recorded
+	// on the enclosing definition and surfaces when (and if) a use reaches it
+	err := checkColumnListLength(cteDef.Columns, cteDef.Query)
+	if err == nil {
+		err = etc.deferredMismatch[cteDef]
+	}
+	if err != nil {
+		enclosing := etc.enclosingCTE(sc)
+		if enclosing == nil {
+			return nil, err
+		}
+		etc.deferredMismatch[enclosing] = err
+	}
 	return &RealTable{
 		tableName:    node.TableNameString(),
 		ASTNode:      node,
@@ -408,6 +433,16 @@ func (etc *earlyTableCollector) buildRecursiveCTE(node *sqlparser.AliasedTableEx
 	}, nil
 }
 
+// enclosingCTE returns the definition whose body is being analyzed at the
+// current scope, or nil when the reference comes from the executed statement.
+func (etc *earlyTableCollector) enclosingCTE(sc *scoper) *CTE {
+	if sc == nil || len(sc.commonTableExprScopes) == 0 {
+		return nil
+	}
+	def := sc.commonTableExprScopes[len(sc.commonTableExprScopes)-1]
+	return etc.cte[def.ID.String()]
+}
+
 func checkValidRecursiveCTE(cteDef *CTE) error {
 	if cteDef.IDForRecurse != nil {
 		return vterrors.VT09029(cteDef.Name)
@@ -435,6 +470,9 @@ func checkValidRecursiveCTE(cteDef *CTE) error {
 }
 
 func (tc *tableCollector) handleDerivedTable(node *sqlparser.AliasedTableExpr, t *sqlparser.DerivedTable) error {
+	if err := checkColumnListLength(node.Columns, t.Select); err != nil {
+		return err
+	}
 	switch sel := t.Select.(type) {
 	case *sqlparser.Select:
 		return tc.addSelectDerivedTable(sel, node, node.Columns, node.As)
@@ -445,6 +483,32 @@ func (tc *tableCollector) handleDerivedTable(node *sqlparser.AliasedTableExpr, t
 	}
 }
 
+// checkColumnListLength validates a declared column list against the select
+// list that defines the table, as MySQL does for derived tables and CTEs.
+func checkColumnListLength(columns sqlparser.Columns, stmt sqlparser.TableStatement) error {
+	if len(columns) == 0 {
+		return nil
+	}
+	firstSelect, err := sqlparser.GetFirstSelect(stmt)
+	if err != nil {
+		return err
+	}
+	selectColumns := firstSelect.GetColumns()
+	if containsStar(selectColumns) {
+		// every star expands to at least one column, so a column list with
+		// fewer names than select expressions is guaranteed not to match;
+		// beyond that we cannot validate against an unexpanded star
+		if len(columns) < len(selectColumns) {
+			return vterrors.VT03033()
+		}
+		return nil
+	}
+	if len(columns) != len(selectColumns) {
+		return vterrors.VT03033()
+	}
+	return nil
+}
+
 func (tc *tableCollector) addSelectDerivedTable(
 	sel *sqlparser.Select,
 	tableExpr *sqlparser.AliasedTableExpr,