refactor: clean up code after code review

Signed-off-by: Andres Taylor <andres@planetscale.com>
vitessio · Nov 17, 2021 · 45ea9b2 · 45ea9b2
1 parent 08b39ab
commit 45ea9b2
Show file tree

Hide file tree

Showing 4 changed files with 13 additions and 25 deletions.
diff --git a/go/vt/sqlparser/comments.go b/go/vt/sqlparser/comments.go
@@ -239,18 +239,6 @@ func ExtractCommentDirectives(comments Comments) CommentDirectives {
 			strVal := directive[sep+1:]
 			directive = directive[:sep]
 
-			// merging strVal with next directives to support comma-separated lists with spaces
-			// i.e: VAR=(a, b, c)
-			if strings.HasPrefix(strVal, "(") {
-				for j := i + 1; j < len(directives)-1; j++ {
-					strVal += directives[j]
-					if strings.HasSuffix(directives[j], ")") {
-						i = j
-						break
-					}
-				}
-			}
-
 			intVal, err := strconv.Atoi(strVal)
 			if err == nil {
 				vals[directive] = intVal

diff --git a/go/vt/vtgate/engine/hash_join.go b/go/vt/vtgate/engine/hash_join.go
@@ -175,7 +175,7 @@ func (hj *HashJoin) TryStreamExecute(vcursor VCursor, bindVars map[string]*query
 			for _, currentLHSRow := range lftRows {
 				lhsVal := currentLHSRow[hj.LHSKey]
 				// hash codes can give false positives, so we need to check with a real comparison as well
-				cmp, err := evalengine.NullsafeCompare(joinVal, lhsVal, collations.Unknown)
+				cmp, err := evalengine.NullsafeCompare(joinVal, lhsVal, hj.Collation)
 				if err != nil {
 					return err
 				}

diff --git a/go/vt/vtgate/planbuilder/gen4_planner.go b/go/vt/vtgate/planbuilder/gen4_planner.go
@@ -27,6 +27,8 @@ import (
 
 var _ selectPlanner = gen4Planner
 
+const MinHashJoinCost = 5
+
 func gen4Planner(query string) func(sqlparser.Statement, *sqlparser.ReservedVars, ContextVSchema) (engine.Primitive, error) {
 	return func(stmt sqlparser.Statement, reservedVars *sqlparser.ReservedVars, vschema ContextVSchema) (engine.Primitive, error) {
 		selStatement, ok := stmt.(sqlparser.SelectStatement)

diff --git a/go/vt/vtgate/planbuilder/querytree_transformers.go b/go/vt/vtgate/planbuilder/querytree_transformers.go
@@ -256,11 +256,8 @@ func getCollationsFor(ctx *planningContext, n *concatenateTree) []collations.ID
 		if !ok {
 			return nil
 		}
-		typ, ok := ctx.semTable.ExprTypes[aliasedE.Expr]
-		if !ok {
-			return nil
-		}
-		colls = append(colls, typ.Collation)
+		typ := ctx.semTable.CollationFor(aliasedE.Expr)
+		colls = append(colls, typ)
 	}
 	return colls
 }
@@ -505,14 +502,15 @@ func transformJoinPlan(ctx *planningContext, n *joinTree) (logicalPlan, error) {
 	}
 
 	if lhsInfo.typ.Collation != rhsInfo.typ.Collation {
-		return nil, vterrors.Errorf(vtrpcpb.Code_UNIMPLEMENTED, "joins with different collations are not yet supported")
-	}
-	coercedType, err := evalengine.CoerceTo(lhsInfo.typ.Type, rhsInfo.typ.Type)
-	if err != nil {
-		return nil, err
+		// joins with different collations are not yet supported
+		canHashJoin = false
 	}
 
 	if canHashJoin {
+		coercedType, err := evalengine.CoerceTo(lhsInfo.typ.Type, rhsInfo.typ.Type)
+		if err != nil {
+			return nil, err
+		}
 		return &hashJoin{
 			Left:           lhs,
 			Right:          rhs,
@@ -541,7 +539,7 @@ func transformJoinPlan(ctx *planningContext, n *joinTree) (logicalPlan, error) {
 // Hash joins are only supporting equality join predicates, which is why the join predicate
 // has to be an EqualOp.
 func canHashJoin(ctx *planningContext, n *joinTree) (canHash bool, lhs, rhs joinColumnInfo, err error) {
-	if len(n.predicatesToRemoveFromHashJoin) != 1 || n.rhs.cost() <= 5 || n.leftJoin ||
+	if len(n.predicatesToRemoveFromHashJoin) != 1 || n.rhs.cost() <= MinHashJoinCost || n.leftJoin ||
 		!sqlparser.ExtractCommentDirectives(ctx.semTable.Comments).IsSet(sqlparser.DirectiveAllowHashJoin) {
 		return
 	}
@@ -591,7 +589,7 @@ func canHashJoin(ctx *planningContext, n *joinTree) (canHash bool, lhs, rhs join
 
 	columns, err := n.rhs.pushOutputColumns([]*sqlparser.ColName{col}, ctx.semTable)
 	if err != nil {
-		return
+		return false, lhs, rhs, nil
 	}
 	if len(columns) != 1 {
 		return