Fix type coercion in cascading non-literal updates (#14524)

Signed-off-by: Manan Gupta <[email protected]>
vitessio · Nov 16, 2023 · fe14d97 · fe14d97
1 parent ae5d2de
commit fe14d97
Show file tree

Hide file tree

Showing 5 changed files with 59 additions and 22 deletions.
diff --git a/go/test/endtoend/vtgate/foreignkey/fk_test.go b/go/test/endtoend/vtgate/foreignkey/fk_test.go
@@ -880,6 +880,13 @@ func TestFkQueries(t *testing.T) {
 				"insert into fk_multicol_t16 (id, cola, colb) values (1,1,1),(2,2,2)",
 				"update fk_multicol_t15 set cola = 3, colb = (id * 2) - 2",
 			},
+		}, {
+			name: "Update that sets to 0 and -0 values",
+			queries: []string{
+				"insert into fk_t15 (id, col) values (1,'-0'), (2, '0'), (3, '5'), (4, '-5')",
+				"insert into fk_t16 (id, col) values (1,'-0'), (2, '0'), (3, '5'), (4, '-5')",
+				"update fk_t15 set col = col * (col - (col))",
+			},
 		},
 	}
 

diff --git a/go/vt/vtgate/engine/cached_size.go b/go/vt/vtgate/engine/cached_size.go
diff --git a/go/vt/vtgate/engine/fk_cascade.go b/go/vt/vtgate/engine/fk_cascade.go
@@ -25,6 +25,7 @@ import (
 	querypb "vitess.io/vitess/go/vt/proto/query"
 	vtrpcpb "vitess.io/vitess/go/vt/proto/vtrpc"
 	"vitess.io/vitess/go/vt/vterrors"
+	"vitess.io/vitess/go/vt/vtgate/evalengine"
 )
 
 // FkChild contains the Child Primitive to be executed collecting the values from the Selection Primitive using the column indexes.
@@ -40,14 +41,16 @@ type FkChild struct {
 }
 
 // NonLiteralUpdateInfo stores the information required to process non-literal update queries.
-// It stores 3 information-
-// 1. UpdateExprCol- The index of the updated expression in the select query.
-// 2. UpdateExprBvName- The bind variable name to store the updated expression into.
-// 3. CompExprCol- The index of the comparison expression in the select query to know if the row value is actually being changed or not.
+// It stores 4 information-
+// 1. ExprCol- The index of the column being updated in the select query.
+// 2. CompExprCol- The index of the comparison expression in the select query to know if the row value is actually being changed or not.
+// 3. UpdateExprCol- The index of the updated expression in the select query.
+// 4. UpdateExprBvName- The bind variable name to store the updated expression into.
 type NonLiteralUpdateInfo struct {
+	ExprCol          int
+	CompExprCol      int
 	UpdateExprCol    int
 	UpdateExprBvName string
-	CompExprCol      int
 }
 
 // FkCascade is a primitive that implements foreign key cascading using Selection as values required to execute the FkChild Primitives.
