From fe14d97754157e885f28f018852d9ef4fd673cd6 Mon Sep 17 00:00:00 2001 From: Manan Gupta <35839558+GuptaManan100@users.noreply.github.com> Date: Thu, 16 Nov 2023 12:10:22 +0530 Subject: [PATCH] Fix type coercion in cascading non-literal updates (#14524) Signed-off-by: Manan Gupta --- go/test/endtoend/vtgate/foreignkey/fk_test.go | 7 ++++ go/vt/vtgate/engine/cached_size.go | 4 +-- go/vt/vtgate/engine/fk_cascade.go | 27 ++++++++++---- go/vt/vtgate/planbuilder/operators/update.go | 8 +++++ .../testdata/foreignkey_cases.json | 35 +++++++++++-------- 5 files changed, 59 insertions(+), 22 deletions(-) diff --git a/go/test/endtoend/vtgate/foreignkey/fk_test.go b/go/test/endtoend/vtgate/foreignkey/fk_test.go index a05f0de7ac1..7a29136e915 100644 --- 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 index 6121ad5f675..a18e2108c4a 100644 --- a/go/vt/vtgate/engine/cached_size.go +++ b/go/vt/vtgate/engine/cached_size.go @@ -290,7 +290,7 @@ func (cached *FkChild) CachedSize(alloc bool) int64 { } // field NonLiteralInfo []vitess.io/vitess/go/vt/vtgate/engine.NonLiteralUpdateInfo { - size += hack.RuntimeAllocSize(int64(cap(cached.NonLiteralInfo)) * int64(32)) + size += hack.RuntimeAllocSize(int64(cap(cached.NonLiteralInfo)) * int64(40)) for _, elem := range cached.NonLiteralInfo { size += elem.CachedSize(false) } @@ -625,7 +625,7 @@ func (cached *NonLiteralUpdateInfo) CachedSize(alloc bool) int64 { } size := int64(0) if alloc { - size += int64(32) + size += int64(48) } // field UpdateExprBvName string size += hack.RuntimeAllocSize(int64(len(cached.UpdateExprBvName))) diff --git a/go/vt/vtgate/engine/fk_cascade.go b/go/vt/vtgate/engine/fk_cascade.go index 691e326fec7..94732ae161b 100644 --- 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 index b7a3a4da2d6..9d39b2f4fc7 100644 --- 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 index 0a57806cb1d..f77cb4aeac3 100644 --- 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",