Skip to content

Commit

Permalink
Fix type coercion in cascading non-literal updates (vitessio#14524)
Browse files Browse the repository at this point in the history
Signed-off-by: Manan Gupta <manan@planetscale.com>
  • Loading branch information
GuptaManan100 authored and ejortegau committed Dec 13, 2023
1 parent 4edc0f3 commit e622992
Show file tree
Hide file tree
Showing 5 changed files with 59 additions and 22 deletions.
7 changes: 7 additions & 0 deletions go/test/endtoend/vtgate/foreignkey/fk_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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))",
},
},
}

Expand Down
4 changes: 2 additions & 2 deletions go/vt/vtgate/engine/cached_size.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

27 changes: 21 additions & 6 deletions go/vt/vtgate/engine/fk_cascade.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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.
Expand Down Expand Up @@ -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 {
Expand Down
8 changes: 8 additions & 0 deletions go/vt/vtgate/planbuilder/operators/update.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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 {
Expand All @@ -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
}

Expand Down
35 changes: 21 additions & 14 deletions go/vt/vtgate/planbuilder/testdata/foreignkey_cases.json
Original file line number Diff line number Diff line change
Expand Up @@ -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)))",
Expand Down Expand Up @@ -901,9 +902,10 @@
],
"NonLiteralUpdateInfo": [
{
"ExprCol": 0,
"CompExprCol": 1,
"UpdateExprCol": 2,
"UpdateExprBvName": "fkc_upd",
"CompExprCol": 1
"UpdateExprBvName": "fkc_upd"
}
],
"Inputs": [
Expand Down Expand Up @@ -958,9 +960,10 @@
],
"NonLiteralUpdateInfo": [
{
"ExprCol": 0,
"CompExprCol": 1,
"UpdateExprCol": 2,
"UpdateExprBvName": "fkc_upd1",
"CompExprCol": 1
"UpdateExprBvName": "fkc_upd1"
}
],
"Inputs": [
Expand Down Expand Up @@ -2102,9 +2105,10 @@
],
"NonLiteralUpdateInfo": [
{
"ExprCol": 0,
"CompExprCol": 1,
"UpdateExprCol": 2,
"UpdateExprBvName": "fkc_upd",
"CompExprCol": 1
"UpdateExprBvName": "fkc_upd"
}
],
"Inputs": [
Expand Down Expand Up @@ -2199,9 +2203,10 @@
],
"NonLiteralUpdateInfo": [
{
"ExprCol": 0,
"CompExprCol": 2,
"UpdateExprCol": 3,
"UpdateExprBvName": "fkc_upd",
"CompExprCol": 2
"UpdateExprBvName": "fkc_upd"
}
],
"Inputs": [
Expand Down Expand Up @@ -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",
Expand Down

0 comments on commit e622992

Please sign in to comment.