@@ -185,7 +188,19 @@ func (fkc *FkCascade) executeNonLiteralExprFkChild(ctx context.Context, vcursor
 
 		// Next, we need to copy the updated expressions value into the bind variables map.
 		for _, info := range child.NonLiteralInfo {
-			bindVars[info.UpdateExprBvName] = sqltypes.ValueBindVariable(row[info.UpdateExprCol])
+			// Type case the value to that of the column that we are updating.
+			// This is required for example when we receive an updated float value of -0, but
+			// the column being updated is a varchar column, then if we don't coerce the value of -0 to
+			// varchar, MySQL ends up setting it to '0' instead of '-0'.
+			finalVal := row[info.UpdateExprCol]
+			if !finalVal.IsNull() {
+				var err error
+				finalVal, err = evalengine.CoerceTo(finalVal, selectionRes.Fields[info.ExprCol].Type)
+				if err != nil {
+					return err
+				}
+			}
+			bindVars[info.UpdateExprBvName] = sqltypes.ValueBindVariable(finalVal)
 		}
 		_, err := vcursor.ExecutePrimitive(ctx, child.Exec, bindVars, wantfields)
 		if err != nil {

diff --git a/go/vt/vtgate/planbuilder/operators/update.go b/go/vt/vtgate/planbuilder/operators/update.go
@@ -334,6 +334,7 @@ func addNonLiteralUpdExprToSelect(ctx *plancontext.PlanningContext, updExpr *sql
 	info := engine.NonLiteralUpdateInfo{
 		CompExprCol:   -1,
 		UpdateExprCol: -1,
+		ExprCol:       -1,
 	}
 	// Add the expressions to the select expressions. We make sure to reuse the offset if it has already been added once.
 	for idx, selectExpr := range exprs {
@@ -343,6 +344,9 @@ func addNonLiteralUpdExprToSelect(ctx *plancontext.PlanningContext, updExpr *sql
 		if ctx.SemTable.EqualsExpr(selectExpr.(*sqlparser.AliasedExpr).Expr, updExpr.Expr) {
 			info.UpdateExprCol = idx
 		}
+		if ctx.SemTable.EqualsExpr(selectExpr.(*sqlparser.AliasedExpr).Expr, updExpr.Name) {
+			info.ExprCol = idx
+		}
 	}
 	// If the expression doesn't exist, then we add the expression and store the offset.
 	if info.CompExprCol == -1 {
@@ -353,6 +357,10 @@ func addNonLiteralUpdExprToSelect(ctx *plancontext.PlanningContext, updExpr *sql
 		info.UpdateExprCol = len(exprs)
 		exprs = append(exprs, aeWrap(updExpr.Expr))
 	}
+	if info.ExprCol == -1 {
+		info.ExprCol = len(exprs)
+		exprs = append(exprs, aeWrap(updExpr.Name))
+	}
 	return info, exprs
 }
 

diff --git a/go/vt/vtgate/planbuilder/testdata/foreignkey_cases.json b/go/vt/vtgate/planbuilder/testdata/foreignkey_cases.json
@@ -840,9 +840,10 @@
                 ],
                 "NonLiteralUpdateInfo": [
                   {
+                    "ExprCol": 0,
+                    "CompExprCol": 1,
                     "UpdateExprCol": 2,
-                    "UpdateExprBvName": "fkc_upd",
-                    "CompExprCol": 1
+                    "UpdateExprBvName": "fkc_upd"
                   }
                 ],
                 "Query": "update u_tbl3 set col3 = null where (col3) in ::fkc_vals and (:fkc_upd is null or (u_tbl3.col3) not in ((:fkc_upd)))",
@@ -901,9 +902,10 @@
             ],
             "NonLiteralUpdateInfo": [
               {
+                "ExprCol": 0,
+                "CompExprCol": 1,
                 "UpdateExprCol": 2,
-                "UpdateExprBvName": "fkc_upd",
-                "CompExprCol": 1
+                "UpdateExprBvName": "fkc_upd"
               }
             ],
             "Inputs": [
@@ -958,9 +960,10 @@
             ],
             "NonLiteralUpdateInfo": [
               {
+                "ExprCol": 0,
+                "CompExprCol": 1,
                 "UpdateExprCol": 2,
-                "UpdateExprBvName": "fkc_upd1",
-                "CompExprCol": 1
+                "UpdateExprBvName": "fkc_upd1"
               }
             ],
             "Inputs": [
@@ -2102,9 +2105,10 @@
             ],
             "NonLiteralUpdateInfo": [
               {
+                "ExprCol": 0,
+                "CompExprCol": 1,
                 "UpdateExprCol": 2,
-                "UpdateExprBvName": "fkc_upd",
-                "CompExprCol": 1
+                "UpdateExprBvName": "fkc_upd"
               }
             ],
             "Inputs": [
@@ -2199,9 +2203,10 @@
             ],
             "NonLiteralUpdateInfo": [
               {
+                "ExprCol": 0,
+                "CompExprCol": 2,
                 "UpdateExprCol": 3,
-                "UpdateExprBvName": "fkc_upd",
-                "CompExprCol": 2
+                "UpdateExprBvName": "fkc_upd"
               }
             ],
             "Inputs": [
@@ -2327,14 +2332,16 @@
                 ],
                 "NonLiteralUpdateInfo": [
                   {
+                    "ExprCol": 0,
+                    "CompExprCol": 2,
                     "UpdateExprCol": 3,
-                    "UpdateExprBvName": "fkc_upd",
-                    "CompExprCol": 2
+                    "UpdateExprBvName": "fkc_upd"
                   },
                   {
+                    "ExprCol": 1,
+                    "CompExprCol": 4,
                     "UpdateExprCol": 5,
-                    "UpdateExprBvName": "fkc_upd1",
-                    "CompExprCol": 4
+                    "UpdateExprBvName": "fkc_upd1"
                   }
                 ],
                 "Query": "update /*+ SET_VAR(foreign_key_checks=OFF) */ u_multicol_tbl3 set cola = :fkc_upd, colb = :fkc_upd1 where (cola, colb) in ::fkc_vals",