v3: address review comments

go/vt/sqlparser/ast.go

@@ -341,6 +341,7 @@ func (*JoinTableExpr) iTableExpr()    {}
 
 // AliasedTableExpr represents a table expression
 // coupled with an optional alias or index hint.
+// If As is empty, no alias was used.
 type AliasedTableExpr struct {
 	Expr  SimpleTableExpr
 	As    SQLName
@@ -369,6 +370,9 @@ func (*TableName) iSimpleTableExpr() {}
 func (*Subquery) iSimpleTableExpr()  {}
 
 // TableName represents a table  name.
+// Qualifier, if specified, represents a database.
+// It's generally not supported because vitess has its own
+// rules about which database to send a query to.
 type TableName struct {
 	Name, Qualifier SQLName
 }

go/vt/vtgate/planbuilder/select2.go

@@ -11,6 +11,13 @@ import (
 	"github.com/youtube/vitess/go/vt/sqlparser"
 )
 
+// This file contains routines for processing the FROM
+// clause. Functions in this file manipulate various data
+// structures. If they return an error, one should assume
+// that the data structures may be in an inconsistent state.
+// In general, the error should just be returned back to the
+// application.
+
 // PlanBuilder represents any object that's used to
 // build a plan. The top-level PlanBuilder will be a
 // tree that points to other PlanBuilder objects.
@@ -118,7 +125,7 @@ type Route struct {
 	// Vindex represents the vindex that will be used
 	// to resolve the route.
 	Vindex Vindex `json:",omitempty"`
-	// Values represents a single value or a list of
+	// Values can be a single value or a list of
 	// values that will be used as input to the Vindex
 	// to compute the target shard(s) where the query must
 	// be sent.
@@ -137,7 +144,7 @@ func buildSelectPlan2(sel *sqlparser.Select, schema *Schema) (PlanBuilder, *Symb
 func processTableExprs(tableExprs sqlparser.TableExprs, schema *Schema) (PlanBuilder, *SymbolTable, error) {
 	if len(tableExprs) != 1 {
 		// TODO(sougou): better error message.
-		return nil, nil, errors.New("no list")
+		return nil, nil, errors.New("lists are not supported")
 	}
 	return processTableExpr(tableExprs[0], schema)
 }
@@ -150,6 +157,9 @@ func processTableExpr(tableExpr sqlparser.TableExpr, schema *Schema) (PlanBuilde
 		return processAliasedTable(tableExpr, schema)
 	case *sqlparser.ParenTableExpr:
 		planBuilder, symbols, err := processTableExprs(tableExpr.Exprs, schema)
+		// We want to point to the higher level parenthesis because
+		// more routes can be merged with this one. If so, the order
+		// should be maintained as dictated by the parenthesis.
 		if route, ok := planBuilder.(*RouteBuilder); ok {
 			route.From = tableExpr
 		}
@@ -286,11 +296,8 @@ func joinRoutes(lRouteBuilder *RouteBuilder, lsymbols *SymbolTable, rRouteBuilde
 		return makeJoinBuilder(lRouteBuilder, lsymbols, rRouteBuilder, rsymbols, join)
 	}
 	if lRouteBuilder.Route.PlanID == SelectUnsharded {
-		if rRouteBuilder.Route.PlanID == SelectUnsharded {
-			// Two Routes from the same unsharded keyspace can be merged.
-			return mergeRoutes(lRouteBuilder, lsymbols, rsymbols, join)
-		}
-		return makeJoinBuilder(lRouteBuilder, lsymbols, rRouteBuilder, rsymbols, join)
+		// Two Routes from the same unsharded keyspace can be merged.
+		return mergeRoutes(lRouteBuilder, lsymbols, rRouteBuilder, rsymbols, join)
 	}
 	// lRouteBuilder is a sharded route. It can't merge with an unsharded route.
 	if rRouteBuilder.Route.PlanID == SelectUnsharded {
@@ -302,8 +309,9 @@ func joinRoutes(lRouteBuilder *RouteBuilder, lsymbols *SymbolTable, rRouteBuilde
 }
 
 // mergeRoutes makes a new RouteBuilder by joining the left and right
-// nodes of a join. This is called if two routes can be merged.
-func mergeRoutes(lRouteBuilder *RouteBuilder, lsymbols, rsymbols *SymbolTable, join *sqlparser.JoinTableExpr) (PlanBuilder, *SymbolTable, error) {
+// nodes of a join. The merged RouteBuilder inherits the plan of the
+// left Route. This function is called if two routes can be merged.
+func mergeRoutes(lRouteBuilder *RouteBuilder, lsymbols *SymbolTable, rRouteBuilder *RouteBuilder, rsymbols *SymbolTable, join *sqlparser.JoinTableExpr) (PlanBuilder, *SymbolTable, error) {
 	lRouteBuilder.From = join
 	err := lsymbols.Merge(rsymbols, lRouteBuilder)
 	if err != nil {
@@ -318,10 +326,9 @@ func mergeRoutes(lRouteBuilder *RouteBuilder, lsymbols, rsymbols *SymbolTable, j
 func joinShardedRoutes(lRouteBuilder *RouteBuilder, lsymbols *SymbolTable, rRouteBuilder *RouteBuilder, rsymbols *SymbolTable, join *sqlparser.JoinTableExpr) (PlanBuilder, *SymbolTable, error) {
 	onFilters := appendFilters(nil, join.On)
 	for _, filter := range onFilters {
-		if !isSameRoute(filter, lsymbols, rsymbols) {
-			continue
+		if isSameRoute(filter, lsymbols, rsymbols) {
+			return mergeRoutes(lRouteBuilder, lsymbols, rRouteBuilder, rsymbols, join)
 		}
-		return mergeRoutes(lRouteBuilder, lsymbols, rsymbols, join)
 	}
 	return makeJoinBuilder(lRouteBuilder, lsymbols, rRouteBuilder, rsymbols, join)
 }

go/vt/vtgate/planbuilder/symboltable.go

@@ -12,7 +12,7 @@ import (
 
 // SymbolTable contains the symbols for a SELECT
 // statement. If it's for a subquery, it points to
-// an outer scope.
+// an outer scope. For now, it only contains tables.
 type SymbolTable struct {
 	tables map[sqlparser.SQLName]*TableAlias
 	outer  *SymbolTable
@@ -20,13 +20,14 @@ type SymbolTable struct {
 
 // TableAlias is part of SymbolTable.
 // It represnts a table alias in a FROM clause.
+// TODO(sougou): Update comments after the struct is finalized.
 type TableAlias struct {
 	// Name represents the name of the alias.
 	Name sqlparser.SQLName
 	// Keyspace points to the keyspace to which this
 	// alias belongs.
 	Keyspace *Keyspace
-	// CoVindexes is the list of column Vindexes for this alisas.
+	// CoVindexes is the list of column Vindexes for this alias.
 	ColVindexes []*ColVindex
 	// Route points to the RouteBuilder object under which this alias
 	// was created.
@@ -49,7 +50,7 @@ func NewSymbolTable(alias sqlparser.SQLName, table *Table, route *RouteBuilder)
 }
 
 // Add merges the new symbol table into the current one
-// without mergine their routes. This means that the new symbols
+// without merging their routes. This means that the new symbols
 // will belong to different routes
 func (smt *SymbolTable) Add(symbols *SymbolTable) error {
 	for k, v := range symbols.tables {
@@ -61,9 +62,8 @@ func (smt *SymbolTable) Add(symbols *SymbolTable) error {
 	return nil
 }
 
-// Merge merges the new symbol table into the current as part of
-// the same route. So, all symbols will be changed to point to the new
-// RouteBuilder.
+// Merge merges the new symbol table into the current one and makes
+// all the tables part of the specified RouteBuilder.
 func (smt *SymbolTable) Merge(symbols *SymbolTable, route *RouteBuilder) error {
 	for _, v := range smt.tables {
 		v.Route = route