From 7ab93d7f62e4d4a78eaa7a0f0c4d29f9ab8fc1ff Mon Sep 17 00:00:00 2001 From: amyangfei Date: Tue, 9 Jul 2019 11:51:05 +0800 Subject: [PATCH 01/13] expression: add max_allowed_packet check in concat/concat_ws --- expression/builtin_string.go | 31 ++++++++++- expression/builtin_string_test.go | 91 +++++++++++++++++++++++++++++++ 2 files changed, 119 insertions(+), 3 deletions(-) diff --git a/expression/builtin_string.go b/expression/builtin_string.go index 5fb2b5a3183c8..aa927804b952a 100644 --- a/expression/builtin_string.go +++ b/expression/builtin_string.go @@ -273,17 +273,26 @@ func (c *concatFunctionClass) getFunction(ctx sessionctx.Context, args []Express if bf.tp.Flen >= mysql.MaxBlobWidth { bf.tp.Flen = mysql.MaxBlobWidth } - sig := &builtinConcatSig{bf} + + valStr, _ := ctx.GetSessionVars().GetSystemVar(variable.MaxAllowedPacket) + maxAllowedPacket, err := strconv.ParseUint(valStr, 10, 64) + if err != nil { + return nil, errors.Trace(err) + } + + sig := &builtinConcatSig{bf, maxAllowedPacket} return sig, nil } type builtinConcatSig struct { baseBuiltinFunc + maxAllowedPacket uint64 } func (b *builtinConcatSig) Clone() builtinFunc { newSig := &builtinConcatSig{} newSig.cloneFrom(&b.baseBuiltinFunc) + newSig.maxAllowedPacket = b.maxAllowedPacket return newSig } @@ -298,6 +307,10 @@ func (b *builtinConcatSig) evalString(row chunk.Row) (d string, isNull bool, err } s = append(s, []byte(d)...) } + if uint64(len(s)) > b.maxAllowedPacket { + b.ctx.GetSessionVars().StmtCtx.AppendWarning(errWarnAllowedPacketOverflowed.GenWithStackByArgs("concat", b.maxAllowedPacket)) + return "", true, nil + } return string(s), false, nil } @@ -338,12 +351,19 @@ func (c *concatWSFunctionClass) getFunction(ctx sessionctx.Context, args []Expre bf.tp.Flen = mysql.MaxBlobWidth } - sig := &builtinConcatWSSig{bf} + valStr, _ := ctx.GetSessionVars().GetSystemVar(variable.MaxAllowedPacket) + maxAllowedPacket, err := strconv.ParseUint(valStr, 10, 64) + if err != nil { + return nil, errors.Trace(err) + } + + sig := &builtinConcatWSSig{bf, maxAllowedPacket} return sig, nil } type builtinConcatWSSig struct { baseBuiltinFunc + maxAllowedPacket uint64 } func (b *builtinConcatWSSig) Clone() builtinFunc { @@ -381,8 +401,13 @@ func (b *builtinConcatWSSig) evalString(row chunk.Row) (string, bool, error) { strs = append(strs, val) } + result := strings.Join(strs, sep) + if uint64(len(result)) > b.maxAllowedPacket { + b.ctx.GetSessionVars().StmtCtx.AppendWarning(errWarnAllowedPacketOverflowed.GenWithStackByArgs("concat_ws", b.maxAllowedPacket)) + return "", true, nil + } // TODO: check whether the length of result is larger than Flen - return strings.Join(strs, sep), false, nil + return result, false, nil } type leftFunctionClass struct { diff --git a/expression/builtin_string_test.go b/expression/builtin_string_test.go index 5c9b6debb62e4..b07ebd7d11f13 100644 --- a/expression/builtin_string_test.go +++ b/expression/builtin_string_test.go @@ -171,6 +171,50 @@ func (s *testEvaluatorSuite) TestConcat(c *C) { } } +func (s *testEvaluatorSuite) TestConcatSig(c *C) { + colTypes := []*types.FieldType{ + {Tp: mysql.TypeVarchar}, + {Tp: mysql.TypeVarchar}, + } + resultType := &types.FieldType{Tp: mysql.TypeVarchar, Flen: 1000} + args := []Expression{ + &Column{Index: 0, RetType: colTypes[0]}, + &Column{Index: 1, RetType: colTypes[1]}, + } + base := baseBuiltinFunc{args: args, ctx: s.ctx, tp: resultType} + concat := &builtinConcatSig{base, 5} + + cases := []struct { + args []interface{} + warnings int + res string + }{ + {[]interface{}{"a", "b"}, 0, "ab"}, + {[]interface{}{"aaa", "bbb"}, 1, ""}, + {[]interface{}{"中", "a"}, 0, "中a"}, + {[]interface{}{"中文", "a"}, 2, ""}, + } + + for _, t := range cases { + input := chunk.NewChunkWithCapacity(colTypes, 10) + input.AppendString(0, t.args[0].(string)) + input.AppendString(1, t.args[1].(string)) + + res, isNull, err := concat.evalString(input.GetRow(0)) + c.Assert(res, Equals, t.res) + c.Assert(err, IsNil) + if t.warnings == 0 { + c.Assert(isNull, IsFalse) + } else { + c.Assert(isNull, IsTrue) + warnings := s.ctx.GetSessionVars().StmtCtx.GetWarnings() + c.Assert(warnings, HasLen, t.warnings) + lastWarn := warnings[len(warnings)-1] + c.Assert(terror.ErrorEqual(errWarnAllowedPacketOverflowed, lastWarn.Err), IsTrue) + } + } +} + func (s *testEvaluatorSuite) TestConcatWS(c *C) { defer testleak.AfterTest(c)() cases := []struct { @@ -246,6 +290,53 @@ func (s *testEvaluatorSuite) TestConcatWS(c *C) { c.Assert(err, IsNil) } +func (s *testEvaluatorSuite) TestConcatWSSig(c *C) { + colTypes := []*types.FieldType{ + {Tp: mysql.TypeVarchar}, + {Tp: mysql.TypeVarchar}, + {Tp: mysql.TypeVarchar}, + } + resultType := &types.FieldType{Tp: mysql.TypeVarchar, Flen: 1000} + args := []Expression{ + &Column{Index: 0, RetType: colTypes[0]}, + &Column{Index: 1, RetType: colTypes[1]}, + &Column{Index: 2, RetType: colTypes[2]}, + } + base := baseBuiltinFunc{args: args, ctx: s.ctx, tp: resultType} + concat := &builtinConcatWSSig{base, 6} + + cases := []struct { + args []interface{} + warnings int + res string + }{ + {[]interface{}{",", "a", "b"}, 0, "a,b"}, + {[]interface{}{",", "aaa", "bbb"}, 1, ""}, + {[]interface{}{",", "中", "a"}, 0, "中,a"}, + {[]interface{}{",", "中文", "a"}, 2, ""}, + } + + for _, t := range cases { + input := chunk.NewChunkWithCapacity(colTypes, 10) + input.AppendString(0, t.args[0].(string)) + input.AppendString(1, t.args[1].(string)) + input.AppendString(2, t.args[2].(string)) + + res, isNull, err := concat.evalString(input.GetRow(0)) + c.Assert(res, Equals, t.res) + c.Assert(err, IsNil) + if t.warnings == 0 { + c.Assert(isNull, IsFalse) + } else { + c.Assert(isNull, IsTrue) + warnings := s.ctx.GetSessionVars().StmtCtx.GetWarnings() + c.Assert(warnings, HasLen, t.warnings) + lastWarn := warnings[len(warnings)-1] + c.Assert(terror.ErrorEqual(errWarnAllowedPacketOverflowed, lastWarn.Err), IsTrue) + } + } +} + func (s *testEvaluatorSuite) TestLeft(c *C) { defer testleak.AfterTest(c)() stmtCtx := s.ctx.GetSessionVars().StmtCtx From f9dab8d7ff159ad4e322913f1b26bcb761453ffe Mon Sep 17 00:00:00 2001 From: amyangfei Date: Tue, 9 Jul 2019 14:25:11 +0800 Subject: [PATCH 02/13] remove errors Trace --- expression/builtin_string.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/expression/builtin_string.go b/expression/builtin_string.go index aa927804b952a..46193f28b90b7 100644 --- a/expression/builtin_string.go +++ b/expression/builtin_string.go @@ -277,7 +277,7 @@ func (c *concatFunctionClass) getFunction(ctx sessionctx.Context, args []Express valStr, _ := ctx.GetSessionVars().GetSystemVar(variable.MaxAllowedPacket) maxAllowedPacket, err := strconv.ParseUint(valStr, 10, 64) if err != nil { - return nil, errors.Trace(err) + return nil, err } sig := &builtinConcatSig{bf, maxAllowedPacket} @@ -354,7 +354,7 @@ func (c *concatWSFunctionClass) getFunction(ctx sessionctx.Context, args []Expre valStr, _ := ctx.GetSessionVars().GetSystemVar(variable.MaxAllowedPacket) maxAllowedPacket, err := strconv.ParseUint(valStr, 10, 64) if err != nil { - return nil, errors.Trace(err) + return nil, err } sig := &builtinConcatWSSig{bf, maxAllowedPacket} From 0a8f1d15b69823ee396fe05aa725e91222d3fe1f Mon Sep 17 00:00:00 2001 From: amyangfei Date: Tue, 9 Jul 2019 14:45:22 +0800 Subject: [PATCH 03/13] fix ci --- expression/builtin_string.go | 1 + expression/integration_test.go | 1 + planner/core/exhaust_physical_plans_test.go | 2 ++ planner/core/logical_plan_test.go | 2 ++ 4 files changed, 6 insertions(+) diff --git a/expression/builtin_string.go b/expression/builtin_string.go index 46193f28b90b7..60b3557e8b256 100644 --- a/expression/builtin_string.go +++ b/expression/builtin_string.go @@ -369,6 +369,7 @@ type builtinConcatWSSig struct { func (b *builtinConcatWSSig) Clone() builtinFunc { newSig := &builtinConcatWSSig{} newSig.cloneFrom(&b.baseBuiltinFunc) + newSig.maxAllowedPacket = b.maxAllowedPacket return newSig } diff --git a/expression/integration_test.go b/expression/integration_test.go index ff0ccb762251a..2dba788cb98a5 100644 --- a/expression/integration_test.go +++ b/expression/integration_test.go @@ -557,6 +557,7 @@ func (s *testIntegrationSuite) TestMathBuiltin(c *C) { } func (s *testIntegrationSuite) TestStringBuiltin(c *C) { + s.ctx.GetSessionVars().SetSystemVar(variable.MaxAllowedPacket, "67108864") defer s.cleanEnv(c) tk := testkit.NewTestKit(c, s.store) tk.MustExec("use test") diff --git a/planner/core/exhaust_physical_plans_test.go b/planner/core/exhaust_physical_plans_test.go index a7cf2a21a9a1f..38013996d0c84 100644 --- a/planner/core/exhaust_physical_plans_test.go +++ b/planner/core/exhaust_physical_plans_test.go @@ -21,6 +21,7 @@ import ( "github.com/pingcap/parser/model" "github.com/pingcap/parser/mysql" "github.com/pingcap/tidb/expression" + "github.com/pingcap/tidb/sessionctx/variable" "github.com/pingcap/tidb/types" ) @@ -40,6 +41,7 @@ func (s *testUnitTestSuit) rewriteSimpleExpr(str string, schema *expression.Sche func (s *testUnitTestSuit) TestIndexJoinAnalyzeLookUpFilters(c *C) { s.ctx.GetSessionVars().PlanID = -1 + s.ctx.GetSessionVars().SetSystemVar(variable.MaxAllowedPacket, "67108864") joinNode := LogicalJoin{}.Init(s.ctx) dataSourceNode := DataSource{}.Init(s.ctx) dsSchema := expression.NewSchema() diff --git a/planner/core/logical_plan_test.go b/planner/core/logical_plan_test.go index 683f2cdbe5d00..7dd0fa4714625 100644 --- a/planner/core/logical_plan_test.go +++ b/planner/core/logical_plan_test.go @@ -29,6 +29,7 @@ import ( "github.com/pingcap/tidb/infoschema" "github.com/pingcap/tidb/planner/property" "github.com/pingcap/tidb/sessionctx" + "github.com/pingcap/tidb/sessionctx/variable" "github.com/pingcap/tidb/util/testleak" ) @@ -1360,6 +1361,7 @@ func (s *testPlanSuite) TestValidate(c *C) { err: ErrUnknownColumn, }, } + s.ctx.GetSessionVars().SetSystemVar(variable.MaxAllowedPacket, "67108864") for _, tt := range tests { sql := tt.sql comment := Commentf("for %s", sql) From efdb40b97d3ea0873d0d7e3512062bd1698a37cf Mon Sep 17 00:00:00 2001 From: amyangfei Date: Fri, 12 Jul 2019 11:24:56 +0800 Subject: [PATCH 04/13] pre check max allowed packet to avoid memory allocation --- expression/builtin_string.go | 26 ++++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/expression/builtin_string.go b/expression/builtin_string.go index 60b3557e8b256..f5b6523fc1cd9 100644 --- a/expression/builtin_string.go +++ b/expression/builtin_string.go @@ -300,17 +300,19 @@ func (b *builtinConcatSig) Clone() builtinFunc { // See https://dev.mysql.com/doc/refman/5.7/en/string-functions.html#function_concat func (b *builtinConcatSig) evalString(row chunk.Row) (d string, isNull bool, err error) { var s []byte + var targetLength int for _, a := range b.getArgs() { d, isNull, err = a.EvalString(b.ctx, row) if isNull || err != nil { return d, isNull, err } + targetLength += len(d) + if uint64(targetLength) > b.maxAllowedPacket { + b.ctx.GetSessionVars().StmtCtx.AppendWarning(errWarnAllowedPacketOverflowed.GenWithStackByArgs("concat", b.maxAllowedPacket)) + return "", true, nil + } s = append(s, []byte(d)...) } - if uint64(len(s)) > b.maxAllowedPacket { - b.ctx.GetSessionVars().StmtCtx.AppendWarning(errWarnAllowedPacketOverflowed.GenWithStackByArgs("concat", b.maxAllowedPacket)) - return "", true, nil - } return string(s), false, nil } @@ -379,6 +381,7 @@ func (b *builtinConcatWSSig) evalString(row chunk.Row) (string, bool, error) { args := b.getArgs() strs := make([]string, 0, len(args)) var sep string + var targetLength int for i, arg := range args { val, isNull, err := arg.EvalString(b.ctx, row) if err != nil { @@ -399,16 +402,19 @@ func (b *builtinConcatWSSig) evalString(row chunk.Row) (string, bool, error) { sep = val continue } + targetLength += len(val) + if i > 1 { + targetLength += len(sep) + } + if uint64(targetLength) > b.maxAllowedPacket { + b.ctx.GetSessionVars().StmtCtx.AppendWarning(errWarnAllowedPacketOverflowed.GenWithStackByArgs("concat_ws", b.maxAllowedPacket)) + return "", true, nil + } strs = append(strs, val) } - result := strings.Join(strs, sep) - if uint64(len(result)) > b.maxAllowedPacket { - b.ctx.GetSessionVars().StmtCtx.AppendWarning(errWarnAllowedPacketOverflowed.GenWithStackByArgs("concat_ws", b.maxAllowedPacket)) - return "", true, nil - } // TODO: check whether the length of result is larger than Flen - return result, false, nil + return strings.Join(strs, sep), false, nil } type leftFunctionClass struct { From 321e7162e96f5048c4719d2e53adc7cdace75075 Mon Sep 17 00:00:00 2001 From: amyangfei Date: Tue, 16 Jul 2019 09:53:09 +0800 Subject: [PATCH 05/13] address comment --- expression/builtin_string.go | 4 +--- expression/integration_test.go | 1 - planner/core/exhaust_physical_plans_test.go | 2 -- planner/core/mock.go | 2 ++ util/mock/context.go | 1 + 5 files changed, 4 insertions(+), 6 deletions(-) diff --git a/expression/builtin_string.go b/expression/builtin_string.go index f5b6523fc1cd9..129b6277433a9 100644 --- a/expression/builtin_string.go +++ b/expression/builtin_string.go @@ -300,14 +300,12 @@ func (b *builtinConcatSig) Clone() builtinFunc { // See https://dev.mysql.com/doc/refman/5.7/en/string-functions.html#function_concat func (b *builtinConcatSig) evalString(row chunk.Row) (d string, isNull bool, err error) { var s []byte - var targetLength int for _, a := range b.getArgs() { d, isNull, err = a.EvalString(b.ctx, row) if isNull || err != nil { return d, isNull, err } - targetLength += len(d) - if uint64(targetLength) > b.maxAllowedPacket { + if uint64(len(s)+len(d)) > b.maxAllowedPacket { b.ctx.GetSessionVars().StmtCtx.AppendWarning(errWarnAllowedPacketOverflowed.GenWithStackByArgs("concat", b.maxAllowedPacket)) return "", true, nil } diff --git a/expression/integration_test.go b/expression/integration_test.go index 2dba788cb98a5..ff0ccb762251a 100644 --- a/expression/integration_test.go +++ b/expression/integration_test.go @@ -557,7 +557,6 @@ func (s *testIntegrationSuite) TestMathBuiltin(c *C) { } func (s *testIntegrationSuite) TestStringBuiltin(c *C) { - s.ctx.GetSessionVars().SetSystemVar(variable.MaxAllowedPacket, "67108864") defer s.cleanEnv(c) tk := testkit.NewTestKit(c, s.store) tk.MustExec("use test") diff --git a/planner/core/exhaust_physical_plans_test.go b/planner/core/exhaust_physical_plans_test.go index 38013996d0c84..a7cf2a21a9a1f 100644 --- a/planner/core/exhaust_physical_plans_test.go +++ b/planner/core/exhaust_physical_plans_test.go @@ -21,7 +21,6 @@ import ( "github.com/pingcap/parser/model" "github.com/pingcap/parser/mysql" "github.com/pingcap/tidb/expression" - "github.com/pingcap/tidb/sessionctx/variable" "github.com/pingcap/tidb/types" ) @@ -41,7 +40,6 @@ func (s *testUnitTestSuit) rewriteSimpleExpr(str string, schema *expression.Sche func (s *testUnitTestSuit) TestIndexJoinAnalyzeLookUpFilters(c *C) { s.ctx.GetSessionVars().PlanID = -1 - s.ctx.GetSessionVars().SetSystemVar(variable.MaxAllowedPacket, "67108864") joinNode := LogicalJoin{}.Init(s.ctx) dataSourceNode := DataSource{}.Init(s.ctx) dsSchema := expression.NewSchema() diff --git a/planner/core/mock.go b/planner/core/mock.go index e91146078a389..a6c3fda95d59f 100644 --- a/planner/core/mock.go +++ b/planner/core/mock.go @@ -20,6 +20,7 @@ import ( "github.com/pingcap/tidb/domain" "github.com/pingcap/tidb/infoschema" "github.com/pingcap/tidb/sessionctx" + "github.com/pingcap/tidb/sessionctx/variable" "github.com/pingcap/tidb/types" "github.com/pingcap/tidb/util/mock" ) @@ -300,6 +301,7 @@ func MockContext() sessionctx.Context { Client: &mock.Client{}, } ctx.GetSessionVars().CurrentDB = "test" + ctx.GetSessionVars().SetSystemVar(variable.MaxAllowedPacket, "67108864") do := &domain.Domain{} do.CreateStatsHandle(ctx) domain.BindDomain(ctx, do) diff --git a/util/mock/context.go b/util/mock/context.go index c3419792ac857..74e7ea1601ca8 100644 --- a/util/mock/context.go +++ b/util/mock/context.go @@ -228,6 +228,7 @@ func NewContext() *Context { sctx.sessionVars.MaxChunkSize = 32 sctx.sessionVars.StmtCtx.TimeZone = time.UTC sctx.sessionVars.GlobalVarsAccessor = variable.NewMockGlobalAccessor() + sctx.sessionVars.SetSystemVar(variable.MaxAllowedPacket, "67108864") return sctx } From f0421ccf26c5cf6386ce5561dd38d97409b94e1d Mon Sep 17 00:00:00 2001 From: amyangfei Date: Tue, 16 Jul 2019 10:09:45 +0800 Subject: [PATCH 06/13] fix error check --- expression/integration_test.go | 1 + planner/core/exhaust_physical_plans_test.go | 2 ++ planner/core/mock.go | 2 -- util/mock/context.go | 2 -- 4 files changed, 3 insertions(+), 4 deletions(-) diff --git a/expression/integration_test.go b/expression/integration_test.go index ff0ccb762251a..1ad9349cec85e 100644 --- a/expression/integration_test.go +++ b/expression/integration_test.go @@ -557,6 +557,7 @@ func (s *testIntegrationSuite) TestMathBuiltin(c *C) { } func (s *testIntegrationSuite) TestStringBuiltin(c *C) { + c.Assert(s.ctx.GetSessionVars().SetSystemVar(variable.MaxAllowedPacket, "67108864"), IsNil) defer s.cleanEnv(c) tk := testkit.NewTestKit(c, s.store) tk.MustExec("use test") diff --git a/planner/core/exhaust_physical_plans_test.go b/planner/core/exhaust_physical_plans_test.go index a7cf2a21a9a1f..127f00ca0f50c 100644 --- a/planner/core/exhaust_physical_plans_test.go +++ b/planner/core/exhaust_physical_plans_test.go @@ -21,6 +21,7 @@ import ( "github.com/pingcap/parser/model" "github.com/pingcap/parser/mysql" "github.com/pingcap/tidb/expression" + "github.com/pingcap/tidb/sessionctx/variable" "github.com/pingcap/tidb/types" ) @@ -40,6 +41,7 @@ func (s *testUnitTestSuit) rewriteSimpleExpr(str string, schema *expression.Sche func (s *testUnitTestSuit) TestIndexJoinAnalyzeLookUpFilters(c *C) { s.ctx.GetSessionVars().PlanID = -1 + c.Assert(s.ctx.GetSessionVars().SetSystemVar(variable.MaxAllowedPacket, "67108864"), IsNil) joinNode := LogicalJoin{}.Init(s.ctx) dataSourceNode := DataSource{}.Init(s.ctx) dsSchema := expression.NewSchema() diff --git a/planner/core/mock.go b/planner/core/mock.go index a6c3fda95d59f..e91146078a389 100644 --- a/planner/core/mock.go +++ b/planner/core/mock.go @@ -20,7 +20,6 @@ import ( "github.com/pingcap/tidb/domain" "github.com/pingcap/tidb/infoschema" "github.com/pingcap/tidb/sessionctx" - "github.com/pingcap/tidb/sessionctx/variable" "github.com/pingcap/tidb/types" "github.com/pingcap/tidb/util/mock" ) @@ -301,7 +300,6 @@ func MockContext() sessionctx.Context { Client: &mock.Client{}, } ctx.GetSessionVars().CurrentDB = "test" - ctx.GetSessionVars().SetSystemVar(variable.MaxAllowedPacket, "67108864") do := &domain.Domain{} do.CreateStatsHandle(ctx) domain.BindDomain(ctx, do) diff --git a/util/mock/context.go b/util/mock/context.go index 74e7ea1601ca8..180e66a57d23e 100644 --- a/util/mock/context.go +++ b/util/mock/context.go @@ -24,7 +24,6 @@ import ( "github.com/pingcap/tidb/kv" "github.com/pingcap/tidb/owner" "github.com/pingcap/tidb/sessionctx" - "github.com/pingcap/tidb/sessionctx/variable" "github.com/pingcap/tidb/types" "github.com/pingcap/tidb/util" "github.com/pingcap/tidb/util/kvcache" @@ -228,7 +227,6 @@ func NewContext() *Context { sctx.sessionVars.MaxChunkSize = 32 sctx.sessionVars.StmtCtx.TimeZone = time.UTC sctx.sessionVars.GlobalVarsAccessor = variable.NewMockGlobalAccessor() - sctx.sessionVars.SetSystemVar(variable.MaxAllowedPacket, "67108864") return sctx } From c59e35fa0d262415ed206cc3e95e77896b8c244f Mon Sep 17 00:00:00 2001 From: amyangfei Date: Tue, 16 Jul 2019 10:12:37 +0800 Subject: [PATCH 07/13] fix ci --- util/mock/context.go | 1 + 1 file changed, 1 insertion(+) diff --git a/util/mock/context.go b/util/mock/context.go index 180e66a57d23e..c3419792ac857 100644 --- a/util/mock/context.go +++ b/util/mock/context.go @@ -24,6 +24,7 @@ import ( "github.com/pingcap/tidb/kv" "github.com/pingcap/tidb/owner" "github.com/pingcap/tidb/sessionctx" + "github.com/pingcap/tidb/sessionctx/variable" "github.com/pingcap/tidb/types" "github.com/pingcap/tidb/util" "github.com/pingcap/tidb/util/kvcache" From 89d726065f59759e9dd429892fd4da0063e4fb81 Mon Sep 17 00:00:00 2001 From: amyangfei Date: Tue, 16 Jul 2019 14:06:56 +0800 Subject: [PATCH 08/13] address comment, set max allowed packet in mock context --- expression/builtin_string.go | 26 ++++++++++++--------- planner/core/exhaust_physical_plans_test.go | 2 -- planner/core/mock.go | 3 +++ util/mock/context.go | 2 ++ 4 files changed, 20 insertions(+), 13 deletions(-) diff --git a/expression/builtin_string.go b/expression/builtin_string.go index 129b6277433a9..4c4c73dc33610 100644 --- a/expression/builtin_string.go +++ b/expression/builtin_string.go @@ -380,26 +380,30 @@ func (b *builtinConcatWSSig) evalString(row chunk.Row) (string, bool, error) { strs := make([]string, 0, len(args)) var sep string var targetLength int - for i, arg := range args { - val, isNull, err := arg.EvalString(b.ctx, row) + + N := len(args) + if N > 0 { + val, isNull, err := args[0].EvalString(b.ctx, row) + if err != nil { + return val, isNull, err + } + // If the separator is NULL, the result is NULL. + if isNull { + return val, isNull, nil + } + sep = val + } + for i := 1; i < N; i++ { + val, isNull, err := args[i].EvalString(b.ctx, row) if err != nil { return val, isNull, err } - if isNull { - // If the separator is NULL, the result is NULL. - if i == 0 { - return val, isNull, nil - } // CONCAT_WS() does not skip empty strings. However, // it does skip any NULL values after the separator argument. continue } - if i == 0 { - sep = val - continue - } targetLength += len(val) if i > 1 { targetLength += len(sep) diff --git a/planner/core/exhaust_physical_plans_test.go b/planner/core/exhaust_physical_plans_test.go index 127f00ca0f50c..a7cf2a21a9a1f 100644 --- a/planner/core/exhaust_physical_plans_test.go +++ b/planner/core/exhaust_physical_plans_test.go @@ -21,7 +21,6 @@ import ( "github.com/pingcap/parser/model" "github.com/pingcap/parser/mysql" "github.com/pingcap/tidb/expression" - "github.com/pingcap/tidb/sessionctx/variable" "github.com/pingcap/tidb/types" ) @@ -41,7 +40,6 @@ func (s *testUnitTestSuit) rewriteSimpleExpr(str string, schema *expression.Sche func (s *testUnitTestSuit) TestIndexJoinAnalyzeLookUpFilters(c *C) { s.ctx.GetSessionVars().PlanID = -1 - c.Assert(s.ctx.GetSessionVars().SetSystemVar(variable.MaxAllowedPacket, "67108864"), IsNil) joinNode := LogicalJoin{}.Init(s.ctx) dataSourceNode := DataSource{}.Init(s.ctx) dsSchema := expression.NewSchema() diff --git a/planner/core/mock.go b/planner/core/mock.go index e91146078a389..49f0b52ab686a 100644 --- a/planner/core/mock.go +++ b/planner/core/mock.go @@ -20,6 +20,7 @@ import ( "github.com/pingcap/tidb/domain" "github.com/pingcap/tidb/infoschema" "github.com/pingcap/tidb/sessionctx" + "github.com/pingcap/tidb/sessionctx/variable" "github.com/pingcap/tidb/types" "github.com/pingcap/tidb/util/mock" ) @@ -303,6 +304,8 @@ func MockContext() sessionctx.Context { do := &domain.Domain{} do.CreateStatsHandle(ctx) domain.BindDomain(ctx, do) + // set MaxAllowedPacket always returns nil, assign this error to avoid errcheck warning + _ = ctx.GetSessionVars().SetSystemVar(variable.MaxAllowedPacket, "67108864") return ctx } diff --git a/util/mock/context.go b/util/mock/context.go index c3419792ac857..2584183a416b0 100644 --- a/util/mock/context.go +++ b/util/mock/context.go @@ -228,6 +228,8 @@ func NewContext() *Context { sctx.sessionVars.MaxChunkSize = 32 sctx.sessionVars.StmtCtx.TimeZone = time.UTC sctx.sessionVars.GlobalVarsAccessor = variable.NewMockGlobalAccessor() + // set MaxAllowedPacket always returns nil, assign this error to avoid errcheck warning + _ = sctx.GetSessionVars().SetSystemVar(variable.MaxAllowedPacket, "67108864") return sctx } From 49966ae0d58524b3e1c03d4ae3fcbf48a95235c7 Mon Sep 17 00:00:00 2001 From: amyangfei Date: Tue, 16 Jul 2019 14:17:13 +0800 Subject: [PATCH 09/13] fix errcheck warning --- planner/core/mock.go | 3 ++- util/mock/context.go | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/planner/core/mock.go b/planner/core/mock.go index 49f0b52ab686a..be93adc5eb240 100644 --- a/planner/core/mock.go +++ b/planner/core/mock.go @@ -305,7 +305,8 @@ func MockContext() sessionctx.Context { do.CreateStatsHandle(ctx) domain.BindDomain(ctx, do) // set MaxAllowedPacket always returns nil, assign this error to avoid errcheck warning - _ = ctx.GetSessionVars().SetSystemVar(variable.MaxAllowedPacket, "67108864") + err := ctx.GetSessionVars().SetSystemVar(variable.MaxAllowedPacket, "67108864") + _ = err return ctx } diff --git a/util/mock/context.go b/util/mock/context.go index 2584183a416b0..f5c56a9237db4 100644 --- a/util/mock/context.go +++ b/util/mock/context.go @@ -229,7 +229,8 @@ func NewContext() *Context { sctx.sessionVars.StmtCtx.TimeZone = time.UTC sctx.sessionVars.GlobalVarsAccessor = variable.NewMockGlobalAccessor() // set MaxAllowedPacket always returns nil, assign this error to avoid errcheck warning - _ = sctx.GetSessionVars().SetSystemVar(variable.MaxAllowedPacket, "67108864") + err := sctx.GetSessionVars().SetSystemVar(variable.MaxAllowedPacket, "67108864") + _ = err return sctx } From 4f236a33be96a94011aeff58ff693c11d8882b92 Mon Sep 17 00:00:00 2001 From: amyangfei Date: Tue, 16 Jul 2019 14:30:32 +0800 Subject: [PATCH 10/13] remove duplicated max allowed packet setter --- expression/integration_test.go | 1 - planner/core/logical_plan_test.go | 2 -- 2 files changed, 3 deletions(-) diff --git a/expression/integration_test.go b/expression/integration_test.go index 1ad9349cec85e..ff0ccb762251a 100644 --- a/expression/integration_test.go +++ b/expression/integration_test.go @@ -557,7 +557,6 @@ func (s *testIntegrationSuite) TestMathBuiltin(c *C) { } func (s *testIntegrationSuite) TestStringBuiltin(c *C) { - c.Assert(s.ctx.GetSessionVars().SetSystemVar(variable.MaxAllowedPacket, "67108864"), IsNil) defer s.cleanEnv(c) tk := testkit.NewTestKit(c, s.store) tk.MustExec("use test") diff --git a/planner/core/logical_plan_test.go b/planner/core/logical_plan_test.go index 7dd0fa4714625..683f2cdbe5d00 100644 --- a/planner/core/logical_plan_test.go +++ b/planner/core/logical_plan_test.go @@ -29,7 +29,6 @@ import ( "github.com/pingcap/tidb/infoschema" "github.com/pingcap/tidb/planner/property" "github.com/pingcap/tidb/sessionctx" - "github.com/pingcap/tidb/sessionctx/variable" "github.com/pingcap/tidb/util/testleak" ) @@ -1361,7 +1360,6 @@ func (s *testPlanSuite) TestValidate(c *C) { err: ErrUnknownColumn, }, } - s.ctx.GetSessionVars().SetSystemVar(variable.MaxAllowedPacket, "67108864") for _, tt := range tests { sql := tt.sql comment := Commentf("for %s", sql) From f0a3edc1ea22c958520fe34260fda1ed8600d4a5 Mon Sep 17 00:00:00 2001 From: amyangfei Date: Tue, 16 Jul 2019 14:39:01 +0800 Subject: [PATCH 11/13] panic if SetSystemVar returns error --- planner/core/mock.go | 6 +++--- util/mock/context.go | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/planner/core/mock.go b/planner/core/mock.go index be93adc5eb240..58d3af912a9fc 100644 --- a/planner/core/mock.go +++ b/planner/core/mock.go @@ -304,9 +304,9 @@ func MockContext() sessionctx.Context { do := &domain.Domain{} do.CreateStatsHandle(ctx) domain.BindDomain(ctx, do) - // set MaxAllowedPacket always returns nil, assign this error to avoid errcheck warning - err := ctx.GetSessionVars().SetSystemVar(variable.MaxAllowedPacket, "67108864") - _ = err + if err := ctx.GetSessionVars().SetSystemVar(variable.MaxAllowedPacket, "67108864"); err != nil { + panic(err) + } return ctx } diff --git a/util/mock/context.go b/util/mock/context.go index f5c56a9237db4..a7b76b282baf3 100644 --- a/util/mock/context.go +++ b/util/mock/context.go @@ -228,9 +228,9 @@ func NewContext() *Context { sctx.sessionVars.MaxChunkSize = 32 sctx.sessionVars.StmtCtx.TimeZone = time.UTC sctx.sessionVars.GlobalVarsAccessor = variable.NewMockGlobalAccessor() - // set MaxAllowedPacket always returns nil, assign this error to avoid errcheck warning - err := sctx.GetSessionVars().SetSystemVar(variable.MaxAllowedPacket, "67108864") - _ = err + if err := sctx.GetSessionVars().SetSystemVar(variable.MaxAllowedPacket, "67108864"); err != nil { + panic(err) + } return sctx } From 48a233d840778a842188c677e310fcea80d81c17 Mon Sep 17 00:00:00 2001 From: amyangfei Date: Tue, 16 Jul 2019 14:56:24 +0800 Subject: [PATCH 12/13] remove duplicated set variable --- planner/core/mock.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/planner/core/mock.go b/planner/core/mock.go index 58d3af912a9fc..e91146078a389 100644 --- a/planner/core/mock.go +++ b/planner/core/mock.go @@ -20,7 +20,6 @@ import ( "github.com/pingcap/tidb/domain" "github.com/pingcap/tidb/infoschema" "github.com/pingcap/tidb/sessionctx" - "github.com/pingcap/tidb/sessionctx/variable" "github.com/pingcap/tidb/types" "github.com/pingcap/tidb/util/mock" ) @@ -304,9 +303,6 @@ func MockContext() sessionctx.Context { do := &domain.Domain{} do.CreateStatsHandle(ctx) domain.BindDomain(ctx, do) - if err := ctx.GetSessionVars().SetSystemVar(variable.MaxAllowedPacket, "67108864"); err != nil { - panic(err) - } return ctx } From 6d7da174f07927d0ac6a7f6f7e74fab9f515c94a Mon Sep 17 00:00:00 2001 From: amyangfei Date: Tue, 16 Jul 2019 15:09:15 +0800 Subject: [PATCH 13/13] address comment --- expression/builtin_string.go | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/expression/builtin_string.go b/expression/builtin_string.go index 4c4c73dc33610..b8882bbcf4080 100644 --- a/expression/builtin_string.go +++ b/expression/builtin_string.go @@ -384,13 +384,10 @@ func (b *builtinConcatWSSig) evalString(row chunk.Row) (string, bool, error) { N := len(args) if N > 0 { val, isNull, err := args[0].EvalString(b.ctx, row) - if err != nil { + if err != nil || isNull { + // If the separator is NULL, the result is NULL. return val, isNull, err } - // If the separator is NULL, the result is NULL. - if isNull { - return val, isNull, nil - } sep = val } for i := 1; i < N; i++ {