Repository: vitessio/vitess
Pull request #9607 (2022-02-01 10:36:02)

collations: fix coercion between character sets
## Description

No more new features for the `evalengine` but let's get some more fixes in: this fixes a crash when comparing strings that have different explicit vs implicit charsets.

## Related Issue(s)
<!-- List related issues and pull requests. If this PR fixes an issue, please add it using Fixes #????  -->


## Checklist
- [ ] Should this PR be backported?
- [x] Tests were added or are not required
- [x] Documentation was added or is not required

## Deployment Notes
<!-- Notes regarding deployment of the contained body of work. These should note any db migrations, etc. -->


Patch:
diff --git a/go/vt/vtgate/evalengine/collate.go b/go/vt/vtgate/evalengine/collate.go
index 2005b13f33..44634adbf9 100644
--- a/go/vt/vtgate/evalengine/collate.go
+++ b/go/vt/vtgate/evalengine/collate.go
@@ -62,3 +62,43 @@ func (d LookupDefaultCollation) CollationForExpr(_ sqlparser.Expr) collations.ID
 func (d LookupDefaultCollation) DefaultCollation() collations.ID {
 	return collations.ID(d)
 }
+
+func mergeCollations(left, right *EvalResult) (collations.ID, error) {
+	lc := left.collation()
+	rc := right.collation()
+	if lc.Collation == rc.Collation {
+		return lc.Collation, nil
+	}
+
+	lt := left.textual()
+	rt := right.textual()
+	if !lt || !rt {
+		if lt {
+			return lc.Collation, nil
+		}
+		if rt {
+			return rc.Collation, nil
+		}
+		return collations.CollationBinaryID, nil
+	}
+
+	env := collations.Local()
+	mc, coerceLeft, coerceRight, err := env.MergeCollations(lc, rc, collations.CoercionOptions{
+		ConvertToSuperset:   true,
+		ConvertWithCoercion: true,
+	})
+	if err != nil {
+		return 0, err
+	}
+
+	if coerceLeft != nil {
+		left.bytes_, _ = coerceLeft(nil, left.bytes())
+	}
+	if coerceRight != nil {
+		right.bytes_, _ = coerceRight(nil, right.bytes())
+	}
+
+	left.replaceCollation(mc)
+	right.replaceCollation(mc)
+	return mc.Collation, nil
+}
diff --git a/go/vt/vtgate/evalengine/comparisons.go b/go/vt/vtgate/evalengine/comparisons.go
index 477f7fbdda..6947e0fe66 100644
--- a/go/vt/vtgate/evalengine/comparisons.go
+++ b/go/vt/vtgate/evalengine/comparisons.go
@@ -141,24 +141,6 @@ func evalResultsAreDateAndNumeric(l, r *EvalResult) bool {
 	return sqltypes.IsDate(ltype) && sqltypes.IsNumber(rtype) || sqltypes.IsNumber(ltype) && sqltypes.IsDate(rtype)
 }
 
