string encoding for sql_mode value

Signed-off-by: Harshit Gangal <[email protected]>
vitessio · Oct 19, 2021 · 87ec407 · 87ec407
1 parent 30408de
commit 87ec407
Show file tree

Hide file tree

Showing 3 changed files with 63 additions and 14 deletions.
diff --git 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
@@ -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
@@ -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