From 23d7c4f87e7941ad648266ce6fe7f50f0359d834 Mon Sep 17 00:00:00 2001 From: Harshit Gangal Date: Tue, 19 Oct 2021 01:23:38 +0530 Subject: [PATCH 1/3] handle sql_mode differently for new value change validation Signed-off-by: Harshit Gangal --- go/vt/vtgate/engine/set.go | 66 ++++++++++++++++++++++++++++++++++---- 1 file changed, 60 insertions(+), 6 deletions(-) diff --git a/go/vt/vtgate/engine/set.go b/go/vt/vtgate/engine/set.go index 53d8fe024de..2b5cdc5ad69 100644 --- a/go/vt/vtgate/engine/set.go +++ b/go/vt/vtgate/engine/set.go @@ -327,6 +327,9 @@ func (svs *SysVarReservedConn) execSetStatement(vcursor VCursor, rss []*srvtopo. func (svs *SysVarReservedConn) checkAndUpdateSysVar(vcursor VCursor, res evalengine.ExpressionEnv) (bool, error) { sysVarExprValidationQuery := fmt.Sprintf("select %s from dual where @@%s != %s", svs.Expr, svs.Name, svs.Expr) + if svs.Name == "sql_mode" { + sysVarExprValidationQuery = fmt.Sprintf("select @@%s orig, %s new", svs.Name, svs.Expr) + } rss, _, err := vcursor.ResolveDestinations(svs.Keyspace.Name, nil, []key.Destination{key.DestinationKeyspaceID{0}}) if err != nil { return false, err @@ -335,18 +338,69 @@ func (svs *SysVarReservedConn) checkAndUpdateSysVar(vcursor VCursor, res evaleng if err != nil { return false, err } - if len(qr.Rows) == 0 { + changed := len(qr.Rows) > 0 + if !changed { return false, nil } - // TODO : validate how value needs to be stored. - value := qr.Rows[0][0] - buf := new(bytes.Buffer) - value.EncodeSQL(buf) - vcursor.Session().SetSysVar(svs.Name, buf.String()) + + var newVal string + if svs.Name == "sql_mode" { + changed, newVal = sqlModeChangedValue(qr) + if !changed { + return false, nil + } + } else { + buf := new(bytes.Buffer) + value := qr.Rows[0][0] + value.EncodeSQL(buf) + newVal = buf.String() + } + vcursor.Session().SetSysVar(svs.Name, newVal) vcursor.Session().NeedsReservedConn() return true, nil } +func sqlModeChangedValue(qr *sqltypes.Result) (bool, string) { + if len(qr.Fields) != 2 { + return false, "" + } + if len(qr.Rows[0]) != 2 { + return false, "" + } + orig := qr.Rows[0][0].ToString() + newVal := qr.Rows[0][1].ToString() + + origArr := strings.Split(orig, ",") + // Keep track of if the value is seen or not. + origMap := map[string]bool{} + for _, oVal := range origArr { + // Default is not seen. + origMap[oVal] = true + } + uniqOrigVal := len(origMap) + origValSeen := 0 + + changed := false + newValArr := strings.Split(newVal, ",") + for _, nVal := range newValArr { + notSeen, exists := origMap[nVal] + if !exists { + changed = true + break + } + if exists && notSeen { + // Value seen. Turn it off + origMap[nVal] = false + origValSeen++ + } + } + if !changed && uniqOrigVal != origValSeen { + changed = true + } + + return changed, newVal +} + var _ SetOp = (*SysVarSetAware)(nil) // MarshalJSON marshals all the json From 30408dee01c353f4719b4584b6590aeee8e8a93c Mon Sep 17 00:00:00 2001 From: Harshit Gangal Date: Tue, 19 Oct 2021 12:27:46 +0530 Subject: [PATCH 2/3] convert to upper case while storing and checking on map. Add unit tests for sql_mode behaviour in set primitive Signed-off-by: Harshit Gangal --- go/vt/vtgate/engine/set.go | 3 +- go/vt/vtgate/engine/set_test.go | 481 ++++++++++++++++++-------------- 2 files changed, 280 insertions(+), 204 deletions(-) diff --git a/go/vt/vtgate/engine/set.go b/go/vt/vtgate/engine/set.go index 2b5cdc5ad69..8373aadca3f 100644 --- a/go/vt/vtgate/engine/set.go +++ b/go/vt/vtgate/engine/set.go @@ -375,7 +375,7 @@ func sqlModeChangedValue(qr *sqltypes.Result) (bool, string) { origMap := map[string]bool{} for _, oVal := range origArr { // Default is not seen. - origMap[oVal] = true + origMap[strings.ToUpper(oVal)] = true } uniqOrigVal := len(origMap) origValSeen := 0 @@ -383,6 +383,7 @@ func sqlModeChangedValue(qr *sqltypes.Result) (bool, string) { changed := false newValArr := strings.Split(newVal, ",") for _, nVal := range newValArr { + nVal = strings.ToUpper(nVal) notSeen, exists := origMap[nVal] if !exists { changed = true diff --git a/go/vt/vtgate/engine/set_test.go b/go/vt/vtgate/engine/set_test.go index d4cb3bbd0db..46dc3705305 100644 --- a/go/vt/vtgate/engine/set_test.go +++ b/go/vt/vtgate/engine/set_test.go @@ -78,237 +78,312 @@ func TestSetTable(t *testing.T) { execErr error } - tests := []testCase{ - { - testName: "nil set ops", - expectedQueryLog: []string{}, - }, - { - testName: "udv", - setOps: []SetOp{ - &UserDefinedVariable{ - Name: "x", - Expr: evalengine.NewLiteralInt(42), - }, - }, - expectedQueryLog: []string{ - `UDV set with (x,INT64(42))`, + ks := &vindexes.Keyspace{Name: "ks", Sharded: true} + tests := []testCase{{ + testName: "nil set ops", + expectedQueryLog: []string{}, + }, { + testName: "udv", + setOps: []SetOp{ + &UserDefinedVariable{ + Name: "x", + Expr: evalengine.NewLiteralInt(42), }, }, - { - testName: "udv with input", - setOps: []SetOp{ - &UserDefinedVariable{ - Name: "x", - Expr: evalengine.NewColumn(0), - }, + expectedQueryLog: []string{ + `UDV set with (x,INT64(42))`, + }, + }, { + testName: "udv with input", + setOps: []SetOp{ + &UserDefinedVariable{ + Name: "x", + Expr: evalengine.NewColumn(0), }, - qr: []*sqltypes.Result{sqltypes.MakeTestResult( - sqltypes.MakeTestFields( - "col0", - "datetime", - ), - "2020-10-28", - )}, - expectedQueryLog: []string{ - `ResolveDestinations ks [] Destinations:DestinationAnyShard()`, - `ExecuteMultiShard ks.-20: select now() from dual {} false false`, - `UDV set with (x,DATETIME("2020-10-28"))`, + }, + qr: []*sqltypes.Result{sqltypes.MakeTestResult( + sqltypes.MakeTestFields( + "col0", + "datetime", + ), + "2020-10-28", + )}, + expectedQueryLog: []string{ + `ResolveDestinations ks [] Destinations:DestinationAnyShard()`, + `ExecuteMultiShard ks.-20: select now() from dual {} false false`, + `UDV set with (x,DATETIME("2020-10-28"))`, + }, + input: &Send{ + Keyspace: ks, + TargetDestination: key.DestinationAnyShard{}, + Query: "select now() from dual", + SingleShardOnly: true, + }, + }, { + testName: "sysvar ignore", + setOps: []SetOp{ + &SysVarIgnore{ + Name: "x", + Expr: "42", }, - input: &Send{ - Keyspace: &vindexes.Keyspace{ - Name: "ks", - Sharded: true, - }, + }, + }, { + testName: "sysvar check and ignore", + setOps: []SetOp{ + &SysVarCheckAndIgnore{ + Name: "x", + Keyspace: ks, TargetDestination: key.DestinationAnyShard{}, - Query: "select now() from dual", - SingleShardOnly: true, + Expr: "dummy_expr", }, }, - { - testName: "sysvar ignore", - setOps: []SetOp{ - &SysVarIgnore{ - Name: "x", - Expr: "42", - }, - }, + qr: []*sqltypes.Result{sqltypes.MakeTestResult( + sqltypes.MakeTestFields( + "id", + "int64", + ), + "1", + )}, + expectedQueryLog: []string{ + `ResolveDestinations ks [] Destinations:DestinationAnyShard()`, + `ExecuteMultiShard ks.-20: select 1 from dual where @@x = dummy_expr {} false false`, }, - { - testName: "sysvar check and ignore", - setOps: []SetOp{ - &SysVarCheckAndIgnore{ - Name: "x", - Keyspace: &vindexes.Keyspace{ - Name: "ks", - Sharded: true, - }, - TargetDestination: key.DestinationAnyShard{}, - Expr: "dummy_expr", - }, - }, - qr: []*sqltypes.Result{sqltypes.MakeTestResult( - sqltypes.MakeTestFields( - "id", - "int64", - ), - "1", - )}, - expectedQueryLog: []string{ - `ResolveDestinations ks [] Destinations:DestinationAnyShard()`, - `ExecuteMultiShard ks.-20: select 1 from dual where @@x = dummy_expr {} false false`, + }, { + testName: "sysvar check and error", + setOps: []SetOp{ + &SysVarCheckAndIgnore{ + Name: "x", + Keyspace: ks, + TargetDestination: key.DestinationAnyShard{}, + Expr: "dummy_expr", }, }, - { - testName: "sysvar check and error", - setOps: []SetOp{ - &SysVarCheckAndIgnore{ - Name: "x", - Keyspace: &vindexes.Keyspace{ - Name: "ks", - Sharded: true, - }, - TargetDestination: key.DestinationAnyShard{}, - Expr: "dummy_expr", - }, + expectedQueryLog: []string{ + `ResolveDestinations ks [] Destinations:DestinationAnyShard()`, + `ExecuteMultiShard ks.-20: select 1 from dual where @@x = dummy_expr {} false false`, + }, + }, { + testName: "sysvar checkAndIgnore multi destination error", + setOps: []SetOp{ + &SysVarCheckAndIgnore{ + Name: "x", + Keyspace: ks, + TargetDestination: key.DestinationAllShards{}, + Expr: "dummy_expr", }, - expectedQueryLog: []string{ - `ResolveDestinations ks [] Destinations:DestinationAnyShard()`, - `ExecuteMultiShard ks.-20: select 1 from dual where @@x = dummy_expr {} false false`, + }, + expectedQueryLog: []string{ + `ResolveDestinations ks [] Destinations:DestinationAllShards()`, + }, + expectedError: "Unexpected error, DestinationKeyspaceID mapping to multiple shards: DestinationAllShards()", + }, { + testName: "sysvar checkAndIgnore execute error", + setOps: []SetOp{ + &SysVarCheckAndIgnore{ + Name: "x", + Keyspace: ks, + TargetDestination: key.DestinationAnyShard{}, + Expr: "dummy_expr", }, }, - { - testName: "sysvar checkAndIgnore multi destination error", - setOps: []SetOp{ - &SysVarCheckAndIgnore{ - Name: "x", - Keyspace: &vindexes.Keyspace{ - Name: "ks", - Sharded: true, - }, - TargetDestination: key.DestinationAllShards{}, - Expr: "dummy_expr", - }, + expectedQueryLog: []string{ + `ResolveDestinations ks [] Destinations:DestinationAnyShard()`, + `ExecuteMultiShard ks.-20: select 1 from dual where @@x = dummy_expr {} false false`, + }, + execErr: errors.New("some random error"), + }, { + testName: "udv ignore checkAndIgnore ", + setOps: []SetOp{ + &UserDefinedVariable{ + Name: "x", + Expr: evalengine.NewLiteralInt(1), }, - expectedQueryLog: []string{ - `ResolveDestinations ks [] Destinations:DestinationAllShards()`, + &SysVarIgnore{ + Name: "y", + Expr: "2", }, - expectedError: "Unexpected error, DestinationKeyspaceID mapping to multiple shards: DestinationAllShards()", - }, - { - testName: "sysvar checkAndIgnore execute error", - setOps: []SetOp{ - &SysVarCheckAndIgnore{ - Name: "x", - Keyspace: &vindexes.Keyspace{ - Name: "ks", - Sharded: true, - }, - TargetDestination: key.DestinationAnyShard{}, - Expr: "dummy_expr", - }, + &SysVarCheckAndIgnore{ + Name: "z", + Keyspace: ks, + TargetDestination: key.DestinationAnyShard{}, + Expr: "dummy_expr", + }, + }, + expectedQueryLog: []string{ + `UDV set with (x,INT64(1))`, + `ResolveDestinations ks [] Destinations:DestinationAnyShard()`, + `ExecuteMultiShard ks.-20: select 1 from dual where @@z = dummy_expr {} false false`, + }, + qr: []*sqltypes.Result{sqltypes.MakeTestResult( + sqltypes.MakeTestFields( + "id", + "int64", + ), + "1", + )}, + }, { + testName: "sysvar set without destination", + setOps: []SetOp{ + &SysVarReservedConn{ + Name: "x", + Keyspace: ks, + TargetDestination: key.DestinationAnyShard{}, + Expr: "dummy_expr", }, - expectedQueryLog: []string{ - `ResolveDestinations ks [] Destinations:DestinationAnyShard()`, - `ExecuteMultiShard ks.-20: select 1 from dual where @@x = dummy_expr {} false false`, + }, + expectedQueryLog: []string{ + `ResolveDestinations ks [] Destinations:DestinationAnyShard()`, + `ExecuteMultiShard ks.-20: set @@x = dummy_expr {} false false`, + }, + }, { + testName: "sysvar set not modifying setting", + setOps: []SetOp{ + &SysVarReservedConn{ + Name: "x", + Keyspace: ks, + Expr: "dummy_expr", }, - execErr: errors.New("some random error"), - }, - { - testName: "udv ignore checkAndIgnore ", - setOps: []SetOp{ - &UserDefinedVariable{ - Name: "x", - Expr: evalengine.NewLiteralInt(1), - }, - &SysVarIgnore{ - Name: "y", - Expr: "2", - }, - &SysVarCheckAndIgnore{ - Name: "z", - Keyspace: &vindexes.Keyspace{ - Name: "ks", - Sharded: true, - }, - TargetDestination: key.DestinationAnyShard{}, - Expr: "dummy_expr", - }, + }, + expectedQueryLog: []string{ + `ResolveDestinations ks [] Destinations:DestinationKeyspaceID(00)`, + `ExecuteMultiShard ks.-20: select dummy_expr from dual where @@x != dummy_expr {} false false`, + }, + }, { + testName: "sysvar set modifying setting", + setOps: []SetOp{ + &SysVarReservedConn{ + Name: "x", + Keyspace: ks, + Expr: "dummy_expr", }, - expectedQueryLog: []string{ - `UDV set with (x,INT64(1))`, - `ResolveDestinations ks [] Destinations:DestinationAnyShard()`, - `ExecuteMultiShard ks.-20: select 1 from dual where @@z = dummy_expr {} false false`, + }, + expectedQueryLog: []string{ + `ResolveDestinations ks [] Destinations:DestinationKeyspaceID(00)`, + `ExecuteMultiShard ks.-20: select dummy_expr from dual where @@x != dummy_expr {} false false`, + `SysVar set with (x,123456)`, + }, + qr: []*sqltypes.Result{sqltypes.MakeTestResult( + sqltypes.MakeTestFields( + "id", + "int64", + ), + "123456", + )}, + }, { + testName: "sql_mode no change - same", + setOps: []SetOp{ + &SysVarReservedConn{ + Name: "sql_mode", + Keyspace: &vindexes.Keyspace{Name: "ks", Sharded: true}, + Expr: "'a,b'", }, - qr: []*sqltypes.Result{sqltypes.MakeTestResult( - sqltypes.MakeTestFields( - "id", - "int64", - ), - "1", - )}, - }, - { - testName: "sysvar set without destination", - setOps: []SetOp{ - &SysVarReservedConn{ - Name: "x", - Keyspace: &vindexes.Keyspace{ - Name: "ks", - Sharded: true, - }, - TargetDestination: key.DestinationAnyShard{}, - Expr: "dummy_expr", - }, + }, + expectedQueryLog: []string{ + `ResolveDestinations ks [] Destinations:DestinationKeyspaceID(00)`, + `ExecuteMultiShard ks.-20: select @@sql_mode orig, 'a,b' new {} false false`, + }, + qr: []*sqltypes.Result{sqltypes.MakeTestResult(sqltypes.MakeTestFields("orig|new", "varchar|varchar"), + "a,b|a,b", + )}, + }, { + testName: "sql_mode no change - jumbled orig", + setOps: []SetOp{ + &SysVarReservedConn{ + Name: "sql_mode", + Keyspace: &vindexes.Keyspace{Name: "ks", Sharded: true}, + Expr: "'a,b'", }, - expectedQueryLog: []string{ - `ResolveDestinations ks [] Destinations:DestinationAnyShard()`, - `ExecuteMultiShard ks.-20: set @@x = dummy_expr {} false false`, + }, + expectedQueryLog: []string{ + `ResolveDestinations ks [] Destinations:DestinationKeyspaceID(00)`, + `ExecuteMultiShard ks.-20: select @@sql_mode orig, 'a,b' new {} false false`, + }, + qr: []*sqltypes.Result{sqltypes.MakeTestResult(sqltypes.MakeTestFields("orig|new", "varchar|varchar"), + "b,a|a,b", + )}, + }, { + testName: "sql_mode no change - jumbled new", + setOps: []SetOp{ + &SysVarReservedConn{ + Name: "sql_mode", + Keyspace: &vindexes.Keyspace{Name: "ks", Sharded: true}, + Expr: "'b,a'", }, }, - { - testName: "sysvar set not modifying setting", - setOps: []SetOp{ - &SysVarReservedConn{ - Name: "x", - Keyspace: &vindexes.Keyspace{ - Name: "ks", - Sharded: true, - }, - Expr: "dummy_expr", - }, + expectedQueryLog: []string{ + `ResolveDestinations ks [] Destinations:DestinationKeyspaceID(00)`, + `ExecuteMultiShard ks.-20: select @@sql_mode orig, 'b,a' new {} false false`, + }, + qr: []*sqltypes.Result{sqltypes.MakeTestResult(sqltypes.MakeTestFields("orig|new", "varchar|varchar"), + "a,b|b,a", + )}, + }, { + testName: "sql_mode no change - same mixed case", + setOps: []SetOp{ + &SysVarReservedConn{ + Name: "sql_mode", + Keyspace: &vindexes.Keyspace{Name: "ks", Sharded: true}, + Expr: "'B,a'", }, - expectedQueryLog: []string{ - `ResolveDestinations ks [] Destinations:DestinationKeyspaceID(00)`, - `ExecuteMultiShard ks.-20: select dummy_expr from dual where @@x != dummy_expr {} false false`, + }, + expectedQueryLog: []string{ + `ResolveDestinations ks [] Destinations:DestinationKeyspaceID(00)`, + `ExecuteMultiShard ks.-20: select @@sql_mode orig, 'B,a' new {} false false`, + }, + qr: []*sqltypes.Result{sqltypes.MakeTestResult(sqltypes.MakeTestFields("orig|new", "varchar|varchar"), + "a,b|B,a", + )}, + }, { + testName: "sql_mode no change - same multiple", + setOps: []SetOp{ + &SysVarReservedConn{ + Name: "sql_mode", + Keyspace: &vindexes.Keyspace{Name: "ks", Sharded: true}, + Expr: "'B,a,A,B,b,a'", }, }, - { - testName: "sysvar set modifying setting", - setOps: []SetOp{ - &SysVarReservedConn{ - Name: "x", - Keyspace: &vindexes.Keyspace{ - Name: "ks", - Sharded: true, - }, - Expr: "dummy_expr", - }, + expectedQueryLog: []string{ + `ResolveDestinations ks [] Destinations:DestinationKeyspaceID(00)`, + `ExecuteMultiShard ks.-20: select @@sql_mode orig, 'B,a,A,B,b,a' new {} false false`, + }, + qr: []*sqltypes.Result{sqltypes.MakeTestResult(sqltypes.MakeTestFields("orig|new", "varchar|varchar"), + "a,b|B,a,A,B,b,a", + )}, + }, { + testName: "sql_mode no change - changed additional", + setOps: []SetOp{ + &SysVarReservedConn{ + Name: "sql_mode", + Keyspace: &vindexes.Keyspace{Name: "ks", Sharded: true}, + Expr: "'B,a,A,B,b,a,c'", }, - expectedQueryLog: []string{ - `ResolveDestinations ks [] Destinations:DestinationKeyspaceID(00)`, - `ExecuteMultiShard ks.-20: select dummy_expr from dual where @@x != dummy_expr {} false false`, - `SysVar set with (x,123456)`, + }, + expectedQueryLog: []string{ + `ResolveDestinations ks [] Destinations:DestinationKeyspaceID(00)`, + `ExecuteMultiShard ks.-20: select @@sql_mode orig, 'B,a,A,B,b,a,c' new {} false false`, + "SysVar set with (sql_mode,B,a,A,B,b,a,c)", + }, + qr: []*sqltypes.Result{sqltypes.MakeTestResult(sqltypes.MakeTestFields("orig|new", "varchar|varchar"), + "a,b|B,a,A,B,b,a,c", + )}, + }, { + testName: "sql_mode no change - changed less", + setOps: []SetOp{ + &SysVarReservedConn{ + Name: "sql_mode", + Keyspace: &vindexes.Keyspace{Name: "ks", Sharded: true}, + Expr: "'B,b,B,b'", }, - qr: []*sqltypes.Result{sqltypes.MakeTestResult( - sqltypes.MakeTestFields( - "id", - "int64", - ), - "123456", - )}, }, - } + expectedQueryLog: []string{ + `ResolveDestinations ks [] Destinations:DestinationKeyspaceID(00)`, + `ExecuteMultiShard ks.-20: select @@sql_mode orig, 'B,b,B,b' new {} false false`, + "SysVar set with (sql_mode,B,b,B,b)", + }, + qr: []*sqltypes.Result{sqltypes.MakeTestResult(sqltypes.MakeTestFields("orig|new", "varchar|varchar"), + "a,b|B,b,B,b", + )}, + }} for _, tc := range tests { t.Run(tc.testName, func(t *testing.T) { From 87ec40723fe6328edc24d2f80383dd96c6ec2f83 Mon Sep 17 00:00:00 2001 From: Harshit Gangal Date: Tue, 19 Oct 2021 13:42:57 +0530 Subject: [PATCH 3/3] string encoding for sql_mode value Signed-off-by: Harshit Gangal --- go/vt/vtgate/engine/set.go | 21 ++++++------ go/vt/vtgate/engine/set_test.go | 54 +++++++++++++++++++++++++++++-- go/vt/vtgate/executor_set_test.go | 2 +- 3 files changed, 63 insertions(+), 14 deletions(-) diff --git a/go/vt/vtgate/engine/set.go b/go/vt/vtgate/engine/set.go index 8373aadca3f..430c0367a44 100644 --- a/go/vt/vtgate/engine/set.go +++ b/go/vt/vtgate/engine/set.go @@ -343,29 +343,28 @@ func (svs *SysVarReservedConn) checkAndUpdateSysVar(vcursor VCursor, res evaleng return false, nil } - var newVal string + var value sqltypes.Value if svs.Name == "sql_mode" { - changed, newVal = sqlModeChangedValue(qr) + changed, value = sqlModeChangedValue(qr) if !changed { return false, nil } } else { - buf := new(bytes.Buffer) - value := qr.Rows[0][0] - value.EncodeSQL(buf) - newVal = buf.String() + value = qr.Rows[0][0] } - vcursor.Session().SetSysVar(svs.Name, newVal) + buf := new(bytes.Buffer) + value.EncodeSQL(buf) + vcursor.Session().SetSysVar(svs.Name, buf.String()) vcursor.Session().NeedsReservedConn() return true, nil } -func sqlModeChangedValue(qr *sqltypes.Result) (bool, string) { +func sqlModeChangedValue(qr *sqltypes.Result) (bool, sqltypes.Value) { if len(qr.Fields) != 2 { - return false, "" + return false, sqltypes.Value{} } if len(qr.Rows[0]) != 2 { - return false, "" + return false, sqltypes.Value{} } orig := qr.Rows[0][0].ToString() newVal := qr.Rows[0][1].ToString() @@ -399,7 +398,7 @@ func sqlModeChangedValue(qr *sqltypes.Result) (bool, string) { changed = true } - return changed, newVal + return changed, qr.Rows[0][1] } var _ SetOp = (*SysVarSetAware)(nil) diff --git a/go/vt/vtgate/engine/set_test.go b/go/vt/vtgate/engine/set_test.go index 46dc3705305..42700b6886a 100644 --- a/go/vt/vtgate/engine/set_test.go +++ b/go/vt/vtgate/engine/set_test.go @@ -361,7 +361,7 @@ func TestSetTable(t *testing.T) { expectedQueryLog: []string{ `ResolveDestinations ks [] Destinations:DestinationKeyspaceID(00)`, `ExecuteMultiShard ks.-20: select @@sql_mode orig, 'B,a,A,B,b,a,c' new {} false false`, - "SysVar set with (sql_mode,B,a,A,B,b,a,c)", + "SysVar set with (sql_mode,'B,a,A,B,b,a,c')", }, qr: []*sqltypes.Result{sqltypes.MakeTestResult(sqltypes.MakeTestFields("orig|new", "varchar|varchar"), "a,b|B,a,A,B,b,a,c", @@ -378,11 +378,61 @@ func TestSetTable(t *testing.T) { expectedQueryLog: []string{ `ResolveDestinations ks [] Destinations:DestinationKeyspaceID(00)`, `ExecuteMultiShard ks.-20: select @@sql_mode orig, 'B,b,B,b' new {} false false`, - "SysVar set with (sql_mode,B,b,B,b)", + "SysVar set with (sql_mode,'B,b,B,b')", }, qr: []*sqltypes.Result{sqltypes.MakeTestResult(sqltypes.MakeTestFields("orig|new", "varchar|varchar"), "a,b|B,b,B,b", )}, + }, { + testName: "sql_mode no change - empty list", + setOps: []SetOp{ + &SysVarReservedConn{ + Name: "sql_mode", + Keyspace: &vindexes.Keyspace{Name: "ks", Sharded: true}, + Expr: "''", + }, + }, + expectedQueryLog: []string{ + `ResolveDestinations ks [] Destinations:DestinationKeyspaceID(00)`, + `ExecuteMultiShard ks.-20: select @@sql_mode orig, '' new {} false false`, + }, + qr: []*sqltypes.Result{sqltypes.MakeTestResult(sqltypes.MakeTestFields("orig|new", "varchar|varchar"), + "|", + )}, + }, { + testName: "sql_mode no change - empty orig", + setOps: []SetOp{ + &SysVarReservedConn{ + Name: "sql_mode", + Keyspace: &vindexes.Keyspace{Name: "ks", Sharded: true}, + Expr: "'a'", + }, + }, + expectedQueryLog: []string{ + `ResolveDestinations ks [] Destinations:DestinationKeyspaceID(00)`, + `ExecuteMultiShard ks.-20: select @@sql_mode orig, 'a' new {} false false`, + "SysVar set with (sql_mode,'a')", + }, + qr: []*sqltypes.Result{sqltypes.MakeTestResult(sqltypes.MakeTestFields("orig|new", "varchar|varchar"), + "|a", + )}, + }, { + testName: "sql_mode no change - empty new", + setOps: []SetOp{ + &SysVarReservedConn{ + Name: "sql_mode", + Keyspace: &vindexes.Keyspace{Name: "ks", Sharded: true}, + Expr: "''", + }, + }, + expectedQueryLog: []string{ + `ResolveDestinations ks [] Destinations:DestinationKeyspaceID(00)`, + `ExecuteMultiShard ks.-20: select @@sql_mode orig, '' new {} false false`, + "SysVar set with (sql_mode,'')", + }, + qr: []*sqltypes.Result{sqltypes.MakeTestResult(sqltypes.MakeTestFields("orig|new", "varchar|varchar"), + "a|", + )}, }} for _, tc := range tests { diff --git a/go/vt/vtgate/executor_set_test.go b/go/vt/vtgate/executor_set_test.go index 2baa339778c..8f36adc74d9 100644 --- a/go/vt/vtgate/executor_set_test.go +++ b/go/vt/vtgate/executor_set_test.go @@ -286,7 +286,7 @@ func TestExecutorSetOp(t *testing.T) { }, { in: "set sql_mode = 'STRICT_ALL_TABLES,NO_AUTO_UPDATES'", sysVars: map[string]string{"sql_mode": "'STRICT_ALL_TABLES,NO_AUTO_UPDATES'"}, - result: returnResult("sql_mode", "varchar", "STRICT_ALL_TABLES,NO_AUTO_UPDATES"), + result: sqltypes.MakeTestResult(sqltypes.MakeTestFields("orig|new", "varchar|varchar"), "|STRICT_ALL_TABLES,NO_AUTO_UPDATES"), }, { // even though the tablet is saying that the value has changed, // useReservedConn is false, so we won't allow this change