Repository: vitessio/vitess
Pull request #8846 (2021-09-20 06:45:54)

Gen4 fix plantests
## Description

This pull request minimizes the use of `Gen4NotSupportedF` errors throughout the codebase. This error is now used only when something is supported by v3 and not by Gen4. Additionally, `SELECT INTO` now has a better error mechanism.

## Related Issue(s)

- #7280 

## Checklist
- [x] Tests were added or are not required
- [x] Documentation was added or is not required



Patch:
diff --git a/go/vt/sqlparser/ast_clone.go b/go/vt/sqlparser/ast_clone.go
index 043d661a81..02db72715f 100644
--- a/go/vt/sqlparser/ast_clone.go
+++ b/go/vt/sqlparser/ast_clone.go
@@ -225,6 +225,8 @@ func CloneSQLNode(in SQLNode) SQLNode {
 		return CloneRefOfRevertMigration(in)
 	case *Rollback:
 		return CloneRefOfRollback(in)
+	case RootNode:
+		return CloneRootNode(in)
 	case *SRollback:
 		return CloneRefOfSRollback(in)
 	case *Savepoint:
@@ -1308,6 +1310,11 @@ func CloneRefOfRollback(n *Rollback) *Rollback {
 	return &out
 }
 
+// CloneRootNode creates a deep clone of the input.
+func CloneRootNode(n RootNode) RootNode {
+	return *CloneRefOfRootNode(&n)
+}
+
 // CloneRefOfSRollback creates a deep clone of the input.
 func CloneRefOfSRollback(n *SRollback) *SRollback {
 	if n == nil {
@@ -2346,6 +2353,16 @@ func CloneSliceOfRefOfRenameTablePair(n []*RenameTablePair) []*RenameTablePair {
 	return res
 }
 
+// CloneRefOfRootNode creates a deep clone of the input.
+func CloneRefOfRootNode(n *RootNode) *RootNode {
+	if n == nil {
+		return nil
+	}
+	out := *n
+	out.SQLNode = CloneSQLNode(n.SQLNode)
+	return &out
+}
+
 // CloneRefOfBool creates a deep clone of the input.
 func CloneRefOfBool(n *bool) *bool {
 	if n == nil {
diff --git a/go/vt/sqlparser/ast_equals.go b/go/vt/sqlparser/ast_equals.go
index 521150cb4f..b3a006c4b5 100644
--- a/go/vt/sqlparser/ast_equals.go
+++ b/go/vt/sqlparser/ast_equals.go
@@ -632,6 +632,12 @@ func EqualsSQLNode(inA, inB SQLNode) bool {
 			return false
 		}
 		return EqualsRefOfRollback(a, b)
+	case RootNode:
+		b, ok := inB.(RootNode)
+		if !ok {
+			return false
+		}
+		return EqualsRootNode(a, b)
 	case *SRollback:
 		b, ok := inB.(*SRollback)
 		if !ok {
@@ -2110,6 +2116,11 @@ func EqualsRefOfRollback(a, b *Rollback) bool {
 	return true
 }
 
+// EqualsRootNode does deep equals between the two objects.
+func EqualsRootNode(a, b RootNode) bool {
+	return EqualsSQLNode(a.SQLNode, b.SQLNode)
+}
+
 // EqualsRefOfSRollback does deep equals between the two objects.
 func EqualsRefOfSRollback(a, b *SRollback) bool {
 	if a == b {
@@ -3863,6 +3874,17 @@ func EqualsSliceOfRefOfRenameTablePair(a, b []*RenameTablePair) bool {
 	return true
 }
 
+// EqualsRefOfRootNode does deep equals between the two objects.
+func EqualsRefOfRootNode(a, b *RootNode) bool {
+	if a == b {
+		return true
+	}
+	if a == nil || b == nil {
+		return false
+	}
+	return EqualsSQLNode(a.SQLNode, b.SQLNode)
+}
+
 // EqualsRefOfBool does deep equals between the two objects.
 func EqualsRefOfBool(a, b *bool) bool {
 	if a == b {
diff --git a/go/vt/sqlparser/ast_rewrite.go b/go/vt/sqlparser/ast_rewrite.go
index e1dd461cdc..20d2e005a7 100644
--- a/go/vt/sqlparser/ast_rewrite.go
+++ b/go/vt/sqlparser/ast_rewrite.go
@@ -224,6 +224,8 @@ func (a *application) rewriteSQLNode(parent SQLNode, node SQLNode, replacer repl
 		return a.rewriteRefOfRevertMigration(parent, node, replacer)
 	case *Rollback:
 		return a.rewriteRefOfRollback(parent, node, replacer)
+	case RootNode:
+		return a.rewriteRootNode(parent, node, replacer)
 	case *SRollback:
 		return a.rewriteRefOfSRollback(parent, node, replacer)
 	case *Savepoint:
@@ -3215,6 +3217,30 @@ func (a *application) rewriteRefOfRollback(parent SQLNode, node *Rollback, repla
 	}
 	return true
 }
+func (a *application) rewriteRootNode(parent SQLNode, node RootNode, replacer replacerFunc) bool {
+	if a.pre != nil {
+		a.cur.replacer = replacer
+		a.cur.parent = parent
+		a.cur.node = node
+		if !a.pre(&a.cur) {
+			return true
+		}
+	}
+	if !a.rewriteSQLNode(node, node.SQLNode, func(newNode, parent SQLNode) {
+		panic("[BUG] tried to replace 'SQLNode' on 'RootNode'")
+	}) {
+		return false
+	}
+	if a.post != nil {
+		a.cur.replacer = replacer
+		a.cur.parent = parent
+		a.cur.node = node
+		if !a.post(&a.cur) {
+			return false
+		}
+	}
+	return true
+}
 func (a *application) rewriteRefOfSRollback(parent SQLNode, node *SRollback, replacer replacerFunc) bool {
 	if node == nil {
 		return true
@@ -5281,6 +5307,33 @@ func (a *application) rewriteRefOfColIdent(parent SQLNode, node *ColIdent, repla
 	}
 	return true
 }
+func (a *application) rewriteRefOfRootNode(parent SQLNode, node *RootNode, replacer replacerFunc) bool {
+	if node == nil {
+		return true
+	}
+	if a.pre != nil {
+		a.cur.replacer = replacer
+		a.cur.parent = parent
+		a.cur.node = node
+		if !a.pre(&a.cur) {
+			return true
+		}
+	}
+	if !a.rewriteSQLNode(node, node.SQLNode, func(newNode, parent SQLNode) {
+		parent.(*RootNode).SQLNode = newNode.(SQLNode)
+	}) {
+		return false
+	}
+	if a.post != nil {
+		a.cur.replacer = replacer
+		a.cur.parent = parent
+		a.cur.node = node
+		if !a.post(&a.cur) {
+			return false
+		}
+	}
+	return true
+}
 func (a *application) rewriteRefOfTableIdent(parent SQLNode, node *TableIdent, replacer replacerFunc) bool {
 	if node == nil {
 		return true
diff --git a/go/vt/sqlparser/ast_visit.go b/go/vt/sqlparser/ast_visit.go
index 56f2029b8d..e8a9548809 100644
--- a/go/vt/sqlparser/ast_visit.go
+++ b/go/vt/sqlparser/ast_visit.go
@@ -224,6 +224,8 @@ func VisitSQLNode(in SQLNode, f Visit) error {
 		return VisitRefOfRevertMigration(in, f)
 	case *Rollback:
 		return VisitRefOfRollback(in, f)
+	case RootNode:
+		return VisitRootNode(in, f)
 	case *SRollback:
 		return VisitRefOfSRollback(in, f)
 	case *Savepoint:
@@ -1630,6 +1632,15 @@ func VisitRefOfRollback(in *Rollback, f Visit) error {
 	}
 	return nil
 }
+func VisitRootNode(in RootNode, f Visit) error {
+	if cont, err := f(in); err != nil || !cont {
+		return err
+	}
+	if err := VisitSQLNode(in.SQLNode, f); err != nil {
+		return err
+	}
+	return nil
+}
 func VisitRefOfSRollback(in *SRollback, f Visit) error {
 	if in == nil {
 		return nil
@@ -2751,6 +2762,18 @@ func VisitRefOfColIdent(in *ColIdent, f Visit) error {
 	}
 	return nil
 }
+func VisitRefOfRootNode(in *RootNode, f Visit) error {
+	if in == nil {
+		return nil
+	}
+	if cont, err := f(in); err != nil || !cont {
+		return err
+	}
+	if err := VisitSQLNode(in.SQLNode, f); err != nil {
+		return err
+	}
+	return nil
+}
 func VisitRefOfTableIdent(in *TableIdent, f Visit) error {
 	if in == nil {
 		return nil
diff --git a/go/vt/sqlparser/rewriter_api.go b/go/vt/sqlparser/rewriter_api.go
index f79e3432b6..6f4069fbd6 100644
--- a/go/vt/sqlparser/rewriter_api.go
+++ b/go/vt/sqlparser/rewriter_api.go
@@ -35,7 +35,7 @@ package sqlparser
 // i.e., fields of basic types (strings, []byte, etc.) are ignored.
 //
 func Rewrite(node SQLNode, pre, post ApplyFunc) (result SQLNode) {
-	parent := &struct{ SQLNode }{node}
+	parent := &RootNode{node}
 
 	// this is the root-replacer, used when the user replaces the root of the ast
 	replacer := func(newNode SQLNode, _ SQLNode) {
@@ -52,6 +52,11 @@ func Rewrite(node SQLNode, pre, post ApplyFunc) (result SQLNode) {
 	return parent.SQLNode
 }
 
+// RootNode is the root node of the AST. It is the first element of the tree.
+type RootNode struct {
+	SQLNode
+}
+
 // An ApplyFunc is invoked by Rewrite for each node n, even if n is nil,
 // before and/or after the node's children, using a Cursor describing
 // the current node and providing operations on it.
diff --git a/go/vt/vtgate/planbuilder/abstract/concatenate.go b/go/vt/vtgate/planbuilder/abstract/concatenate.go
index 149c12f81d..092be3dcec 100644
--- a/go/vt/vtgate/planbuilder/abstract/concatenate.go
+++ b/go/vt/vtgate/planbuilder/abstract/concatenate.go
@@ -17,7 +17,9 @@ 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"
 )
 
@@ -47,11 +49,22 @@ func (c *Concatenate) TableID() semantics.TableSet {
 }
 
 // PushPredicate implements the Operator interface
-func (c *Concatenate) PushPredicate(expr sqlparser.Expr, semTable *semantics.SemTable) error {
-	panic("implement me")
+func (c *Concatenate) PushPredicate(sqlparser.Expr, *semantics.SemTable) error {
+	return vterrors.Errorf(vtrpcpb.Code_UNIMPLEMENTED, "can't push predicates on concatenate")
 }
 
 // UnsolvedPredicates implements the Operator interface
-func (c *Concatenate) UnsolvedPredicates(semTable *semantics.SemTable) []sqlparser.Expr {
+func (c *Concatenate) UnsolvedPredicates(*semantics.SemTable) []sqlparser.Expr {
 	panic("implement me")
 }
+
+// CheckValid implements the Operator interface
+func (c *Concatenate) CheckValid() error {
+	for _, source := range c.Sources {
+		err := source.CheckValid()
+		if err != nil {
+			return err
+		}
+	}
+	return nil
+}
diff --git a/go/vt/vtgate/planbuilder/abstract/derived.go b/go/vt/vtgate/planbuilder/abstract/derived.go
index 01c28f9171..ba880d75bd 100644
--- a/go/vt/vtgate/planbuilder/abstract/derived.go
+++ b/go/vt/vtgate/planbuilder/abstract/derived.go
@@ -58,3 +58,8 @@ func (d *Derived) PushPredicate(expr sqlparser.Expr, semTable *semantics.SemTabl
 func (d *Derived) UnsolvedPredicates(semTable *semantics.SemTable) []sqlparser.Expr {
 	return d.Inner.UnsolvedPredicates(semTable)
 }
+
+// CheckValid implements the Operator interface
+func (d *Derived) CheckValid() error {
+	return d.Inner.CheckValid()
+}
diff --git a/go/vt/vtgate/planbuilder/abstract/distinct.go b/go/vt/vtgate/planbuilder/abstract/distinct.go
index 190f777916..622d972b6e 100644
--- a/go/vt/vtgate/planbuilder/abstract/distinct.go
+++ b/go/vt/vtgate/planbuilder/abstract/distinct.go
@@ -42,3 +42,8 @@ func (d *Distinct) PushPredicate(expr sqlparser.Expr, semTable *semantics.SemTab
 func (d *Distinct) UnsolvedPredicates(semTable *semantics.SemTable) []sqlparser.Expr {
 	return d.Source.UnsolvedPredicates(semTable)
 }
+
+// CheckValid implements the Operator interface
+func (d *Distinct) CheckValid() error {
+	return d.Source.CheckValid()
+}
diff --git a/go/vt/vtgate/planbuilder/abstract/join.go b/go/vt/vtgate/planbuilder/abstract/join.go
index 788b3dc93f..88bcccb9ec 100644
--- a/go/vt/vtgate/planbuilder/abstract/join.go
+++ b/go/vt/vtgate/planbuilder/abstract/join.go
@@ -17,7 +17,9 @@ 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"
 )
 
@@ -42,7 +44,7 @@ func (j *Join) PushPredicate(expr sqlparser.Expr, semTable *semantics.SemTable)
 		return nil
 	}
 
-	return semantics.Gen4NotSupportedF("still not sure what to do with this predicate")
+	return vterrors.Errorf(vtrpcpb.Code_INTERNAL, "Cannot push predicate: %s", sqlparser.String(expr))
 }
 
 // TableID implements the Operator interface
@@ -68,3 +70,13 @@ func (j *Join) UnsolvedPredicates(semTable *semantics.SemTable) []sqlparser.Expr
 	}
 	return result
 }
+
+// CheckValid implements the Operator interface
+func (j *Join) CheckValid() error {
+	err := j.LHS.CheckValid()
+	if err != nil {
+		return err
+	}
+
+	return j.RHS.CheckValid()
+}
diff --git a/go/vt/vtgate/planbuilder/abstract/left_join.go b/go/vt/vtgate/planbuilder/abstract/left_join.go
index 2b992010af..9a3174bf8b 100644
--- a/go/vt/vtgate/planbuilder/abstract/left_join.go
+++ b/go/vt/vtgate/planbuilder/abstract/left_join.go
@@ -17,7 +17,9 @@ 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"
 )
 
@@ -36,7 +38,7 @@ func (oj *LeftJoin) PushPredicate(expr sqlparser.Expr, semTable *semantics.SemTa
 		return oj.Left.PushPredicate(expr, semTable)
 	}
 
-	return semantics.Gen4NotSupportedF("cannot push predicates to the RHS of an outer join")
+	return vterrors.Errorf(vtrpcpb.Code_INTERNAL, "Cannot push predicate: %s", sqlparser.String(expr))
 }
 
 // TableID implements the Operator interface
@@ -62,3 +64,12 @@ func (oj *LeftJoin) UnsolvedPredicates(semTable *semantics.SemTable) []sqlparser
 	}
 	return result
 }
+
+// CheckValid implements the Operator interface
+func (oj *LeftJoin) CheckValid() error {
+	err := oj.Left.CheckValid()
+	if err != nil {
+		return err
+	}
+	return oj.Right.CheckValid()
+}
diff --git a/go/vt/vtgate/planbuilder/abstract/operator.go b/go/vt/vtgate/planbuilder/abstract/operator.go
index 15e788c1bc..11675a2646 100644
--- a/go/vt/vtgate/planbuilder/abstract/operator.go
+++ b/go/vt/vtgate/planbuilder/abstract/operator.go
@@ -45,6 +45,9 @@ type (
 		// UnsolvedPredicates returns any predicates that have dependencies on the given Operator and
 		// on the outside of it (a parent Select expression, any other table not used by Operator, etc).
 		UnsolvedPredicates(semTable *semantics.SemTable) []sqlparser.Expr
+
+		// CheckValid checks if we have a valid operator tree, and returns an error if something is wrong
+		CheckValid() error
 	}
 )
 
@@ -79,7 +82,7 @@ func getOperatorFromTableExpr(tableExpr sqlparser.TableExpr, semTable *semantics
 			}
 			return &Derived{Alias: tableExpr.As.String(), Inner: inner, Sel: tbl.Select}, nil
 		default:
-			return nil, semantics.Gen4NotSupportedF("%T", tbl)
+			return nil, vterrors.Errorf(vtrpcpb.Code_INTERNAL, "unable to use: %T", tbl)
 		}
 	case *sqlparser.JoinTableExpr:
 		switch tableExpr.Join {
@@ -119,12 +122,12 @@ func getOperatorFromTableExpr(tableExpr sqlparser.TableExpr, semTable *semantics
 			}
 			return op, nil
 		default:
-			return nil, semantics.Gen4NotSupportedF("%s joins", tableExpr.Join.ToString())
+			return nil, vterrors.Errorf(vtrpcpb.Code_UNIMPLEMENTED, "unsupported: %s", tableExpr.Join.ToString())
 		}
 	case *sqlparser.ParenTableExpr:
 		return crossJoin(tableExpr.Exprs, semTable)
 	default:
-		return nil, semantics.Gen4NotSupportedF("%T table type", tableExpr)
+		return nil, vterrors.Errorf(vtrpcpb.Code_INTERNAL, "unable to use: %T table type", tableExpr)
 	}
 }
 
diff --git a/go/vt/vtgate/planbuilder/abstract/querygraph.go b/go/vt/vtgate/planbuilder/abstract/querygraph.go
index fa0a786cc9..441cb3573c 100644
--- a/go/vt/vtgate/planbuilder/abstract/querygraph.go
+++ b/go/vt/vtgate/planbuilder/abstract/querygraph.go
@@ -158,3 +158,8 @@ func (qg *QueryGraph) UnsolvedPredicates(_ *semantics.SemTable) []sqlparser.Expr
 	}
 	return result
 }
+
+// CheckValid implements the Operator interface
+func (qg *QueryGraph) CheckValid() error {
+	return nil
+}
diff --git a/go/vt/vtgate/planbuilder/abstract/subquery.go b/go/vt/vtgate/planbuilder/abstract/subquery.go
index 277e9dcb9a..72fbffea8e 100644
--- a/go/vt/vtgate/planbuilder/abstract/subquery.go
+++ b/go/vt/vtgate/planbuilder/abstract/subquery.go
@@ -17,7 +17,9 @@ 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/engine"
 	"vitess.io/vitess/go/vt/vtgate/semantics"
 )
@@ -48,8 +50,8 @@ func (s *SubQuery) TableID() semantics.TableSet {
 }
 
 // PushPredicate implements the Operator interface
-func (s *SubQuery) PushPredicate(expr sqlparser.Expr, semTable *semantics.SemTable) error {
-	return semantics.Gen4NotSupportedF("pushing predicate on subquery")
+func (s *SubQuery) PushPredicate(sqlparser.Expr, *semantics.SemTable) error {
+	return vterrors.Errorf(vtrpcpb.Code_INTERNAL, "[BUG] should not try to push predicate on subquery")
 }
 
 // UnsolvedPredicates implements the Operator interface
@@ -73,3 +75,14 @@ func (s *SubQuery) UnsolvedPredicates(semTable *semantics.SemTable) []sqlparser.
 	}
 	return result
 }
+
+// CheckValid implements the Operator interface
+func (s *SubQuery) CheckValid() error {
+	for _, inner := range s.Inner {
+		err := inner.Inner.CheckValid()
+		if err != nil {
+			return err
+		}
+	}
+	return s.Outer.CheckValid()
+}
diff --git a/go/vt/vtgate/planbuilder/abstract/vindex.go b/go/vt/vtgate/planbuilder/abstract/vindex.go
index 47c3ac1b84..a454fec5e6 100644
--- a/go/vt/vtgate/planbuilder/abstract/vindex.go
+++ b/go/vt/vtgate/planbuilder/abstract/vindex.go
@@ -48,7 +48,7 @@ type (
 var _ Operator = (*Vindex)(nil)
 
 // TableID implements the Operator interface
-func (v Vindex) TableID() semantics.TableSet {
+func (v *Vindex) TableID() semantics.TableSet {
 	return v.Table.TableID
 }
 
@@ -98,6 +98,15 @@ func (v *Vindex) PushPredicate(expr sqlparser.Expr, semTable *semantics.SemTable
 }
 
 // UnsolvedPredicates implements the Operator interface
-func (v Vindex) UnsolvedPredicates(semTable *semantics.SemTable) []sqlparser.Expr {
-	panic("implement me")
+func (v *Vindex) UnsolvedPredicates(*semantics.SemTable) []sqlparser.Expr {
+	return nil
+}
+
+// CheckValid implements the Operator interface
+func (v *Vindex) CheckValid() error {
+	if len(v.Table.Predicates) == 0 {
+		return vterrors.Errorf(vtrpcpb.Code_UNIMPLEMENTED, "unsupported: where clause for vindex function must be of the form id = <val> (where clause missing)")
+	}
+
+	return nil
 }
diff --git a/go/vt/vtgate/planbuilder/gen4_planner.go b/go/vt/vtgate/planbuilder/gen4_planner.go
index bfc33890e0..28f61d6605 100644
--- a/go/vt/vtgate/planbuilder/gen4_planner.go
+++ b/go/vt/vtgate/planbuilder/gen4_planner.go
@@ -97,6 +97,10 @@ func newBuildSelectPlan(selStmt sqlparser.SelectStatement, reservedVars *sqlpars
 	if err != nil {
 		return nil, err
 	}
+	err = opTree.CheckValid()
+	if err != nil {
+		return nil, err
+	}
 
 	tree, err := optimizeQuery(ctx, opTree)
 	if err != nil {
diff --git a/go/vt/vtgate/planbuilder/horizon_planning.go b/go/vt/vtgate/planbuilder/horizon_planning.go
index 36f16be9b4..99e7d2abf7 100644
--- a/go/vt/vtgate/planbuilder/horizon_planning.go
+++ b/go/vt/vtgate/planbuilder/horizon_planning.go
@@ -39,6 +39,10 @@ type horizonPlanning struct {
 }
 
 func (hp *horizonPlanning) planHorizon(ctx *planningContext, plan logicalPlan) (logicalPlan, error) {
+	if err := checkUnsupportedConstructs(hp.sel); err != nil {
+		return nil, err
+	}
+
 	rb, isRoute := plan.(*route)
 	if !isRoute && ctx.semTable.ProjectionErr != nil {
 		return nil, ctx.semTable.ProjectionErr
@@ -59,10 +63,6 @@ func (hp *horizonPlanning) planHorizon(ctx *planningContext, plan logicalPlan) (
 
 	hp.qp = qp
 
-	if err := checkUnsupportedConstructs(hp.sel); err != nil {
-		return nil, err
-	}
-
 	needAggrOrHaving := hp.qp.NeedsAggregation() || hp.sel.Having != nil
 	canShortcut := isRoute && !needAggrOrHaving && len(hp.qp.OrderExprs) == 0
 
@@ -515,7 +515,7 @@ func planGroupByGen4(groupExpr abstract.GroupBy, plan logicalPlan, semTable *sem
 		}
 		return colAdded || colAddedRecursively, nil
 	default:
-		return false, semantics.Gen4NotSupportedF("group by on: %T", plan)
+		return false, vterrors.Errorf(vtrpcpb.Code_UNIMPLEMENTED, "unsupported: group by on: %T", plan)
 	}
 }
 
@@ -663,7 +663,7 @@ func wrapAndPushExpr(expr sqlparser.Expr, weightStrExpr sqlparser.Expr, plan log
 	}
 	_, ok := expr.(*sqlparser.ColName)
 	if !ok {
-		return 0, 0, false, semantics.Gen4NotSupportedF("group by/order by non-column expression")
+		return 0, 0, false, vterrors.Errorf(vtrpcpb.Code_UNIMPLEMENTED, "unsupported: in scatter query: complex order by expression: %s", sqlparser.String(expr))
 	}
 	qt := semTable.TypeFor(expr)
 	wsNeeded := true
@@ -980,14 +980,9 @@ func (hp *horizonPlanning) planHaving(ctx *planningContext, plan logicalPlan) er
 func pushHaving(expr sqlparser.Expr, plan logicalPlan, semTable *semantics.SemTable) error {
 	switch node := plan.(type) {
 	case *route:
-		sel, ok := node.Select.(*sqlparser.Select)
-		if !ok {
-			return vterrors.Errorf(vtrpcpb.Code_UNIMPLEMENTED, "unsupported: filtering on unexpected select statement: %T", node.Select)
-		}
+		sel := sqlparser.GetFirstSelect(node.Select)
 		sel.AddHaving(expr)
 		return nil
-	case *join:
-		return semantics.Gen4NotSupportedF("having on join")
 	case *pulloutSubquery:
 		return pushHaving(expr, node.underlying, semTable)
 	case *simpleProjection:
diff --git a/go/vt/vtgate/planbuilder/route_planning.go b/go/vt/vtgate/planbuilder/route_planning.go
index 43041e458e..14f2d78464 100644
--- a/go/vt/vtgate/planbuilder/route_planning.go
+++ b/go/vt/vtgate/planbuilder/route_planning.go
@@ -101,7 +101,7 @@ func optimizeQuery(ctx *planningContext, opTree abstract.Operator) (queryTree, e
 			source: qt,
 		}, nil
 	default:
-		return nil, semantics.Gen4NotSupportedF("optimizeQuery")
+		return nil, vterrors.Errorf(vtrpcpb.Code_INTERNAL, "invalid operator tree: %T", op)
 	}
 }
 
diff --git a/go/vt/vtgate/planbuilder/subquerytree.go b/go/vt/vtgate/planbuilder/subquerytree.go
index add920ed35..328c336c32 100644
--- a/go/vt/vtgate/planbuilder/subquerytree.go
+++ b/go/vt/vtgate/planbuilder/subquerytree.go
@@ -17,7 +17,9 @@ limitations under the License.
 package planbuilder
 
 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/engine"
 	"vitess.io/vitess/go/vt/vtgate/semantics"
 )
@@ -52,5 +54,5 @@ func (s *subqueryTree) clone() queryTree {
 }
 
 func (s *subqueryTree) pushOutputColumns([]*sqlparser.ColName, *semantics.SemTable) ([]int, error) {
-	return nil, semantics.Gen4NotSupportedF("pushing output columns on subquery tree")
+	return nil, vterrors.Errorf(vtrpcpb.Code_INTERNAL, "[BUG] should not try to push output columns on subquery")
 }
diff --git a/go/vt/vtgate/semantics/analyzer.go b/go/vt/vtgate/semantics/analyzer.go
index 7c20096ce2..fe8242acdb 100644
--- a/go/vt/vtgate/semantics/analyzer.go
+++ b/go/vt/vtgate/semantics/analyzer.go
@@ -266,6 +266,13 @@ func (a *analyzer) analyze(statement sqlparser.Statement) error {
 
 func checkForInvalidConstructs(cursor *sqlparser.Cursor) error {
 	switch node := cursor.Node().(type) {
+	case *sqlparser.Select:
+		if node.Into == nil {
+			return nil
+		}
+		if _, isRootNode := cursor.Parent().(*sqlparser.RootNode); !isRootNode {
+			return vterrors.NewErrorf(vtrpcpb.Code_INVALID_ARGUMENT, vterrors.CantUseOptionHere, "Incorrect usage/placement of 'INTO'")
+		}
 	case *sqlparser.JoinTableExpr:
 		if node.Condition != nil && node.Condition.Using != nil {
 			return vterrors.New(vtrpcpb.Code_UNIMPLEMENTED, "unsupported: join with USING(column_list) clause for complex queries")
@@ -273,22 +280,6 @@ func checkForInvalidConstructs(cursor *sqlparser.Cursor) error {
 		if node.Join == sqlparser.NaturalJoinType || node.Join == sqlparser.NaturalRightJoinType || node.Join == sqlparser.NaturalLeftJoinType {
 			return vterrors.New(vtrpcpb.Code_UNIMPLEMENTED, "unsupported: "+node.Join.ToString())
 		}
-	case *sqlparser.Subquery:
-		sel, ok := node.Select.(*sqlparser.Select)
-		if !ok {
-			return Gen4NotSupportedF("%T in subquery", node.Select)
-		}
-		if sel.Into != nil {
-			return vterrors.NewErrorf(vtrpcpb.Code_INVALID_ARGUMENT, vterrors.CantUseOptionHere, "Incorrect usage/placement of 'INTO'")
-		}
-	case *sqlparser.DerivedTable:
-		sel, ok := node.Select.(*sqlparser.Select)
-		if !ok {
-			return nil
-		}
-		if sel.Into != nil {
-			return vterrors.NewErrorf(vtrpcpb.Code_INVALID_ARGUMENT, vterrors.CantUseOptionHere, "Incorrect usage/placement of 'INTO'")
-		}
 	case *sqlparser.FuncExpr:
 		if sqlparser.IsLockingFunc(node) {
 			return vterrors.Errorf(vtrpcpb.Code_UNIMPLEMENTED, "%v allowed only with dual", sqlparser.String(node))
diff --git a/go/vt/vtgate/semantics/table_collector.go b/go/vt/vtgate/semantics/table_collector.go
index 737b25ee03..1db47ba1b6 100644
--- a/go/vt/vtgate/semantics/table_collector.go
+++ b/go/vt/vtgate/semantics/table_collector.go
@@ -17,7 +17,10 @@ limitations under the License.
 package semantics
 
 import (
+	"vitess.io/vitess/go/vt/key"
+	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/vindexes"
 )
 
@@ -77,7 +80,7 @@ func (tc *tableCollector) up(cursor *sqlparser.Cursor) error {
 			return scope.addTable(tableInfo)
 
 		default:
-			return Gen4NotSupportedF("union in derived table")
+			return Gen4NotSupportedF("%T in a derived table", sel)
 		}
 
 	case sqlparser.TableName:
@@ -88,10 +91,14 @@ func (tc *tableCollector) up(cursor *sqlparser.Cursor) error {
 			isInfSchema = true
 		} else {
 			var err error
-			tbl, vindex, _, _, _, err = tc.si.FindTableOrVindex(t)
+			var target key.Destination
+			tbl, vindex, _, _, target, err = tc.si.FindTableOrVindex(t)
 			if err != nil {
 				return err
 			}
+			if target != nil {
+				return vterrors.Errorf(vtrpcpb.Code_UNIMPLEMENTED, "unsupported: SELECT with a target destination")
+			}
 			if tbl == nil && vindex != nil {
 				tbl = newVindexTable(t.Name)
 			}


Test patch:
diff --git a/go/vt/vtgate/planbuilder/testdata/aggr_cases.txt b/go/vt/vtgate/planbuilder/testdata/aggr_cases.txt
index fcade7e03f..b08a3bb1ae 100644
--- a/go/vt/vtgate/planbuilder/testdata/aggr_cases.txt
+++ b/go/vt/vtgate/planbuilder/testdata/aggr_cases.txt
@@ -2162,6 +2162,34 @@ Gen4 plan same as above
 "syntax error: count(distinct *)"
 Gen4 plan same as above
 
+# if derived table scatter and ordering, then V3 doesn't allow outer constructs to be pushed down.
+"select count(*) from (select col, user_extra.extra from user join user_extra on user.id = user_extra.user_id order by user_extra.extra) a"
+"unsupported: cross-shard query with aggregates"
+{
+  "QueryType": "SELECT",
+  "Original": "select count(*) from (select col, user_extra.extra from user join user_extra on user.id = user_extra.user_id order by user_extra.extra) a",
+  "Instructions": {
+    "OperatorType": "Aggregate",
+    "Variant": "Ordered",
+    "Aggregates": "count(0) AS count(*)",
+    "Inputs": [
+      {
+        "OperatorType": "Route",
+        "Variant": "SelectScatter",
+        "Keyspace": {
+          "Name": "user",
+          "Sharded": true
+        },
+        "FieldQuery": "select count(*) from (select col, user_extra.extra, weight_string(user_extra.extra) from `user`, user_extra where 1 != 1) as a where 1 != 1",
+        "OrderBy": "(1|2) ASC",
+        "Query": "select count(*) from (select col, user_extra.extra, weight_string(user_extra.extra) from `user`, user_extra where `user`.id = user_extra.user_id order by user_extra.extra asc) as a",
+        "ResultColumns": 2,
+        "Table": "`user`, user_extra"
+      }
+    ]
+  }
+}
+
 # optimize group by when using distinct with no aggregation
 "select distinct col1, col2 from user group by col1, col2"
 {
diff --git a/go/vt/vtgate/planbuilder/testdata/ddl_cases_no_default_keyspace.txt b/go/vt/vtgate/planbuilder/testdata/ddl_cases_no_default_keyspace.txt
index 0950bbe9f3..c87d48386a 100644
--- a/go/vt/vtgate/planbuilder/testdata/ddl_cases_no_default_keyspace.txt
+++ b/go/vt/vtgate/planbuilder/testdata/ddl_cases_no_default_keyspace.txt
@@ -509,18 +509,6 @@ Gen4 plan same as above
     "Query": "create view view_a as select * from music where user_id = 1"
   }
 }
-{
-  "QueryType": "DDL",
-  "Original": "create view user.view_a as select sql_calc_found_rows * from music where user_id = 1",
-  "Instructions": {
-    "OperatorType": "DDL",
-    "Keyspace": {
-      "Name": "user",
-      "Sharded": true
-    },
-    "Query": "create view view_a as select sql_calc_found_rows * from music where user_id = 1"
-  }
-}
 
 # DDL
 "create index a on user(id)"
diff --git a/go/vt/vtgate/planbuilder/testdata/select_cases.txt b/go/vt/vtgate/planbuilder/testdata/select_cases.txt
index 443db1564c..5e3943dfbc 100644
--- a/go/vt/vtgate/planbuilder/testdata/select_cases.txt
+++ b/go/vt/vtgate/planbuilder/testdata/select_cases.txt
@@ -1612,7 +1612,6 @@ Gen4 plan same as above
     "Vindex": "user_index"
   }
 }
-Gen4 plan same as above
 
 # sql_calc_found_rows with limit
 "select sql_calc_found_rows * from music limit 100"
@@ -1822,11 +1821,16 @@ Gen4 plan same as above
 "syntax error at position 55 near 'union'"
 Gen4 plan same as above
 
-# Into outfile s3 in sub-query is incorrect
+# Into outfile s3 in derived table is incorrect
 "select id from (select id from user into outfile s3 'inner_outfile') as t2"
 "syntax error at position 41 near 'into'"
 Gen4 plan same as above
 
+# Into outfile s3 in derived table with union incorrect
+"select id from (select id from user into outfile s3 'inner_outfile' union select 1) as t2"
+"syntax error at position 41 near 'into'"
+Gen4 plan same as above
+
 "select (select u.id from user as u where u.id = 1), a.id from user as a where a.id = 1"
 {
   "QueryType": "SELECT",
diff --git a/go/vt/vtgate/planbuilder/testdata/unsupported_cases.txt b/go/vt/vtgate/planbuilder/testdata/unsupported_cases.txt
index bdc7bccfc1..b90e773c66 100644
--- a/go/vt/vtgate/planbuilder/testdata/unsupported_cases.txt
+++ b/go/vt/vtgate/planbuilder/testdata/unsupported_cases.txt
@@ -5,6 +5,7 @@
 # TODO: Implement support for select with a target destination
 "select * from `user[-]`.user_metadata"
 "unsupported: SELECT with a target destination"
+Gen4 plan same as above
 
 # Unsupported INSERT statement with a target destination
 "insert into `user[-]`.user_metadata (a, b) values (1,2)"
@@ -129,10 +130,6 @@ Gen4 error: unsupported: in scatter query: complex aggregate expression
 "unsupported: cross-shard query with aggregates"
 Gen4 error: Expression of SELECT list is not in GROUP BY clause and contains nonaggregated column '`user`.id' which is not functionally dependent on columns in GROUP BY clause; this is incompatible with sql_mode=only_full_group_by
 
-# if derived table scatter and ordering, then we don't allow outer constructs to be pushed down.
-"select count(*) from (select col, user_extra.extra from user join user_extra on user.id = user_extra.user_id order by user_extra.extra) a"
-"unsupported: cross-shard query with aggregates"
-
 # subqueries not supported in group by
 "select id from user group by id, (select id from user_extra)"
 "unsupported: subqueries disallowed in GROUP or ORDER BY"
@@ -140,10 +137,12 @@ Gen4 error: Expression of SELECT list is not in GROUP BY clause and contains non
 # Order by uses cross-shard expression
 "select id from user order by id+1"
 "unsupported: in scatter query: complex order by expression: id + 1"
+Gen4 plan same as above
 
 # Order by column number with collate
 "select user.col1 as a from user order by 1 collate utf8_general_ci"
 "unsupported: in scatter query: complex order by expression: 1 collate utf8_general_ci"
+Gen4 plan same as above
 
 # subqueries in update
 "update user set col = (select id from unsharded)"
@@ -352,6 +351,13 @@ Gen4 plan same as above
 # vindex func without where condition
 "select keyspace_id from user_index"
 "unsupported: where clause for vindex function must be of the form id = <val> (where clause missing)"
+Gen4 plan same as above
+
+# vindex func in subquery without where
+"select id from user where exists(select keyspace_id from user_index)"
+"unsupported: where clause for vindex function must be of the form id = <val> (where clause missing)"
+Gen4 plan same as above
+
 
 "select func(keyspace_id) from user_index where id = :id"
 "unsupported: expression on results of a vindex function"