Gen4Fallback bug fix

go/vt/vtgate/planbuilder/abstract/join.go

@@ -58,7 +58,6 @@ func (j *Join) PushPredicate(expr sqlparser.Expr, semTable *semantics.SemTable)
 			j.LeftJoin = false
 			return j.RHS.PushPredicate(expr, semTable)
 		}
-		// TODO - we should do this on the vtgate level once we have a Filter primitive
 		return vterrors.Errorf(vtrpcpb.Code_UNIMPLEMENTED, "unsupported: cross-shard left join and where clause")
 	case deps.IsSolvedBy(j.LHS.TableID().Merge(j.RHS.TableID())):
 		j.Predicate = sqlparser.AndExpressions(j.Predicate, expr)

go/vt/vtgate/planbuilder/fallback_planner.go

@@ -48,7 +48,7 @@ func (fp *fallbackPlanner) plan(query string) func(sqlparser.Statement, *sqlpars
 	backupF := fp.fallback(query)
 
 	return func(stmt sqlparser.Statement, reservedVars *sqlparser.ReservedVars, vschema ContextVSchema) (engine.Primitive, error) {
-		res, err := primaryF(stmt, reservedVars, vschema)
+		res, err := primaryF(sqlparser.CloneStatement(stmt), reservedVars, vschema)
 		if err != nil {
 			return backupF(stmt, reservedVars, vschema)
 		}

go/vt/vtgate/planbuilder/fallback_planner_test.go

@@ -28,10 +28,11 @@ import (
 )
 
 type testPlanner struct {
-	panic  interface{}
-	err    error
-	res    engine.Primitive
-	called bool
+	panic       interface{}
+	err         error
+	res         engine.Primitive
+	messWithAST func(sqlparser.Statement)
+	called      bool
 }
 
 var _ selectPlanner = (*testPlanner)(nil).plan
@@ -42,6 +43,9 @@ func (tp *testPlanner) plan(_ string) func(sqlparser.Statement, *sqlparser.Reser
 		if tp.panic != nil {
 			panic(tp.panic)
 		}
+		if tp.messWithAST != nil {
+			tp.messWithAST(statement)
+		}
 		return tp.res, tp.err
 	}
 }
@@ -78,3 +82,27 @@ func TestFallbackPlanner(t *testing.T) {
 	assert.True(t, a.called)
 	assert.True(t, b.called)
 }
+
+func TestFallbackClonesBeforePlanning(t *testing.T) {
+	a := &testPlanner{
+		messWithAST: func(statement sqlparser.Statement) {
+			sel := statement.(*sqlparser.Select)
+			sel.SelectExprs = nil
+		},
+	}
+	b := &testPlanner{}
+	fb := &fallbackPlanner{
+		primary:  a.plan,
+		fallback: b.plan,
+	}
+
+	stmt := &sqlparser.Select{
+		SelectExprs: sqlparser.SelectExprs{&sqlparser.StarExpr{}},
+	}
+	var vschema ContextVSchema
+
+	// first planner succeeds
+	_, _ = fb.plan("query")(stmt, nil, vschema)
+
+	assert.NotNilf(t, stmt.SelectExprs, "should not have changed")
+}

go/vt/vtgate/planbuilder/gen4_planner.go

@@ -237,8 +237,8 @@ func planHorizon(ctx *planningContext, plan logicalPlan, in sqlparser.SelectStat
 	case *sqlparser.Union:
 		var err error
 		rb, isRoute := plan.(*routeGen4)
-		if !isRoute && ctx.semTable.ProjectionErr != nil {
-			return nil, ctx.semTable.ProjectionErr
+		if !isRoute && ctx.semTable.ShardedError != nil {
+			return nil, ctx.semTable.ShardedError
 		}
 		if isRoute && rb.isSingleShard() {
 			err = planSingleShardRoutePlan(node, rb)

go/vt/vtgate/planbuilder/horizon_planning.go

@@ -40,8 +40,8 @@ type horizonPlanning struct {
 
 func (hp *horizonPlanning) planHorizon(ctx *planningContext, plan logicalPlan) (logicalPlan, error) {
 	rb, isRoute := plan.(*routeGen4)
-	if !isRoute && ctx.semTable.ProjectionErr != nil {
-		return nil, ctx.semTable.ProjectionErr
+	if !isRoute && ctx.semTable.ShardedError != nil {
+		return nil, ctx.semTable.ShardedError
 	}
 
 	if isRoute && rb.isSingleShard() {

go/vt/vtgate/planbuilder/testdata/filter_cases.txt

@@ -4087,3 +4087,22 @@ Gen4 plan same as above
     "Vindex": "multicolIdx"
   }
 }
+
+# left join with where clause - should be handled by gen4 but still isn't
+"select 0 from unsharded_a left join unsharded_b on unsharded_a.col = unsharded_b.col where coalesce(unsharded_b.col, 4) = 5"
+{
+  "QueryType": "SELECT",
+  "Original": "select 0 from unsharded_a left join unsharded_b on unsharded_a.col = unsharded_b.col where coalesce(unsharded_b.col, 4) = 5",
+  "Instructions": {
+    "OperatorType": "Route",
+    "Variant": "SelectUnsharded",
+    "Keyspace": {
+      "Name": "main",
+      "Sharded": false
+    },
+    "FieldQuery": "select 0 from unsharded_a left join unsharded_b on unsharded_a.col = unsharded_b.col where 1 != 1",
+    "Query": "select 0 from unsharded_a left join unsharded_b on unsharded_a.col = unsharded_b.col where coalesce(unsharded_b.col, 4) = 5",
+    "Table": "unsharded_a, unsharded_b"
+  }
+}
+Gen4 error: unsupported: cross-shard left join and where clause

go/vt/vtgate/semantics/analyzer.go

@@ -81,7 +81,7 @@ func (a analyzer) newSemTable(statement sqlparser.SelectStatement, coll collatio
 		ExprTypes:        a.typer.exprTypes,
 		Tables:           a.tables.Tables,
 		selectScope:      a.scoper.rScope,
-		ProjectionErr:    a.projErr,
+		ShardedError:     a.projErr,
 		Warning:          a.warning,
 		Comments:         statement.GetComments(),
 		SubqueryMap:      a.binder.subqueryMap,

go/vt/vtgate/semantics/analyzer_test.go

@@ -397,10 +397,10 @@ func TestUnknownColumnMap2(t *testing.T) {
 					si := &FakeSI{Tables: test.schema}
 					tbl, err := Analyze(parse.(sqlparser.SelectStatement), "", si)
 					if test.err {
-						require.True(t, err != nil || tbl.ProjectionErr != nil)
+						require.True(t, err != nil || tbl.ShardedError != nil)
 					} else {
 						require.NoError(t, err)
-						require.NoError(t, tbl.ProjectionErr)
+						require.NoError(t, tbl.ShardedError)
 						typ := tbl.TypeFor(expr)
 						assert.Equal(t, test.typ, typ)
 					}
@@ -507,7 +507,7 @@ func TestScopeForSubqueries(t *testing.T) {
 			sel2 := sel.SelectExprs[1].(*sqlparser.AliasedExpr).Expr.(*sqlparser.Subquery).Select.(*sqlparser.Select)
 			exp := extract(sel2, 0)
 			s1 := semTable.RecursiveDeps(exp)
-			require.NoError(t, semTable.ProjectionErr)
+			require.NoError(t, semTable.ShardedError)
 			// if scoping works as expected, we should be able to see the inner table being used by the inner expression
 			assert.Equal(t, tc.deps, s1)
 		})

go/vt/vtgate/semantics/semantic_state.go

@@ -70,9 +70,8 @@ type (
 	// SemTable contains semantic analysis information about the query.
 	SemTable struct {
 		Tables []TableInfo
-		// ProjectionErr stores the error that we got during the semantic analysis of the SelectExprs.
-		// This is only a real error if we are unable to plan the query as a single route
-		ProjectionErr error
+		// ShardedError stores any errors that have to be generated if the query cannot be planned as a single route.
+		ShardedError error
 
 		// Recursive contains the dependencies from the expression to the actual tables
 		// in the query (i.e. not including derived tables). If an expression is a column on a derived table,