-func evalCoerceAndCompare(lVal, rVal *EvalResult, fulleq bool) (int, bool, error) {
-	if lVal.collation().Collation != rVal.collation().Collation {
-		if err := mergeCollations(lVal, rVal); err != nil {
-			return 0, false, err
-		}
-	}
-	return evalCompareAll(lVal, rVal, fulleq)
-}
-
-func evalCoerceAndCompareNullSafe(lVal, rVal *EvalResult) (bool, error) {
-	if lVal.collation().Collation != rVal.collation().Collation {
-		if err := mergeCollations(lVal, rVal); err != nil {
-			return false, err
-		}
-	}
-	return evalCompareNullSafe(lVal, rVal)
-}
-
 func compareAsTuples(lVal, rVal *EvalResult) bool {
 	left := lVal.typeof() == querypb.Type_TUPLE
 	right := rVal.typeof() == querypb.Type_TUPLE
@@ -187,7 +169,7 @@ func evalCompareMany(left, right []EvalResult, fulleq bool) (int, bool, error) {
 	var seenNull bool
 	for idx, lResult := range left {
 		rResult := right[idx]
-		n, isNull, err := evalCoerceAndCompare(&lResult, &rResult, fulleq)
+		n, isNull, err := evalCompareAll(&lResult, &rResult, fulleq)
 		if err != nil {
 			return 0, false, err
 		}
@@ -264,7 +246,7 @@ func evalCompareTuplesNullSafe(lVal, rVal *EvalResult) (bool, error) {
 	}
 	for idx, lResult := range left {
 		rResult := &right[idx]
-		res, err := evalCoerceAndCompareNullSafe(&lResult, rResult)
+		res, err := evalCompareNullSafe(&lResult, rResult)
 		if err != nil {
 			return false, err
 		}
@@ -317,7 +299,7 @@ func (i *InExpr) eval(env *ExpressionEnv, result *EvalResult) {
 		}
 		if idx, ok := i.Hashed[hash]; ok {
 			var numeric int
-			numeric, foundNull, err = evalCoerceAndCompare(&left, &righttuple[idx], true)
+			numeric, foundNull, err = evalCompareAll(&left, &righttuple[idx], true)
 			if err != nil {
 				throwEvalError(err)
 			}
@@ -325,7 +307,7 @@ func (i *InExpr) eval(env *ExpressionEnv, result *EvalResult) {
 		}
 	} else {
 		for _, rtuple := range righttuple {
-			numeric, isNull, err := evalCoerceAndCompare(&left, &rtuple, true)
+			numeric, isNull, err := evalCompareAll(&left, &rtuple, true)
 			if err != nil {
 				throwEvalError(err)
 			}
@@ -370,14 +352,12 @@ func (l *LikeExpr) eval(env *ExpressionEnv, result *EvalResult) {
 	left.init(env, l.Left)
 	right.init(env, l.Right)
 
-	if err := mergeCollations(&left, &right); err != nil {
+	coll, err := mergeCollations(&left, &right)
+	if err != nil {
 		throwEvalError(err)
 	}
 
 	var matched bool
-	var leftcoll = left.collation()
-	var rightcoll = right.collation()
-
 	switch {
 	case left.typeof() == querypb.Type_TUPLE || right.typeof() == querypb.Type_TUPLE:
 		panic("failed to typecheck tuples")
@@ -385,11 +365,11 @@ func (l *LikeExpr) eval(env *ExpressionEnv, result *EvalResult) {
 		result.setNull()
 		return
 	case left.textual() && right.textual():
-		matched = l.matchWildcard(left.bytes(), right.bytes(), rightcoll.Collation)
+		matched = l.matchWildcard(left.bytes(), right.bytes(), coll)
 	case right.textual():
-		matched = l.matchWildcard(left.value().Raw(), right.bytes(), rightcoll.Collation)
+		matched = l.matchWildcard(left.value().Raw(), right.bytes(), coll)
 	case left.textual():
-		matched = l.matchWildcard(left.bytes(), right.value().Raw(), leftcoll.Collation)
+		matched = l.matchWildcard(left.bytes(), right.value().Raw(), coll)
 	default:
 		matched = l.matchWildcard(left.value().Raw(), right.value().Raw(), collations.CollationBinaryID)
 	}
diff --git a/go/vt/vtgate/evalengine/convert.go b/go/vt/vtgate/evalengine/convert.go
index 474c3b297c..0776a0a091 100644
--- a/go/vt/vtgate/evalengine/convert.go
+++ b/go/vt/vtgate/evalengine/convert.go
@@ -199,11 +199,7 @@ func convertExpr(e sqlparser.Expr, lookup ConverterLookup) (Expr, error) {
 		collation := getCollation(node, lookup)
 		return NewColumn(idx, collation), nil
 	case *sqlparser.ComparisonExpr:
-		expr, err := convertComparisonExpr(node.Operator, node.Left, node.Right, lookup)
-		if err != nil {
-			return nil, convertNotSupported(e)
-		}
-		return expr, err
+		return convertComparisonExpr(node.Operator, node.Left, node.Right, lookup)
 	case sqlparser.Argument:
 		collation := getCollation(e, lookup)
 		return NewBindVar(string(node), collation), nil
diff --git a/go/vt/vtgate/evalengine/evalengine.go b/go/vt/vtgate/evalengine/evalengine.go
index 4f31f363e3..fcae7c3e78 100644
--- a/go/vt/vtgate/evalengine/evalengine.go
+++ b/go/vt/vtgate/evalengine/evalengine.go
@@ -320,29 +320,6 @@ func compareDateAndString(l, r *EvalResult) (int, error) {
 	return compareGoTimes(lTime, rTime)
 }
 
-func mergeCollations(left, right *EvalResult) error {
-	if !left.textual() || !right.textual() {
-		return nil
-	}
-	env := collations.Local()
-	tc, coerceLeft, coerceRight, err := env.MergeCollations(left.collation(), right.collation(), collations.CoercionOptions{
-		ConvertToSuperset:   true,
-		ConvertWithCoercion: true,
-	})
-	if err != nil {
-		return err
-	}
-	if coerceLeft != nil {
-		left.bytes_, _ = coerceLeft(nil, left.bytes())
-	}
-	if coerceRight != nil {
-		right.bytes_, _ = coerceRight(nil, right.bytes())
-	}
-	left.replaceCollation(tc)
-	right.replaceCollation(tc)
-	return nil
-}
-
 func compareGoTimes(lTime, rTime time.Time) (int, error) {
 	if lTime.Before(rTime) {
 		return -1, nil
@@ -356,10 +333,11 @@ func compareGoTimes(lTime, rTime time.Time) (int, error) {
 // More on string collations coercibility on MySQL documentation:
 // 		- https://dev.mysql.com/doc/refman/8.0/en/charset-collation-coercibility.html
 func compareStrings(l, r *EvalResult) int {
-	if l.collation().Collation != r.collation().Collation {
-		panic("compareStrings: did not coerce")
+	coll, err := mergeCollations(l, r)
+	if err != nil {
+		throwEvalError(err)
 	}
-	collation := collations.Local().LookupByID(l.collation().Collation)
+	collation := collations.Local().LookupByID(coll)
 	if collation == nil {
 		panic("unknown collation after coercion")
 	}


Test patch:
diff --git a/go/vt/vtgate/evalengine/integration/comparison_test.go b/go/vt/vtgate/evalengine/integration/comparison_test.go
index 1692d93f46..d827a971ed 100644
--- a/go/vt/vtgate/evalengine/integration/comparison_test.go
+++ b/go/vt/vtgate/evalengine/integration/comparison_test.go
@@ -62,6 +62,32 @@ func compareRemoteQuery(t *testing.T, conn *mysql.Conn, query string) {
 }
 
 func TestAllComparisons(t *testing.T) {
+	var elems = []string{"NULL", "-1", "0", "1",
+		`'foo'`, `'bar'`, `'FOO'`, `'BAR'`,
+		`'foo' collate utf8mb4_as_cs`, // invalid collation for testing error messages
+		`'foo' collate utf8mb4_cs_as`,
+		`'FOO' collate utf8mb4_cs_as`,
+		`_latin1 'foo'`,
+		`_latin1 'FOO'`,
+	}
+	var operators = []string{"=", "!=", "<=>", "<", "<=", ">", ">="}
+
+	var conn = mysqlconn(t)
+	defer conn.Close()
+
+	for _, op := range operators {
+		t.Run(op, func(t *testing.T) {
+			for i := 0; i < len(elems); i++ {
+				for j := 0; j < len(elems); j++ {
+					query := fmt.Sprintf("SELECT %s %s %s", elems[i], op, elems[j])
+					compareRemoteQuery(t, conn, query)
+				}
+			}
+		})
+	}
+}
+
+func TestAllTupleComparisons(t *testing.T) {
 	var elems = []string{"NULL", "-1", "0", "1"}
 	var operators = []string{"=", "!=", "<=>", "<", "<=", ">", ">="}
 
diff --git a/go/vt/vtgate/evalengine/integration/fuzz_test.go b/go/vt/vtgate/evalengine/integration/fuzz_test.go
index a9f16c579e..d1c55ff88d 100644
--- a/go/vt/vtgate/evalengine/integration/fuzz_test.go
+++ b/go/vt/vtgate/evalengine/integration/fuzz_test.go
@@ -79,7 +79,7 @@ func (g *gencase) expr() string {
 var fuzzMaxTime = flag.Duration("fuzz-duration", 30*time.Second, "maximum time to fuzz for")
 var fuzzMaxFailures = flag.Int("fuzz-total", 0, "maximum number of failures to fuzz for")
 var fuzzSeed = flag.Int64("fuzz-seed", time.Now().Unix(), "RNG seed when generating fuzz expressions")
-var extractError = regexp.MustCompile(`(.*?) \(errno (\d+)\) \(sqlstate (\d+)\) during query: (.*?)`)
+var extractError = regexp.MustCompile(`(.*?) \(errno (\d+)\) \(sqlstate (\w+)\) during query: (.*?)`)
 
 var knownErrors = []*regexp.Regexp{
 	regexp.MustCompile(`value is out of range in '(.*?)'`),
@@ -89,7 +89,7 @@ var knownErrors = []*regexp.Regexp{
 func errorsMatch(remote, local error) bool {
 	rem := extractError.FindStringSubmatch(remote.Error())
 	if rem == nil {
-		panic("could not extract error message")
+		panic(fmt.Sprintf("could not extract error message: %q", remote.Error()))
 	}
 
 	remoteMessage := rem[1]
diff --git a/go/vt/vtgate/evalengine/mysql_test.go b/go/vt/vtgate/evalengine/mysql_test.go
index 081e0464c2..fcdd41a39d 100644
--- a/go/vt/vtgate/evalengine/mysql_test.go
+++ b/go/vt/vtgate/evalengine/mysql_test.go
@@ -118,6 +118,6 @@ func TestMySQLGolden(t *testing.T) {
 
 func TestDebug1(t *testing.T) {
 	// Debug
-	eval, err := testSingle(t, `SELECT LEAST(0.0,'foobar')`)
+	eval, err := testSingle(t, `SELECT 'foo' = _latin1 'foo'`)
 	t.Logf("eval=%s err=%v", eval.Value(), err) // want value=""
 }
diff --git a/go/vt/vtgate/planbuilder/testdata/unsupported_cases.txt b/go/vt/vtgate/planbuilder/testdata/unsupported_cases.txt
index e048f8832a..065c29cf59 100644
--- a/go/vt/vtgate/planbuilder/testdata/unsupported_cases.txt
+++ b/go/vt/vtgate/planbuilder/testdata/unsupported_cases.txt
@@ -505,11 +505,11 @@ Gen4 plan same as above
 # TODO this should be planned without using OA and MS
 "select u.id from user u join user_extra ue on ue.id = u.id group by u.id having count(u.name) = 3"
 "unsupported: cross-shard query with aggregates"
-Gen4 error: expr cannot be converted, not supported: count(u.`name`) = 3
+Gen4 error: expr cannot be converted, not supported: count(u.`name`)
 
 "select (select 1 from user u having count(ue.col) > 10) from user_extra ue"
 "symbol ue.col not found in subquery"
-Gen4 error: expr cannot be converted, not supported: count(ue.col) > 10
+Gen4 error: expr cannot be converted, not supported: count(ue.col)
 
 # aggregation filtering by having on a route with no group by
 "select 1 from user having count(id) = 10"
@@ -528,7 +528,7 @@ Gen4 error: expr cannot be converted, not supported: count(ue.col) > 10
     "Table": "`user`"
   }
 }
-Gen4 error: expr cannot be converted, not supported: count(id) = 10
+Gen4 error: expr cannot be converted, not supported: count(id)
 
 # aggregation filtering by having on a route with no group by with non-unique vindex filter
 "select 1 from user having count(id) = 10 and name = 'a'"
@@ -551,7 +551,7 @@ Gen4 error: expr cannot be converted, not supported: count(id) = 10
     "Vindex": "name_user_map"
   }
 }
-Gen4 error: expr cannot be converted, not supported: count(id) = 10
+Gen4 error: expr cannot be converted, not supported: count(id)
 
 # subquery of information_schema with itself and star expression in outer select
 "select a.*, u.id from information_schema.a a, user u where a.id in (select * from information_schema.b)"
@@ -631,7 +631,7 @@ Gen4 plan same as above
 # Equal filter with hexadecimal value
 "select count(*) a from user having a = 0x01"
 "unsupported: filtering on results of aggregates"
-Gen4 error: expr cannot be converted, not supported: a = 0x01
+Gen4 error: expr cannot be converted, not supported: 0x01
 
 # scatter aggregate with complex select list (can't build order by)
 "select distinct a+1 from user"