Ensure hexval and int don't share bindvar

There was some existing code that attempted to disambiguate literal
values as map keys by prefixing a single quote. Since this proved
to not be correctly handling all situations, we thought it appropriate
to key by the Literal itself. This ensures that a literal type is also
considered first class in disambiguation.

We chose to use the Literal rather than *Literal because literals with
the same type and value are intended to share a BindVar.

There's now slightly more data in the key since it is a struct with
a Type field of Int, but since the key previously was an arbitrary length
string, the difference should be negligble. Running the benchmarks
didn't seem to indicate anything of concern.

Signed-off-by: William Martin <[email protected]>

go/vt/sqlparser/normalizer.go

@@ -46,7 +46,7 @@ func Normalize(stmt Statement, reserved *ReservedVars, bindVars map[string]*quer
 type normalizer struct {
 	bindVars  map[string]*querypb.BindVariable
 	reserved  *ReservedVars
-	vals      map[string]string
+	vals      map[Literal]string
 	err       error
 	inDerived bool
 }
@@ -55,7 +55,7 @@ func newNormalizer(reserved *ReservedVars, bindVars map[string]*querypb.BindVari
 	return &normalizer{
 		bindVars: bindVars,
 		reserved: reserved,
-		vals:     make(map[string]string),
+		vals:     make(map[Literal]string),
 	}
 }
 
@@ -198,30 +198,18 @@ func (nz *normalizer) convertLiteralDedup(node *Literal, cursor *Cursor) {
 	}
 
 	// Check if there's a bindvar for that value already.
-	key := keyFor(bval, node)
-	bvname, ok := nz.vals[key]
+	bvname, ok := nz.vals[*node]
 	if !ok {
 		// If there's no such bindvar, make a new one.
 		bvname = nz.reserved.nextUnusedVar()
-		nz.vals[key] = bvname
+		nz.vals[*node] = bvname
 		nz.bindVars[bvname] = bval
 	}
 
 	// Modify the AST node to a bindvar.
 	cursor.Replace(NewTypedArgument(bvname, node.SQLType()))
 }
 
-func keyFor(bval *querypb.BindVariable, lit *Literal) string {
-	if bval.Type != sqltypes.VarBinary && bval.Type != sqltypes.VarChar {
-		return lit.Val
-	}
-
-	// Prefixing strings with "'" ensures that a string
-	// and number that have the same representation don't
-	// collide.
-	return "'" + lit.Val
-}
-
 // convertLiteral converts an Literal without the dedup.
 func (nz *normalizer) convertLiteral(node *Literal, cursor *Cursor) {
 	err := validateLiteral(node)
@@ -279,15 +267,14 @@ func (nz *normalizer) parameterize(left, right Expr) Expr {
 	if bval == nil {
 		return nil
 	}
-	key := keyFor(bval, lit)
-	bvname := nz.decideBindVarName(key, lit, col, bval)
+	bvname := nz.decideBindVarName(lit, col, bval)
 	return NewTypedArgument(bvname, lit.SQLType())
 }
 
-func (nz *normalizer) decideBindVarName(key string, lit *Literal, col *ColName, bval *querypb.BindVariable) string {
+func (nz *normalizer) decideBindVarName(lit *Literal, col *ColName, bval *querypb.BindVariable) string {
 	if len(lit.Val) <= 256 {
 		// first we check if we already have a bindvar for this value. if we do, we re-use that bindvar name
-		bvname, ok := nz.vals[key]
+		bvname, ok := nz.vals[*lit]
 		if ok {
 			return bvname
 		}
@@ -297,7 +284,7 @@ func (nz *normalizer) decideBindVarName(key string, lit *Literal, col *ColName,
 	// Big values are most likely not for vindexes.
 	// We save a lot of CPU because we avoid building
 	bvname := nz.reserved.ReserveColName(col)
-	nz.vals[key] = bvname
+	nz.vals[*lit] = bvname
 	nz.bindVars[bvname] = bval
 
 	return bvname