From d98c59b9bdfb2d8502d880b1552f0a4a183990ae Mon Sep 17 00:00:00 2001 From: Guo Song Date: Fri, 8 Mar 2019 09:10:28 +0800 Subject: [PATCH 01/10] add json_array_append --- expression/builtin_json.go | 86 ++++++++++++++++++++++++++++++++- expression/builtin_json_test.go | 65 +++++++++++++++++++++++++ expression/distsql_builtin.go | 2 + 3 files changed, 152 insertions(+), 1 deletion(-) diff --git a/expression/builtin_json.go b/expression/builtin_json.go index d3a6c39e86e7c..28173b767db25 100644 --- a/expression/builtin_json.go +++ b/expression/builtin_json.go @@ -54,6 +54,7 @@ var ( _ builtinFunc = &builtinJSONQuoteSig{} _ builtinFunc = &builtinJSONUnquoteSig{} _ builtinFunc = &builtinJSONArraySig{} + _ builtinFunc = &builtinJSONArrayAppendSig{} _ builtinFunc = &builtinJSONObjectSig{} _ builtinFunc = &builtinJSONExtractSig{} _ builtinFunc = &builtinJSONSetSig{} @@ -702,8 +703,91 @@ type jsonArrayAppendFunctionClass struct { baseFunctionClass } +type builtinJSONArrayAppendSig struct { + baseBuiltinFunc +} + func (c *jsonArrayAppendFunctionClass) getFunction(ctx sessionctx.Context, args []Expression) (builtinFunc, error) { - return nil, errFunctionNotExists.GenWithStackByArgs("FUNCTION", "JSON_ARRAY_APPEND") + if err := c.verifyArgs(args); err != nil { + return nil, err + } + if len(args)&1 != 1 { + return nil, ErrIncorrectParameterCount.GenWithStackByArgs(c.funcName) + } + argTps := make([]types.EvalType, 0, len(args)) + argTps = append(argTps, types.ETJson) + for i := 1; i < len(args)-1; i += 2 { + argTps = append(argTps, types.ETString, types.ETJson) + } + bf := newBaseBuiltinFuncWithTp(ctx, args, types.ETJson, argTps...) + for i := 2; i < len(args); i += 2 { + DisableParseJSONFlag4Expr(args[i]) + } + sig := &builtinJSONArrayAppendSig{bf} + sig.setPbCode(tipb.ScalarFuncSig_JsonArrayAppendSig) + return sig, nil +} + +func (b *builtinJSONArrayAppendSig) Clone() builtinFunc { + newSig := &builtinJSONArrayAppendSig{} + newSig.cloneFrom(&b.baseBuiltinFunc) + return newSig +} + +func (b *builtinJSONArrayAppendSig) evalJSON(row chunk.Row) (res json.BinaryJSON, isNull bool, err error) { + res, isNull, err = b.args[0].EvalJSON(b.ctx, row) + if isNull || err != nil { + // bad json will only get nil in mysql + // select JSON_ARRAY_APPEND('asdf', "$", NULL); + return res, true, nil + } + + for i := 1; i < len(b.args)-1; i += 2 { + // bad arguments should return null + // NOTE: mysql does not report error + // also they does not ignore it, they just return NULL + s, isNull, err := b.args[i].EvalString(b.ctx, row) + if isNull || err != nil { + return res, isNull, nil + } + pathExpr, err := json.ParseJSONPathExpr(s) + if err != nil { + return res, true, nil + } + if pathExpr.ContainsAnyAsterisk() { + return res, true, nil + } + + var exists bool + obj, exists := res.Extract([]json.PathExpression{pathExpr}) + if !exists { + // just do nothing for this + continue + } + + if obj.TypeCode != json.TypeCodeArray { + // JSON_ARRAY_APPEND({"a": "b"}, "$", "c") => + // [{"a": "b"}, "c"] + // We should convert them to a single array first + obj = json.CreateBinary([]interface{}{obj}) + } + + value, isnull, err := b.args[i+1].EvalJSON(b.ctx, row) + if err != nil { + return res, true, err + } + + if isnull { + value = json.CreateBinary(nil) + } + + obj = json.MergeBinary([]json.BinaryJSON{obj, value}) + res, err = res.Modify([]json.PathExpression{pathExpr}, []json.BinaryJSON{obj}, json.ModifySet) + } + if err != nil { + return res, true, err + } + return res, isNull, err } type jsonArrayInsertFunctionClass struct { diff --git a/expression/builtin_json_test.go b/expression/builtin_json_test.go index 58d7712aa1064..7449d8fef21ad 100644 --- a/expression/builtin_json_test.go +++ b/expression/builtin_json_test.go @@ -686,3 +686,68 @@ func (s *testEvaluatorSuite) TestJSONDepth(c *C) { } } } + +func (s *testEvaluatorSuite) TestJSONArrayAppend(c *C) { + defer testleak.AfterTest(c)() + sampleJSON, err := json.ParseBinaryFromString(`{"b": 2}`) + c.Assert(err, IsNil) + fc := funcs[ast.JSONArrayAppend] + tbl := []struct { + input []interface{} + expected interface{} + success bool + }{ + {[]interface{}{`{"a": 1, "b": [2, 3], "c": 4}`, `$.d`, `z`}, `{"a": 1, "b": [2, 3], "c": 4}`, true}, + {[]interface{}{`{"a": 1, "b": [2, 3], "c": 4}`, `$`, `w`}, `[{"a": 1, "b": [2, 3], "c": 4}, "w"]`, true}, + {[]interface{}{`{"a": 1, "b": [2, 3], "c": 4}`, `$`, nil}, `[{"a": 1, "b": [2, 3], "c": 4}, null]`, true}, + {[]interface{}{`{"a": 1}`, `$`, `{"b": 2}`}, `[{"a": 1}, "{\"b\": 2}"]`, true}, + {[]interface{}{`{"a": 1}`, `$`, sampleJSON}, `[{"a": 1}, {"b": 2}]`, true}, + {[]interface{}{`{"a": 1}`, `$.a`, sampleJSON}, `{"a": [1, {"b": 2}]}`, true}, + // explains hy call res.Modify every path-value pair and not do as JSON_SET do? + {[]interface{}{`{"a": 1}`, `$.a`, sampleJSON, `$.a[1]`, sampleJSON}, `{"a": [1, [{"b": 2}, {"b": 2}]]}`, true}, + {[]interface{}{nil, `$`, nil}, nil, true}, + // bad arguments + {[]interface{}{`{"a": 1, "b": [2, 3], "c": 4}`, nil, nil}, nil, true}, + {[]interface{}{`{"a": 1, "b": [2, 3], "c": 4}`, `asdf`, nil}, nil, true}, + {[]interface{}{`{"a": 1, "b": [2, 3], "c": 4}`, `$.*`, nil}, nil, true}, + {[]interface{}{`asdf`, `$`, nil}, nil, true}, + {[]interface{}{``, `$`, nil}, nil, true}, + {[]interface{}{`[]`, `$`, nil}, `[null]`, true}, + {[]interface{}{`{}`, `$`, nil}, `[{}, null]`, true}, + // following tests comes from MySQL doc + {[]interface{}{`["a", ["b", "c"], "d"]`, `$[1]`, 1}, `["a", ["b", "c", 1], "d"]`, true}, + {[]interface{}{`["a", ["b", "c"], "d"]`, `$[0]`, 2}, `[["a", 2], ["b", "c"], "d"]`, true}, + {[]interface{}{`["a", ["b", "c"], "d"]`, `$[1][0]`, 3}, `["a", [["b", 3], "c"], "d"]`, true}, + {[]interface{}{`{"a": 1, "b": [2, 3], "c": 4}`, `$.b`, `x`}, `{"a": 1, "b": [2, 3, "x"], "c": 4}`, true}, + {[]interface{}{`{"a": 1, "b": [2, 3], "c": 4}`, `$.c`, `y`}, `{"a": 1, "b": [2, 3], "c": [4, "y"]}`, true}, + // following tests comes from MySQL test + {[]interface{}{`[1,2,3, {"a":[4,5,6]}]`, `$`, 7}, `[1, 2, 3, {"a": [4, 5, 6]}, 7]`, true}, + {[]interface{}{`[1,2,3, {"a":[4,5,6]}]`, `$`, 7, `$[3].a`, 3.14}, `[1, 2, 3, {"a": [4, 5, 6, 3.14]}, 7]`, true}, + {[]interface{}{`[1,2,3, {"a":[4,5,6]}]`, `$`, 7, `$[3].b`, 8}, `[1, 2, 3, {"a": [4, 5, 6]}, 7]`, true}, + } + for i, t := range tbl { + args := types.MakeDatums(t.input...) + f, err := fc.getFunction(s.ctx, s.datumsToConstants(args)) + c.Assert(err, IsNil) + d, err := evalBuiltinFunc(f, chunk.Row{}) + comment := Commentf("case:%v \n input:%v \n output: %s \n expected: %v", i, t.input, d.GetString(), t.expected) + if t.success { + c.Assert(err, IsNil, comment) + + if t.expected == nil { + c.Assert(d.IsNull(), IsTrue, comment) + } else { + var j1 json.BinaryJSON + j1, err = json.ParseBinaryFromString(t.expected.(string)) + c.Assert(err, IsNil) + j2 := d.GetMysqlJSON() + var cmp int + cmp = json.CompareBinary(j1, j2) + c.Assert(err, IsNil, comment) + c.Assert(cmp, Equals, 0, comment) + } + } else { + c.Assert(err, NotNil, comment) + } + } +} diff --git a/expression/distsql_builtin.go b/expression/distsql_builtin.go index e0c5b659c4e54..c47afe4a4c924 100644 --- a/expression/distsql_builtin.go +++ b/expression/distsql_builtin.go @@ -420,6 +420,8 @@ func getSignatureByPB(ctx sessionctx.Context, sigCode tipb.ScalarFuncSig, tp *ti f = &builtinJSONUnquoteSig{base} case tipb.ScalarFuncSig_JsonArraySig: f = &builtinJSONArraySig{base} + case tipb.ScalarFuncSig_JsonArrayAppendSig: + f = &builtinJSONArrayAppendSig{base} case tipb.ScalarFuncSig_JsonObjectSig: f = &builtinJSONObjectSig{base} case tipb.ScalarFuncSig_JsonExtractSig: From 479da312bf227c12d56f951a34d6315d5f900ce2 Mon Sep 17 00:00:00 2001 From: Guo Song Date: Fri, 8 Mar 2019 09:23:28 +0800 Subject: [PATCH 02/10] remove unnecessary error checks --- expression/builtin_json.go | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/expression/builtin_json.go b/expression/builtin_json.go index 28173b767db25..487269f783c9b 100644 --- a/expression/builtin_json.go +++ b/expression/builtin_json.go @@ -782,12 +782,10 @@ func (b *builtinJSONArrayAppendSig) evalJSON(row chunk.Row) (res json.BinaryJSON } obj = json.MergeBinary([]json.BinaryJSON{obj, value}) - res, err = res.Modify([]json.PathExpression{pathExpr}, []json.BinaryJSON{obj}, json.ModifySet) + res, _ = res.Modify([]json.PathExpression{pathExpr}, []json.BinaryJSON{obj}, json.ModifySet) + // we have done same checks where res.Modify might return with error before, thus ignore it } - if err != nil { - return res, true, err - } - return res, isNull, err + return res, isNull, nil } type jsonArrayInsertFunctionClass struct { From ef1a24d9570209eb1631a71c4afd2a0921573ce4 Mon Sep 17 00:00:00 2001 From: Guo Song Date: Fri, 8 Mar 2019 10:46:36 +0800 Subject: [PATCH 03/10] fix ci, append warnings instead of just ignore them --- expression/builtin_json.go | 22 +++++++++---- expression/builtin_json_test.go | 58 +++++++++++++++++++-------------- 2 files changed, 50 insertions(+), 30 deletions(-) diff --git a/expression/builtin_json.go b/expression/builtin_json.go index 487269f783c9b..47cbe0538eb5a 100644 --- a/expression/builtin_json.go +++ b/expression/builtin_json.go @@ -738,7 +738,7 @@ func (b *builtinJSONArrayAppendSig) evalJSON(row chunk.Row) (res json.BinaryJSON res, isNull, err = b.args[0].EvalJSON(b.ctx, row) if isNull || err != nil { // bad json will only get nil in mysql - // select JSON_ARRAY_APPEND('asdf', "$", NULL); + // select JSON_ARRAY_APPEND('asdf', "$", NULL);show warnings; return res, true, nil } @@ -748,13 +748,16 @@ func (b *builtinJSONArrayAppendSig) evalJSON(row chunk.Row) (res json.BinaryJSON // also they does not ignore it, they just return NULL s, isNull, err := b.args[i].EvalString(b.ctx, row) if isNull || err != nil { + b.ctx.GetSessionVars().StmtCtx.AppendWarning(json.ErrInvalidJSONPath.FastGen("Syntax error in JSON path in argument %d to function 'json_array_append'", i+1)) return res, isNull, nil } pathExpr, err := json.ParseJSONPathExpr(s) if err != nil { + b.ctx.GetSessionVars().StmtCtx.AppendWarning(json.ErrInvalidJSONPath.FastGen("Syntax error in JSON path in argument %d to function 'json_array_append'", i+1)) return res, true, nil } if pathExpr.ContainsAnyAsterisk() { + b.ctx.GetSessionVars().StmtCtx.AppendWarning(json.ErrInvalidJSONPath.FastGen("Wildcard path expression is not allowed in function 'json_array_append' at position %d", i+1)) return res, true, nil } @@ -766,13 +769,17 @@ func (b *builtinJSONArrayAppendSig) evalJSON(row chunk.Row) (res json.BinaryJSON } if obj.TypeCode != json.TypeCodeArray { - // JSON_ARRAY_APPEND({"a": "b"}, "$", "c") => - // [{"a": "b"}, "c"] + // JSON_ARRAY_APPEND({"a": "b"}, "$", "c") => [{"a": "b"}, "c"] // We should convert them to a single array first obj = json.CreateBinary([]interface{}{obj}) } - value, isnull, err := b.args[i+1].EvalJSON(b.ctx, row) + var ( + value json.BinaryJSON + isnull bool + ) + + value, isnull, err = b.args[i+1].EvalJSON(b.ctx, row) if err != nil { return res, true, err } @@ -782,10 +789,13 @@ func (b *builtinJSONArrayAppendSig) evalJSON(row chunk.Row) (res json.BinaryJSON } obj = json.MergeBinary([]json.BinaryJSON{obj, value}) - res, _ = res.Modify([]json.PathExpression{pathExpr}, []json.BinaryJSON{obj}, json.ModifySet) + res, err = res.Modify([]json.PathExpression{pathExpr}, []json.BinaryJSON{obj}, json.ModifySet) + if err != nil { + b.ctx.GetSessionVars().StmtCtx.AppendWarning(err) + } // we have done same checks where res.Modify might return with error before, thus ignore it } - return res, isNull, nil + return res, isNull, err } type jsonArrayInsertFunctionClass struct { diff --git a/expression/builtin_json_test.go b/expression/builtin_json_test.go index 7449d8fef21ad..f34af5ee12e72 100644 --- a/expression/builtin_json_test.go +++ b/expression/builtin_json_test.go @@ -696,44 +696,54 @@ func (s *testEvaluatorSuite) TestJSONArrayAppend(c *C) { input []interface{} expected interface{} success bool + warning bool }{ - {[]interface{}{`{"a": 1, "b": [2, 3], "c": 4}`, `$.d`, `z`}, `{"a": 1, "b": [2, 3], "c": 4}`, true}, - {[]interface{}{`{"a": 1, "b": [2, 3], "c": 4}`, `$`, `w`}, `[{"a": 1, "b": [2, 3], "c": 4}, "w"]`, true}, - {[]interface{}{`{"a": 1, "b": [2, 3], "c": 4}`, `$`, nil}, `[{"a": 1, "b": [2, 3], "c": 4}, null]`, true}, - {[]interface{}{`{"a": 1}`, `$`, `{"b": 2}`}, `[{"a": 1}, "{\"b\": 2}"]`, true}, - {[]interface{}{`{"a": 1}`, `$`, sampleJSON}, `[{"a": 1}, {"b": 2}]`, true}, - {[]interface{}{`{"a": 1}`, `$.a`, sampleJSON}, `{"a": [1, {"b": 2}]}`, true}, + {[]interface{}{`{"a": 1, "b": [2, 3], "c": 4}`, `$.d`, `z`}, `{"a": 1, "b": [2, 3], "c": 4}`, true, false}, + {[]interface{}{`{"a": 1, "b": [2, 3], "c": 4}`, `$`, `w`}, `[{"a": 1, "b": [2, 3], "c": 4}, "w"]`, true, false}, + {[]interface{}{`{"a": 1, "b": [2, 3], "c": 4}`, `$`, nil}, `[{"a": 1, "b": [2, 3], "c": 4}, null]`, true, false}, + {[]interface{}{`{"a": 1}`, `$`, `{"b": 2}`}, `[{"a": 1}, "{\"b\": 2}"]`, true, false}, + {[]interface{}{`{"a": 1}`, `$`, sampleJSON}, `[{"a": 1}, {"b": 2}]`, true, false}, + {[]interface{}{`{"a": 1}`, `$.a`, sampleJSON}, `{"a": [1, {"b": 2}]}`, true, false}, // explains hy call res.Modify every path-value pair and not do as JSON_SET do? - {[]interface{}{`{"a": 1}`, `$.a`, sampleJSON, `$.a[1]`, sampleJSON}, `{"a": [1, [{"b": 2}, {"b": 2}]]}`, true}, - {[]interface{}{nil, `$`, nil}, nil, true}, + {[]interface{}{`{"a": 1}`, `$.a`, sampleJSON, `$.a[1]`, sampleJSON}, `{"a": [1, [{"b": 2}, {"b": 2}]]}`, true, false}, + {[]interface{}{nil, `$`, nil}, nil, true, false}, // bad arguments - {[]interface{}{`{"a": 1, "b": [2, 3], "c": 4}`, nil, nil}, nil, true}, - {[]interface{}{`{"a": 1, "b": [2, 3], "c": 4}`, `asdf`, nil}, nil, true}, - {[]interface{}{`{"a": 1, "b": [2, 3], "c": 4}`, `$.*`, nil}, nil, true}, - {[]interface{}{`asdf`, `$`, nil}, nil, true}, - {[]interface{}{``, `$`, nil}, nil, true}, - {[]interface{}{`[]`, `$`, nil}, `[null]`, true}, - {[]interface{}{`{}`, `$`, nil}, `[{}, null]`, true}, + {[]interface{}{`asdf`, `$`, nil}, nil, true, false}, + {[]interface{}{``, `$`, nil}, nil, true, false}, + {[]interface{}{`[]`, `$`, nil}, `[null]`, true, false}, + {[]interface{}{`{}`, `$`, nil}, `[{}, null]`, true, false}, // following tests comes from MySQL doc - {[]interface{}{`["a", ["b", "c"], "d"]`, `$[1]`, 1}, `["a", ["b", "c", 1], "d"]`, true}, - {[]interface{}{`["a", ["b", "c"], "d"]`, `$[0]`, 2}, `[["a", 2], ["b", "c"], "d"]`, true}, - {[]interface{}{`["a", ["b", "c"], "d"]`, `$[1][0]`, 3}, `["a", [["b", 3], "c"], "d"]`, true}, - {[]interface{}{`{"a": 1, "b": [2, 3], "c": 4}`, `$.b`, `x`}, `{"a": 1, "b": [2, 3, "x"], "c": 4}`, true}, - {[]interface{}{`{"a": 1, "b": [2, 3], "c": 4}`, `$.c`, `y`}, `{"a": 1, "b": [2, 3], "c": [4, "y"]}`, true}, + {[]interface{}{`["a", ["b", "c"], "d"]`, `$[1]`, 1}, `["a", ["b", "c", 1], "d"]`, true, false}, + {[]interface{}{`["a", ["b", "c"], "d"]`, `$[0]`, 2}, `[["a", 2], ["b", "c"], "d"]`, true, false}, + {[]interface{}{`["a", ["b", "c"], "d"]`, `$[1][0]`, 3}, `["a", [["b", 3], "c"], "d"]`, true, false}, + {[]interface{}{`{"a": 1, "b": [2, 3], "c": 4}`, `$.b`, `x`}, `{"a": 1, "b": [2, 3, "x"], "c": 4}`, true, false}, + {[]interface{}{`{"a": 1, "b": [2, 3], "c": 4}`, `$.c`, `y`}, `{"a": 1, "b": [2, 3], "c": [4, "y"]}`, true, false}, // following tests comes from MySQL test - {[]interface{}{`[1,2,3, {"a":[4,5,6]}]`, `$`, 7}, `[1, 2, 3, {"a": [4, 5, 6]}, 7]`, true}, - {[]interface{}{`[1,2,3, {"a":[4,5,6]}]`, `$`, 7, `$[3].a`, 3.14}, `[1, 2, 3, {"a": [4, 5, 6, 3.14]}, 7]`, true}, - {[]interface{}{`[1,2,3, {"a":[4,5,6]}]`, `$`, 7, `$[3].b`, 8}, `[1, 2, 3, {"a": [4, 5, 6]}, 7]`, true}, + {[]interface{}{`[1,2,3, {"a":[4,5,6]}]`, `$`, 7}, `[1, 2, 3, {"a": [4, 5, 6]}, 7]`, true, false}, + {[]interface{}{`[1,2,3, {"a":[4,5,6]}]`, `$`, 7, `$[3].a`, 3.14}, `[1, 2, 3, {"a": [4, 5, 6, 3.14]}, 7]`, true, false}, + {[]interface{}{`[1,2,3, {"a":[4,5,6]}]`, `$`, 7, `$[3].b`, 8}, `[1, 2, 3, {"a": [4, 5, 6]}, 7]`, true, false}, + // warning instead of error for illegal params + {[]interface{}{`{"a": 1, "b": [2, 3], "c": 4}`, nil, nil}, nil, true, true}, + {[]interface{}{`{"a": 1, "b": [2, 3], "c": 4}`, `asdf`, nil}, nil, true, true}, + {[]interface{}{`{"a": 1, "b": [2, 3], "c": 4}`, 42, nil}, nil, true, true}, + {[]interface{}{`{"a": 1, "b": [2, 3], "c": 4}`, `$.*`, nil}, nil, true, true}, } for i, t := range tbl { args := types.MakeDatums(t.input...) + s.ctx.GetSessionVars().StmtCtx.SetWarnings(nil) f, err := fc.getFunction(s.ctx, s.datumsToConstants(args)) c.Assert(err, IsNil) d, err := evalBuiltinFunc(f, chunk.Row{}) - comment := Commentf("case:%v \n input:%v \n output: %s \n expected: %v", i, t.input, d.GetString(), t.expected) + comment := Commentf("case:%v \n input:%v \n output: %s \n expected: %v \n warnings: %v \n should return warnings? %v", i, t.input, d.GetMysqlJSON(), t.expected, s.ctx.GetSessionVars().StmtCtx.GetWarnings(), t.warning) if t.success { c.Assert(err, IsNil, comment) + if t.warning { + c.Assert(int(s.ctx.GetSessionVars().StmtCtx.WarningCount()), Greater, 0, comment) + } else { + c.Assert(int(s.ctx.GetSessionVars().StmtCtx.WarningCount()), Equals, 0, comment) + } + if t.expected == nil { c.Assert(d.IsNull(), IsTrue, comment) } else { From 1682a500140643a0466ee177e3a38257dce136b9 Mon Sep 17 00:00:00 2001 From: Guo Song Date: Fri, 8 Mar 2019 11:11:44 +0800 Subject: [PATCH 04/10] fix some isNull, add some warnings --- expression/builtin_json.go | 13 ++++++++----- expression/builtin_json_test.go | 6 ++++-- 2 files changed, 12 insertions(+), 7 deletions(-) diff --git a/expression/builtin_json.go b/expression/builtin_json.go index 47cbe0538eb5a..9324ea4d0542a 100644 --- a/expression/builtin_json.go +++ b/expression/builtin_json.go @@ -736,9 +736,11 @@ func (b *builtinJSONArrayAppendSig) Clone() builtinFunc { func (b *builtinJSONArrayAppendSig) evalJSON(row chunk.Row) (res json.BinaryJSON, isNull bool, err error) { res, isNull, err = b.args[0].EvalJSON(b.ctx, row) - if isNull || err != nil { - // bad json will only get nil in mysql - // select JSON_ARRAY_APPEND('asdf', "$", NULL);show warnings; + if err != nil { + b.ctx.GetSessionVars().StmtCtx.AppendWarning(json.ErrInvalidJSONPath.FastGen("Syntax error in JSON text in argument 1 to function 'json_set'")) + return res, true, nil + } + if isNull { return res, true, nil } @@ -749,7 +751,7 @@ func (b *builtinJSONArrayAppendSig) evalJSON(row chunk.Row) (res json.BinaryJSON s, isNull, err := b.args[i].EvalString(b.ctx, row) if isNull || err != nil { b.ctx.GetSessionVars().StmtCtx.AppendWarning(json.ErrInvalidJSONPath.FastGen("Syntax error in JSON path in argument %d to function 'json_array_append'", i+1)) - return res, isNull, nil + return res, true, nil } pathExpr, err := json.ParseJSONPathExpr(s) if err != nil { @@ -795,7 +797,8 @@ func (b *builtinJSONArrayAppendSig) evalJSON(row chunk.Row) (res json.BinaryJSON } // we have done same checks where res.Modify might return with error before, thus ignore it } - return res, isNull, err + // res won't be null + return res, false, err } type jsonArrayInsertFunctionClass struct { diff --git a/expression/builtin_json_test.go b/expression/builtin_json_test.go index f34af5ee12e72..08ff373c45cf3 100644 --- a/expression/builtin_json_test.go +++ b/expression/builtin_json_test.go @@ -707,9 +707,11 @@ func (s *testEvaluatorSuite) TestJSONArrayAppend(c *C) { // explains hy call res.Modify every path-value pair and not do as JSON_SET do? {[]interface{}{`{"a": 1}`, `$.a`, sampleJSON, `$.a[1]`, sampleJSON}, `{"a": [1, [{"b": 2}, {"b": 2}]]}`, true, false}, {[]interface{}{nil, `$`, nil}, nil, true, false}, + {[]interface{}{nil, `$`, `a`}, nil, true, false}, + {[]interface{}{`null`, `$`, nil}, `[null]`, true, false}, // bad arguments - {[]interface{}{`asdf`, `$`, nil}, nil, true, false}, - {[]interface{}{``, `$`, nil}, nil, true, false}, + {[]interface{}{`asdf`, `$`, nil}, nil, true, true}, + {[]interface{}{``, `$`, nil}, nil, true, true}, {[]interface{}{`[]`, `$`, nil}, `[null]`, true, false}, {[]interface{}{`{}`, `$`, nil}, `[{}, null]`, true, false}, // following tests comes from MySQL doc From 1c3269cfc37ef8614072f1ea206f793a79bba2ff Mon Sep 17 00:00:00 2001 From: Guo Song Date: Fri, 8 Mar 2019 11:20:12 +0800 Subject: [PATCH 05/10] fix test --- expression/builtin_json_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/expression/builtin_json_test.go b/expression/builtin_json_test.go index 08ff373c45cf3..1bad8d38f6a28 100644 --- a/expression/builtin_json_test.go +++ b/expression/builtin_json_test.go @@ -708,7 +708,7 @@ func (s *testEvaluatorSuite) TestJSONArrayAppend(c *C) { {[]interface{}{`{"a": 1}`, `$.a`, sampleJSON, `$.a[1]`, sampleJSON}, `{"a": [1, [{"b": 2}, {"b": 2}]]}`, true, false}, {[]interface{}{nil, `$`, nil}, nil, true, false}, {[]interface{}{nil, `$`, `a`}, nil, true, false}, - {[]interface{}{`null`, `$`, nil}, `[null]`, true, false}, + {[]interface{}{`null`, `$`, nil}, `[null, null]`, true, false}, // bad arguments {[]interface{}{`asdf`, `$`, nil}, nil, true, true}, {[]interface{}{``, `$`, nil}, nil, true, true}, From b4186433954de05d4b435e80cfb4c6a7aa287998 Mon Sep 17 00:00:00 2001 From: Guo Song Date: Mon, 11 Mar 2019 11:15:16 +0800 Subject: [PATCH 06/10] add test in strict sql mode, move param count check in verifyArgs --- expression/builtin_json.go | 39 +++++++---- expression/builtin_json_test.go | 114 +++++++++++++++++++++++--------- 2 files changed, 107 insertions(+), 46 deletions(-) diff --git a/expression/builtin_json.go b/expression/builtin_json.go index 9324ea4d0542a..7b6c98c5b06dd 100644 --- a/expression/builtin_json.go +++ b/expression/builtin_json.go @@ -707,13 +707,17 @@ type builtinJSONArrayAppendSig struct { baseBuiltinFunc } +func (c *jsonArrayAppendFunctionClass) verifyArgs(args []Expression) error { + if len(args) < 3 || (len(args)&1 != 1) { + return ErrIncorrectParameterCount.GenWithStackByArgs(c.funcName) + } + return nil +} + func (c *jsonArrayAppendFunctionClass) getFunction(ctx sessionctx.Context, args []Expression) (builtinFunc, error) { if err := c.verifyArgs(args); err != nil { return nil, err } - if len(args)&1 != 1 { - return nil, ErrIncorrectParameterCount.GenWithStackByArgs(c.funcName) - } argTps := make([]types.EvalType, 0, len(args)) argTps = append(argTps, types.ETJson) for i := 1; i < len(args)-1; i += 2 { @@ -734,11 +738,18 @@ func (b *builtinJSONArrayAppendSig) Clone() builtinFunc { return newSig } +func (b *builtinJSONArrayAppendSig) checkError(err error) error { + if err == nil || b.ctx.GetSessionVars().StrictSQLMode { + return err + } + b.ctx.GetSessionVars().StmtCtx.AppendWarning(err) + return nil +} + func (b *builtinJSONArrayAppendSig) evalJSON(row chunk.Row) (res json.BinaryJSON, isNull bool, err error) { res, isNull, err = b.args[0].EvalJSON(b.ctx, row) if err != nil { - b.ctx.GetSessionVars().StmtCtx.AppendWarning(json.ErrInvalidJSONPath.FastGen("Syntax error in JSON text in argument 1 to function 'json_set'")) - return res, true, nil + return res, true, b.checkError(err) } if isNull { return res, true, nil @@ -749,18 +760,19 @@ func (b *builtinJSONArrayAppendSig) evalJSON(row chunk.Row) (res json.BinaryJSON // NOTE: mysql does not report error // also they does not ignore it, they just return NULL s, isNull, err := b.args[i].EvalString(b.ctx, row) - if isNull || err != nil { - b.ctx.GetSessionVars().StmtCtx.AppendWarning(json.ErrInvalidJSONPath.FastGen("Syntax error in JSON path in argument %d to function 'json_array_append'", i+1)) - return res, true, nil + if isNull { + return res, true, b.checkError(json.ErrInvalidJSONPath.GenWithStackByArgs(s)) + } + err = b.checkError(err) + if err != nil { + return res, true, err } pathExpr, err := json.ParseJSONPathExpr(s) if err != nil { - b.ctx.GetSessionVars().StmtCtx.AppendWarning(json.ErrInvalidJSONPath.FastGen("Syntax error in JSON path in argument %d to function 'json_array_append'", i+1)) - return res, true, nil + return res, true, b.checkError(json.ErrInvalidJSONPath.GenWithStackByArgs(s)) } if pathExpr.ContainsAnyAsterisk() { - b.ctx.GetSessionVars().StmtCtx.AppendWarning(json.ErrInvalidJSONPath.FastGen("Wildcard path expression is not allowed in function 'json_array_append' at position %d", i+1)) - return res, true, nil + return res, true, b.checkError(json.ErrInvalidJSONPathWildcard.GenWithStackByArgs(s)) } var exists bool @@ -792,8 +804,9 @@ func (b *builtinJSONArrayAppendSig) evalJSON(row chunk.Row) (res json.BinaryJSON obj = json.MergeBinary([]json.BinaryJSON{obj, value}) res, err = res.Modify([]json.PathExpression{pathExpr}, []json.BinaryJSON{obj}, json.ModifySet) + err = b.checkError(err) if err != nil { - b.ctx.GetSessionVars().StmtCtx.AppendWarning(err) + return res, true, err } // we have done same checks where res.Modify might return with error before, thus ignore it } diff --git a/expression/builtin_json_test.go b/expression/builtin_json_test.go index 1bad8d38f6a28..a64c64a183845 100644 --- a/expression/builtin_json_test.go +++ b/expression/builtin_json_test.go @@ -16,6 +16,7 @@ package expression import ( . "github.com/pingcap/check" "github.com/pingcap/parser/ast" + "github.com/pingcap/parser/terror" "github.com/pingcap/tidb/types" "github.com/pingcap/tidb/types/json" "github.com/pingcap/tidb/util/chunk" @@ -695,53 +696,65 @@ func (s *testEvaluatorSuite) TestJSONArrayAppend(c *C) { tbl := []struct { input []interface{} expected interface{} - success bool - warning bool + success int // 0 always success, 1 success on non-strict, 2 always fail + warning *terror.Error }{ - {[]interface{}{`{"a": 1, "b": [2, 3], "c": 4}`, `$.d`, `z`}, `{"a": 1, "b": [2, 3], "c": 4}`, true, false}, - {[]interface{}{`{"a": 1, "b": [2, 3], "c": 4}`, `$`, `w`}, `[{"a": 1, "b": [2, 3], "c": 4}, "w"]`, true, false}, - {[]interface{}{`{"a": 1, "b": [2, 3], "c": 4}`, `$`, nil}, `[{"a": 1, "b": [2, 3], "c": 4}, null]`, true, false}, - {[]interface{}{`{"a": 1}`, `$`, `{"b": 2}`}, `[{"a": 1}, "{\"b\": 2}"]`, true, false}, - {[]interface{}{`{"a": 1}`, `$`, sampleJSON}, `[{"a": 1}, {"b": 2}]`, true, false}, - {[]interface{}{`{"a": 1}`, `$.a`, sampleJSON}, `{"a": [1, {"b": 2}]}`, true, false}, + {[]interface{}{`{"a": 1, "b": [2, 3], "c": 4}`, `$.d`, `z`}, `{"a": 1, "b": [2, 3], "c": 4}`, 0, nil}, + {[]interface{}{`{"a": 1, "b": [2, 3], "c": 4}`, `$`, `w`}, `[{"a": 1, "b": [2, 3], "c": 4}, "w"]`, 0, nil}, + {[]interface{}{`{"a": 1, "b": [2, 3], "c": 4}`, `$`, nil}, `[{"a": 1, "b": [2, 3], "c": 4}, null]`, 0, nil}, + {[]interface{}{`{"a": 1}`, `$`, `{"b": 2}`}, `[{"a": 1}, "{\"b\": 2}"]`, 0, nil}, + {[]interface{}{`{"a": 1}`, `$`, sampleJSON}, `[{"a": 1}, {"b": 2}]`, 0, nil}, + {[]interface{}{`{"a": 1}`, `$.a`, sampleJSON}, `{"a": [1, {"b": 2}]}`, 0, nil}, // explains hy call res.Modify every path-value pair and not do as JSON_SET do? - {[]interface{}{`{"a": 1}`, `$.a`, sampleJSON, `$.a[1]`, sampleJSON}, `{"a": [1, [{"b": 2}, {"b": 2}]]}`, true, false}, - {[]interface{}{nil, `$`, nil}, nil, true, false}, - {[]interface{}{nil, `$`, `a`}, nil, true, false}, - {[]interface{}{`null`, `$`, nil}, `[null, null]`, true, false}, + {[]interface{}{`{"a": 1}`, `$.a`, sampleJSON, `$.a[1]`, sampleJSON}, `{"a": [1, [{"b": 2}, {"b": 2}]]}`, 0, nil}, + {[]interface{}{nil, `$`, nil}, nil, 0, nil}, + {[]interface{}{nil, `$`, `a`}, nil, 0, nil}, + {[]interface{}{`null`, `$`, nil}, `[null, null]`, 0, nil}, // bad arguments - {[]interface{}{`asdf`, `$`, nil}, nil, true, true}, - {[]interface{}{``, `$`, nil}, nil, true, true}, - {[]interface{}{`[]`, `$`, nil}, `[null]`, true, false}, - {[]interface{}{`{}`, `$`, nil}, `[{}, null]`, true, false}, + {[]interface{}{`asdf`, `$`, nil}, nil, 1, json.ErrInvalidJSONText}, + {[]interface{}{``, `$`, nil}, nil, 1, json.ErrInvalidJSONText}, + {[]interface{}{`[]`, `$`, nil}, `[null]`, 0, nil}, + {[]interface{}{`{}`, `$`, nil}, `[{}, null]`, 0, nil}, + {[]interface{}{`{"a": 1, "b": [2, 3], "c": 4}`, `$.d`}, nil, 2, ErrIncorrectParameterCount}, + {[]interface{}{`{"a": 1, "b": [2, 3], "c": 4}`, `$.c`, `y`, `$.b`}, nil, 2, ErrIncorrectParameterCount}, // following tests comes from MySQL doc - {[]interface{}{`["a", ["b", "c"], "d"]`, `$[1]`, 1}, `["a", ["b", "c", 1], "d"]`, true, false}, - {[]interface{}{`["a", ["b", "c"], "d"]`, `$[0]`, 2}, `[["a", 2], ["b", "c"], "d"]`, true, false}, - {[]interface{}{`["a", ["b", "c"], "d"]`, `$[1][0]`, 3}, `["a", [["b", 3], "c"], "d"]`, true, false}, - {[]interface{}{`{"a": 1, "b": [2, 3], "c": 4}`, `$.b`, `x`}, `{"a": 1, "b": [2, 3, "x"], "c": 4}`, true, false}, - {[]interface{}{`{"a": 1, "b": [2, 3], "c": 4}`, `$.c`, `y`}, `{"a": 1, "b": [2, 3], "c": [4, "y"]}`, true, false}, + {[]interface{}{`["a", ["b", "c"], "d"]`, `$[1]`, 1}, `["a", ["b", "c", 1], "d"]`, 0, nil}, + {[]interface{}{`["a", ["b", "c"], "d"]`, `$[0]`, 2}, `[["a", 2], ["b", "c"], "d"]`, 0, nil}, + {[]interface{}{`["a", ["b", "c"], "d"]`, `$[1][0]`, 3}, `["a", [["b", 3], "c"], "d"]`, 0, nil}, + {[]interface{}{`{"a": 1, "b": [2, 3], "c": 4}`, `$.b`, `x`}, `{"a": 1, "b": [2, 3, "x"], "c": 4}`, 0, nil}, + {[]interface{}{`{"a": 1, "b": [2, 3], "c": 4}`, `$.c`, `y`}, `{"a": 1, "b": [2, 3], "c": [4, "y"]}`, 0, nil}, // following tests comes from MySQL test - {[]interface{}{`[1,2,3, {"a":[4,5,6]}]`, `$`, 7}, `[1, 2, 3, {"a": [4, 5, 6]}, 7]`, true, false}, - {[]interface{}{`[1,2,3, {"a":[4,5,6]}]`, `$`, 7, `$[3].a`, 3.14}, `[1, 2, 3, {"a": [4, 5, 6, 3.14]}, 7]`, true, false}, - {[]interface{}{`[1,2,3, {"a":[4,5,6]}]`, `$`, 7, `$[3].b`, 8}, `[1, 2, 3, {"a": [4, 5, 6]}, 7]`, true, false}, + {[]interface{}{`[1,2,3, {"a":[4,5,6]}]`, `$`, 7}, `[1, 2, 3, {"a": [4, 5, 6]}, 7]`, 0, nil}, + {[]interface{}{`[1,2,3, {"a":[4,5,6]}]`, `$`, 7, `$[3].a`, 3.14}, `[1, 2, 3, {"a": [4, 5, 6, 3.14]}, 7]`, 0, nil}, + {[]interface{}{`[1,2,3, {"a":[4,5,6]}]`, `$`, 7, `$[3].b`, 8}, `[1, 2, 3, {"a": [4, 5, 6]}, 7]`, 0, nil}, // warning instead of error for illegal params - {[]interface{}{`{"a": 1, "b": [2, 3], "c": 4}`, nil, nil}, nil, true, true}, - {[]interface{}{`{"a": 1, "b": [2, 3], "c": 4}`, `asdf`, nil}, nil, true, true}, - {[]interface{}{`{"a": 1, "b": [2, 3], "c": 4}`, 42, nil}, nil, true, true}, - {[]interface{}{`{"a": 1, "b": [2, 3], "c": 4}`, `$.*`, nil}, nil, true, true}, + {[]interface{}{`{"a": 1, "b": [2, 3], "c": 4}`, nil, nil}, nil, 1, json.ErrInvalidJSONPath}, + {[]interface{}{`{"a": 1, "b": [2, 3], "c": 4}`, `asdf`, nil}, nil, 1, json.ErrInvalidJSONPath}, + {[]interface{}{`{"a": 1, "b": [2, 3], "c": 4}`, 42, nil}, nil, 1, json.ErrInvalidJSONPath}, + {[]interface{}{`{"a": 1, "b": [2, 3], "c": 4}`, `$.*`, nil}, nil, 1, json.ErrInvalidJSONPathWildcard}, } + + // non-strict mode for i, t := range tbl { args := types.MakeDatums(t.input...) + s.ctx.GetSessionVars().StrictSQLMode = false s.ctx.GetSessionVars().StmtCtx.SetWarnings(nil) f, err := fc.getFunction(s.ctx, s.datumsToConstants(args)) + // if t.success <= 1, no error should return in getFunction + if t.success == 2 && err != nil { + c.Assert(t.warning.Equal(err), Equals, true) + // won't execute f + continue + } c.Assert(err, IsNil) d, err := evalBuiltinFunc(f, chunk.Row{}) - comment := Commentf("case:%v \n input:%v \n output: %s \n expected: %v \n warnings: %v \n should return warnings? %v", i, t.input, d.GetMysqlJSON(), t.expected, s.ctx.GetSessionVars().StmtCtx.GetWarnings(), t.warning) - if t.success { + comment := Commentf("case:%v \n input:%v \n output: %s \n expected: %v \n warnings: %v \n expected error %v", i, t.input, d.GetMysqlJSON(), t.expected, s.ctx.GetSessionVars().StmtCtx.GetWarnings(), t.warning) + if t.success <= 1 { c.Assert(err, IsNil, comment) - if t.warning { - c.Assert(int(s.ctx.GetSessionVars().StmtCtx.WarningCount()), Greater, 0, comment) + if t.warning != nil { + c.Assert(int(s.ctx.GetSessionVars().StmtCtx.WarningCount()), Equals, 1, comment) + c.Assert(t.warning.Equal(s.ctx.GetSessionVars().StmtCtx.GetWarnings()[0].Err), Equals, true) } else { c.Assert(int(s.ctx.GetSessionVars().StmtCtx.WarningCount()), Equals, 0, comment) } @@ -759,7 +772,42 @@ func (s *testEvaluatorSuite) TestJSONArrayAppend(c *C) { c.Assert(cmp, Equals, 0, comment) } } else { - c.Assert(err, NotNil, comment) + c.Assert(t.warning.Equal(err), Equals, true) + } + } + + // strict mode + for i, t := range tbl { + args := types.MakeDatums(t.input...) + s.ctx.GetSessionVars().StrictSQLMode = true + s.ctx.GetSessionVars().StmtCtx.SetWarnings(nil) + f, err := fc.getFunction(s.ctx, s.datumsToConstants(args)) + if t.warning != nil && err != nil { + c.Assert(t.warning.Equal(err), Equals, true) + // won't execute f + continue + } + c.Assert(err, IsNil) + d, err := evalBuiltinFunc(f, chunk.Row{}) + comment := Commentf("case:%v \n input:%v \n output: %s \n expected: %v \n warnings: %v \n expected error %v", i, t.input, d.GetMysqlJSON(), t.expected, s.ctx.GetSessionVars().StmtCtx.GetWarnings(), t.warning) + if t.success == 0 { + c.Assert(err, IsNil, comment) + c.Assert(int(s.ctx.GetSessionVars().StmtCtx.WarningCount()), Equals, 0, comment) + + if t.expected == nil { + c.Assert(d.IsNull(), IsTrue, comment) + } else { + var j1 json.BinaryJSON + j1, err = json.ParseBinaryFromString(t.expected.(string)) + c.Assert(err, IsNil) + j2 := d.GetMysqlJSON() + var cmp int + cmp = json.CompareBinary(j1, j2) + c.Assert(err, IsNil, comment) + c.Assert(cmp, Equals, 0, comment) + } + } else { + c.Assert(t.warning.Equal(err), Equals, true) } } } From d62d7d8457b51ae411de7c4b2f8c30ee21654f1c Mon Sep 17 00:00:00 2001 From: Guo Song Date: Mon, 11 Mar 2019 18:52:25 +0800 Subject: [PATCH 07/10] fix behavior --- expression/builtin_json.go | 36 +++------- expression/builtin_json_test.go | 112 ++++++++++---------------------- 2 files changed, 43 insertions(+), 105 deletions(-) diff --git a/expression/builtin_json.go b/expression/builtin_json.go index 7b6c98c5b06dd..db6924755ef64 100644 --- a/expression/builtin_json.go +++ b/expression/builtin_json.go @@ -738,41 +738,24 @@ func (b *builtinJSONArrayAppendSig) Clone() builtinFunc { return newSig } -func (b *builtinJSONArrayAppendSig) checkError(err error) error { - if err == nil || b.ctx.GetSessionVars().StrictSQLMode { - return err - } - b.ctx.GetSessionVars().StmtCtx.AppendWarning(err) - return nil -} - func (b *builtinJSONArrayAppendSig) evalJSON(row chunk.Row) (res json.BinaryJSON, isNull bool, err error) { res, isNull, err = b.args[0].EvalJSON(b.ctx, row) - if err != nil { - return res, true, b.checkError(err) - } - if isNull { - return res, true, nil + if err != nil || isNull { + return res, true, err } for i := 1; i < len(b.args)-1; i += 2 { - // bad arguments should return null - // NOTE: mysql does not report error - // also they does not ignore it, they just return NULL + // if JSON path is Null, then mysql break and return null s, isNull, err := b.args[i].EvalString(b.ctx, row) - if isNull { - return res, true, b.checkError(json.ErrInvalidJSONPath.GenWithStackByArgs(s)) - } - err = b.checkError(err) - if err != nil { + if isNull || err != nil { return res, true, err } pathExpr, err := json.ParseJSONPathExpr(s) if err != nil { - return res, true, b.checkError(json.ErrInvalidJSONPath.GenWithStackByArgs(s)) + return res, true, json.ErrInvalidJSONPath.GenWithStackByArgs(s) } if pathExpr.ContainsAnyAsterisk() { - return res, true, b.checkError(json.ErrInvalidJSONPathWildcard.GenWithStackByArgs(s)) + return res, true, json.ErrInvalidJSONPathWildcard.GenWithStackByArgs(s) } var exists bool @@ -804,14 +787,13 @@ func (b *builtinJSONArrayAppendSig) evalJSON(row chunk.Row) (res json.BinaryJSON obj = json.MergeBinary([]json.BinaryJSON{obj, value}) res, err = res.Modify([]json.PathExpression{pathExpr}, []json.BinaryJSON{obj}, json.ModifySet) - err = b.checkError(err) if err != nil { + // err should always be nil, the function should never return here return res, true, err } - // we have done same checks where res.Modify might return with error before, thus ignore it } - // res won't be null - return res, false, err + // res won't be nil, error should be nil + return res, false, nil } type jsonArrayInsertFunctionClass struct { diff --git a/expression/builtin_json_test.go b/expression/builtin_json_test.go index a64c64a183845..d8369c57358ad 100644 --- a/expression/builtin_json_test.go +++ b/expression/builtin_json_test.go @@ -696,101 +696,57 @@ func (s *testEvaluatorSuite) TestJSONArrayAppend(c *C) { tbl := []struct { input []interface{} expected interface{} - success int // 0 always success, 1 success on non-strict, 2 always fail - warning *terror.Error + err *terror.Error }{ - {[]interface{}{`{"a": 1, "b": [2, 3], "c": 4}`, `$.d`, `z`}, `{"a": 1, "b": [2, 3], "c": 4}`, 0, nil}, - {[]interface{}{`{"a": 1, "b": [2, 3], "c": 4}`, `$`, `w`}, `[{"a": 1, "b": [2, 3], "c": 4}, "w"]`, 0, nil}, - {[]interface{}{`{"a": 1, "b": [2, 3], "c": 4}`, `$`, nil}, `[{"a": 1, "b": [2, 3], "c": 4}, null]`, 0, nil}, - {[]interface{}{`{"a": 1}`, `$`, `{"b": 2}`}, `[{"a": 1}, "{\"b\": 2}"]`, 0, nil}, - {[]interface{}{`{"a": 1}`, `$`, sampleJSON}, `[{"a": 1}, {"b": 2}]`, 0, nil}, - {[]interface{}{`{"a": 1}`, `$.a`, sampleJSON}, `{"a": [1, {"b": 2}]}`, 0, nil}, + {[]interface{}{`{"a": 1, "b": [2, 3], "c": 4}`, `$.d`, `z`}, `{"a": 1, "b": [2, 3], "c": 4}`, nil}, + {[]interface{}{`{"a": 1, "b": [2, 3], "c": 4}`, `$`, `w`}, `[{"a": 1, "b": [2, 3], "c": 4}, "w"]`, nil}, + {[]interface{}{`{"a": 1, "b": [2, 3], "c": 4}`, `$`, nil}, `[{"a": 1, "b": [2, 3], "c": 4}, null]`, nil}, + {[]interface{}{`{"a": 1}`, `$`, `{"b": 2}`}, `[{"a": 1}, "{\"b\": 2}"]`, nil}, + {[]interface{}{`{"a": 1}`, `$`, sampleJSON}, `[{"a": 1}, {"b": 2}]`, nil}, + {[]interface{}{`{"a": 1}`, `$.a`, sampleJSON}, `{"a": [1, {"b": 2}]}`, nil}, // explains hy call res.Modify every path-value pair and not do as JSON_SET do? - {[]interface{}{`{"a": 1}`, `$.a`, sampleJSON, `$.a[1]`, sampleJSON}, `{"a": [1, [{"b": 2}, {"b": 2}]]}`, 0, nil}, - {[]interface{}{nil, `$`, nil}, nil, 0, nil}, - {[]interface{}{nil, `$`, `a`}, nil, 0, nil}, - {[]interface{}{`null`, `$`, nil}, `[null, null]`, 0, nil}, + {[]interface{}{`{"a": 1}`, `$.a`, sampleJSON, `$.a[1]`, sampleJSON}, `{"a": [1, [{"b": 2}, {"b": 2}]]}`, nil}, + {[]interface{}{nil, `$`, nil}, nil, nil}, + {[]interface{}{nil, `$`, `a`}, nil, nil}, + {[]interface{}{`null`, `$`, nil}, `[null, null]`, nil}, // bad arguments - {[]interface{}{`asdf`, `$`, nil}, nil, 1, json.ErrInvalidJSONText}, - {[]interface{}{``, `$`, nil}, nil, 1, json.ErrInvalidJSONText}, - {[]interface{}{`[]`, `$`, nil}, `[null]`, 0, nil}, - {[]interface{}{`{}`, `$`, nil}, `[{}, null]`, 0, nil}, - {[]interface{}{`{"a": 1, "b": [2, 3], "c": 4}`, `$.d`}, nil, 2, ErrIncorrectParameterCount}, - {[]interface{}{`{"a": 1, "b": [2, 3], "c": 4}`, `$.c`, `y`, `$.b`}, nil, 2, ErrIncorrectParameterCount}, + {[]interface{}{`asdf`, `$`, nil}, nil, json.ErrInvalidJSONText}, + {[]interface{}{``, `$`, nil}, nil, json.ErrInvalidJSONText}, + {[]interface{}{`[]`, `$`, nil}, `[null]`, nil}, + {[]interface{}{`{}`, `$`, nil}, `[{}, null]`, nil}, + {[]interface{}{`{"a": 1, "b": [2, 3], "c": 4}`, `$.d`}, nil, ErrIncorrectParameterCount}, + {[]interface{}{`{"a": 1, "b": [2, 3], "c": 4}`, `$.c`, `y`, `$.b`}, nil, ErrIncorrectParameterCount}, // following tests comes from MySQL doc - {[]interface{}{`["a", ["b", "c"], "d"]`, `$[1]`, 1}, `["a", ["b", "c", 1], "d"]`, 0, nil}, - {[]interface{}{`["a", ["b", "c"], "d"]`, `$[0]`, 2}, `[["a", 2], ["b", "c"], "d"]`, 0, nil}, - {[]interface{}{`["a", ["b", "c"], "d"]`, `$[1][0]`, 3}, `["a", [["b", 3], "c"], "d"]`, 0, nil}, - {[]interface{}{`{"a": 1, "b": [2, 3], "c": 4}`, `$.b`, `x`}, `{"a": 1, "b": [2, 3, "x"], "c": 4}`, 0, nil}, - {[]interface{}{`{"a": 1, "b": [2, 3], "c": 4}`, `$.c`, `y`}, `{"a": 1, "b": [2, 3], "c": [4, "y"]}`, 0, nil}, + {[]interface{}{`["a", ["b", "c"], "d"]`, `$[1]`, 1}, `["a", ["b", "c", 1], "d"]`, nil}, + {[]interface{}{`["a", ["b", "c"], "d"]`, `$[0]`, 2}, `[["a", 2], ["b", "c"], "d"]`, nil}, + {[]interface{}{`["a", ["b", "c"], "d"]`, `$[1][0]`, 3}, `["a", [["b", 3], "c"], "d"]`, nil}, + {[]interface{}{`{"a": 1, "b": [2, 3], "c": 4}`, `$.b`, `x`}, `{"a": 1, "b": [2, 3, "x"], "c": 4}`, nil}, + {[]interface{}{`{"a": 1, "b": [2, 3], "c": 4}`, `$.c`, `y`}, `{"a": 1, "b": [2, 3], "c": [4, "y"]}`, nil}, // following tests comes from MySQL test - {[]interface{}{`[1,2,3, {"a":[4,5,6]}]`, `$`, 7}, `[1, 2, 3, {"a": [4, 5, 6]}, 7]`, 0, nil}, - {[]interface{}{`[1,2,3, {"a":[4,5,6]}]`, `$`, 7, `$[3].a`, 3.14}, `[1, 2, 3, {"a": [4, 5, 6, 3.14]}, 7]`, 0, nil}, - {[]interface{}{`[1,2,3, {"a":[4,5,6]}]`, `$`, 7, `$[3].b`, 8}, `[1, 2, 3, {"a": [4, 5, 6]}, 7]`, 0, nil}, + {[]interface{}{`[1,2,3, {"a":[4,5,6]}]`, `$`, 7}, `[1, 2, 3, {"a": [4, 5, 6]}, 7]`, nil}, + {[]interface{}{`[1,2,3, {"a":[4,5,6]}]`, `$`, 7, `$[3].a`, 3.14}, `[1, 2, 3, {"a": [4, 5, 6, 3.14]}, 7]`, nil}, + {[]interface{}{`[1,2,3, {"a":[4,5,6]}]`, `$`, 7, `$[3].b`, 8}, `[1, 2, 3, {"a": [4, 5, 6]}, 7]`, nil}, // warning instead of error for illegal params - {[]interface{}{`{"a": 1, "b": [2, 3], "c": 4}`, nil, nil}, nil, 1, json.ErrInvalidJSONPath}, - {[]interface{}{`{"a": 1, "b": [2, 3], "c": 4}`, `asdf`, nil}, nil, 1, json.ErrInvalidJSONPath}, - {[]interface{}{`{"a": 1, "b": [2, 3], "c": 4}`, 42, nil}, nil, 1, json.ErrInvalidJSONPath}, - {[]interface{}{`{"a": 1, "b": [2, 3], "c": 4}`, `$.*`, nil}, nil, 1, json.ErrInvalidJSONPathWildcard}, + {[]interface{}{`{"a": 1, "b": [2, 3], "c": 4}`, `asdf`, nil}, nil, json.ErrInvalidJSONPath}, + {[]interface{}{`{"a": 1, "b": [2, 3], "c": 4}`, 42, nil}, nil, json.ErrInvalidJSONPath}, + {[]interface{}{`{"a": 1, "b": [2, 3], "c": 4}`, `$.*`, nil}, nil, json.ErrInvalidJSONPathWildcard}, } // non-strict mode for i, t := range tbl { args := types.MakeDatums(t.input...) - s.ctx.GetSessionVars().StrictSQLMode = false s.ctx.GetSessionVars().StmtCtx.SetWarnings(nil) f, err := fc.getFunction(s.ctx, s.datumsToConstants(args)) // if t.success <= 1, no error should return in getFunction - if t.success == 2 && err != nil { - c.Assert(t.warning.Equal(err), Equals, true) + if err != nil { + c.Assert(t.err.Equal(err), Equals, true) // won't execute f continue } - c.Assert(err, IsNil) - d, err := evalBuiltinFunc(f, chunk.Row{}) - comment := Commentf("case:%v \n input:%v \n output: %s \n expected: %v \n warnings: %v \n expected error %v", i, t.input, d.GetMysqlJSON(), t.expected, s.ctx.GetSessionVars().StmtCtx.GetWarnings(), t.warning) - if t.success <= 1 { - c.Assert(err, IsNil, comment) - - if t.warning != nil { - c.Assert(int(s.ctx.GetSessionVars().StmtCtx.WarningCount()), Equals, 1, comment) - c.Assert(t.warning.Equal(s.ctx.GetSessionVars().StmtCtx.GetWarnings()[0].Err), Equals, true) - } else { - c.Assert(int(s.ctx.GetSessionVars().StmtCtx.WarningCount()), Equals, 0, comment) - } - - if t.expected == nil { - c.Assert(d.IsNull(), IsTrue, comment) - } else { - var j1 json.BinaryJSON - j1, err = json.ParseBinaryFromString(t.expected.(string)) - c.Assert(err, IsNil) - j2 := d.GetMysqlJSON() - var cmp int - cmp = json.CompareBinary(j1, j2) - c.Assert(err, IsNil, comment) - c.Assert(cmp, Equals, 0, comment) - } - } else { - c.Assert(t.warning.Equal(err), Equals, true) - } - } - - // strict mode - for i, t := range tbl { - args := types.MakeDatums(t.input...) - s.ctx.GetSessionVars().StrictSQLMode = true - s.ctx.GetSessionVars().StmtCtx.SetWarnings(nil) - f, err := fc.getFunction(s.ctx, s.datumsToConstants(args)) - if t.warning != nil && err != nil { - c.Assert(t.warning.Equal(err), Equals, true) - // won't execute f - continue - } - c.Assert(err, IsNil) + c.Assert(f, NotNil) d, err := evalBuiltinFunc(f, chunk.Row{}) - comment := Commentf("case:%v \n input:%v \n output: %s \n expected: %v \n warnings: %v \n expected error %v", i, t.input, d.GetMysqlJSON(), t.expected, s.ctx.GetSessionVars().StmtCtx.GetWarnings(), t.warning) - if t.success == 0 { + comment := Commentf("case:%v \n input:%v \n output: %s \n expected: %v \n warnings: %v \n expected error %v", i, t.input, d.GetMysqlJSON(), t.expected, s.ctx.GetSessionVars().StmtCtx.GetWarnings(), t.err) + if t.err == nil { c.Assert(err, IsNil, comment) c.Assert(int(s.ctx.GetSessionVars().StmtCtx.WarningCount()), Equals, 0, comment) @@ -807,7 +763,7 @@ func (s *testEvaluatorSuite) TestJSONArrayAppend(c *C) { c.Assert(cmp, Equals, 0, comment) } } else { - c.Assert(t.warning.Equal(err), Equals, true) + c.Assert(t.err.Equal(err), Equals, true, comment) } } } From 0263af6b26f44a6d2f33733c65aa9ce864665302 Mon Sep 17 00:00:00 2001 From: Guo Song Date: Mon, 11 Mar 2019 19:21:37 +0800 Subject: [PATCH 08/10] add one test --- expression/builtin_json_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/expression/builtin_json_test.go b/expression/builtin_json_test.go index d8369c57358ad..4d10b7fc52352 100644 --- a/expression/builtin_json_test.go +++ b/expression/builtin_json_test.go @@ -727,6 +727,7 @@ func (s *testEvaluatorSuite) TestJSONArrayAppend(c *C) { {[]interface{}{`[1,2,3, {"a":[4,5,6]}]`, `$`, 7, `$[3].a`, 3.14}, `[1, 2, 3, {"a": [4, 5, 6, 3.14]}, 7]`, nil}, {[]interface{}{`[1,2,3, {"a":[4,5,6]}]`, `$`, 7, `$[3].b`, 8}, `[1, 2, 3, {"a": [4, 5, 6]}, 7]`, nil}, // warning instead of error for illegal params + {[]interface{}{`{"a": 1, "b": [2, 3], "c": 4}`, nil, nil}, nil, nil}, {[]interface{}{`{"a": 1, "b": [2, 3], "c": 4}`, `asdf`, nil}, nil, json.ErrInvalidJSONPath}, {[]interface{}{`{"a": 1, "b": [2, 3], "c": 4}`, 42, nil}, nil, json.ErrInvalidJSONPath}, {[]interface{}{`{"a": 1, "b": [2, 3], "c": 4}`, `$.*`, nil}, nil, json.ErrInvalidJSONPathWildcard}, From 2c7506c05ba39870660ee666260bded39d3cc56d Mon Sep 17 00:00:00 2001 From: Guo Song Date: Tue, 12 Mar 2019 11:25:50 +0800 Subject: [PATCH 09/10] rearrange tests and fix comments --- expression/builtin_json.go | 10 +++++----- expression/builtin_json_test.go | 25 +++++++++++-------------- 2 files changed, 16 insertions(+), 19 deletions(-) diff --git a/expression/builtin_json.go b/expression/builtin_json.go index db6924755ef64..67a0ed5019305 100644 --- a/expression/builtin_json.go +++ b/expression/builtin_json.go @@ -745,7 +745,7 @@ func (b *builtinJSONArrayAppendSig) evalJSON(row chunk.Row) (res json.BinaryJSON } for i := 1; i < len(b.args)-1; i += 2 { - // if JSON path is Null, then mysql break and return null + // If JSON path is NULL, MySQL breaks and returns NULL. s, isNull, err := b.args[i].EvalString(b.ctx, row) if isNull || err != nil { return res, true, err @@ -761,13 +761,13 @@ func (b *builtinJSONArrayAppendSig) evalJSON(row chunk.Row) (res json.BinaryJSON var exists bool obj, exists := res.Extract([]json.PathExpression{pathExpr}) if !exists { - // just do nothing for this + // If path not exists, just do nothing and no errors. continue } if obj.TypeCode != json.TypeCodeArray { // JSON_ARRAY_APPEND({"a": "b"}, "$", "c") => [{"a": "b"}, "c"] - // We should convert them to a single array first + // We should convert them to a single array first. obj = json.CreateBinary([]interface{}{obj}) } @@ -788,11 +788,11 @@ func (b *builtinJSONArrayAppendSig) evalJSON(row chunk.Row) (res json.BinaryJSON obj = json.MergeBinary([]json.BinaryJSON{obj, value}) res, err = res.Modify([]json.PathExpression{pathExpr}, []json.BinaryJSON{obj}, json.ModifySet) if err != nil { - // err should always be nil, the function should never return here + // err should always be nil, the function should never return here. return res, true, err } } - // res won't be nil, error should be nil + // res won't be nil, error should be nil. return res, false, nil } diff --git a/expression/builtin_json_test.go b/expression/builtin_json_test.go index 4d10b7fc52352..f5507ca9eea15 100644 --- a/expression/builtin_json_test.go +++ b/expression/builtin_json_test.go @@ -704,44 +704,41 @@ func (s *testEvaluatorSuite) TestJSONArrayAppend(c *C) { {[]interface{}{`{"a": 1}`, `$`, `{"b": 2}`}, `[{"a": 1}, "{\"b\": 2}"]`, nil}, {[]interface{}{`{"a": 1}`, `$`, sampleJSON}, `[{"a": 1}, {"b": 2}]`, nil}, {[]interface{}{`{"a": 1}`, `$.a`, sampleJSON}, `{"a": [1, {"b": 2}]}`, nil}, - // explains hy call res.Modify every path-value pair and not do as JSON_SET do? + {[]interface{}{`{"a": 1}`, `$.a`, sampleJSON, `$.a[1]`, sampleJSON}, `{"a": [1, [{"b": 2}, {"b": 2}]]}`, nil}, {[]interface{}{nil, `$`, nil}, nil, nil}, {[]interface{}{nil, `$`, `a`}, nil, nil}, {[]interface{}{`null`, `$`, nil}, `[null, null]`, nil}, - // bad arguments - {[]interface{}{`asdf`, `$`, nil}, nil, json.ErrInvalidJSONText}, - {[]interface{}{``, `$`, nil}, nil, json.ErrInvalidJSONText}, {[]interface{}{`[]`, `$`, nil}, `[null]`, nil}, {[]interface{}{`{}`, `$`, nil}, `[{}, null]`, nil}, + // Bad arguments. + {[]interface{}{`asdf`, `$`, nil}, nil, json.ErrInvalidJSONText}, + {[]interface{}{``, `$`, nil}, nil, json.ErrInvalidJSONText}, {[]interface{}{`{"a": 1, "b": [2, 3], "c": 4}`, `$.d`}, nil, ErrIncorrectParameterCount}, {[]interface{}{`{"a": 1, "b": [2, 3], "c": 4}`, `$.c`, `y`, `$.b`}, nil, ErrIncorrectParameterCount}, - // following tests comes from MySQL doc + {[]interface{}{`{"a": 1, "b": [2, 3], "c": 4}`, nil, nil}, nil, nil}, + {[]interface{}{`{"a": 1, "b": [2, 3], "c": 4}`, `asdf`, nil}, nil, json.ErrInvalidJSONPath}, + {[]interface{}{`{"a": 1, "b": [2, 3], "c": 4}`, 42, nil}, nil, json.ErrInvalidJSONPath}, + {[]interface{}{`{"a": 1, "b": [2, 3], "c": 4}`, `$.*`, nil}, nil, json.ErrInvalidJSONPathWildcard}, + // Following tests come from MySQL doc. {[]interface{}{`["a", ["b", "c"], "d"]`, `$[1]`, 1}, `["a", ["b", "c", 1], "d"]`, nil}, {[]interface{}{`["a", ["b", "c"], "d"]`, `$[0]`, 2}, `[["a", 2], ["b", "c"], "d"]`, nil}, {[]interface{}{`["a", ["b", "c"], "d"]`, `$[1][0]`, 3}, `["a", [["b", 3], "c"], "d"]`, nil}, {[]interface{}{`{"a": 1, "b": [2, 3], "c": 4}`, `$.b`, `x`}, `{"a": 1, "b": [2, 3, "x"], "c": 4}`, nil}, {[]interface{}{`{"a": 1, "b": [2, 3], "c": 4}`, `$.c`, `y`}, `{"a": 1, "b": [2, 3], "c": [4, "y"]}`, nil}, - // following tests comes from MySQL test + // Following tests come from MySQL test. {[]interface{}{`[1,2,3, {"a":[4,5,6]}]`, `$`, 7}, `[1, 2, 3, {"a": [4, 5, 6]}, 7]`, nil}, {[]interface{}{`[1,2,3, {"a":[4,5,6]}]`, `$`, 7, `$[3].a`, 3.14}, `[1, 2, 3, {"a": [4, 5, 6, 3.14]}, 7]`, nil}, {[]interface{}{`[1,2,3, {"a":[4,5,6]}]`, `$`, 7, `$[3].b`, 8}, `[1, 2, 3, {"a": [4, 5, 6]}, 7]`, nil}, - // warning instead of error for illegal params - {[]interface{}{`{"a": 1, "b": [2, 3], "c": 4}`, nil, nil}, nil, nil}, - {[]interface{}{`{"a": 1, "b": [2, 3], "c": 4}`, `asdf`, nil}, nil, json.ErrInvalidJSONPath}, - {[]interface{}{`{"a": 1, "b": [2, 3], "c": 4}`, 42, nil}, nil, json.ErrInvalidJSONPath}, - {[]interface{}{`{"a": 1, "b": [2, 3], "c": 4}`, `$.*`, nil}, nil, json.ErrInvalidJSONPathWildcard}, } - // non-strict mode for i, t := range tbl { args := types.MakeDatums(t.input...) s.ctx.GetSessionVars().StmtCtx.SetWarnings(nil) f, err := fc.getFunction(s.ctx, s.datumsToConstants(args)) - // if t.success <= 1, no error should return in getFunction + // No error should return in getFunction if t.err is nil. if err != nil { c.Assert(t.err.Equal(err), Equals, true) - // won't execute f continue } c.Assert(f, NotNil) From 2eee87557db61d3257b130585f537a7c4623dea7 Mon Sep 17 00:00:00 2001 From: Guo Song Date: Thu, 14 Mar 2019 11:03:08 +0800 Subject: [PATCH 10/10] update some comments and reformat tests --- expression/builtin_json.go | 19 ++++++++----------- expression/builtin_json_test.go | 33 +++++++++++++++++---------------- 2 files changed, 25 insertions(+), 27 deletions(-) diff --git a/expression/builtin_json.go b/expression/builtin_json.go index 67a0ed5019305..3129f373206d0 100644 --- a/expression/builtin_json.go +++ b/expression/builtin_json.go @@ -750,6 +750,8 @@ func (b *builtinJSONArrayAppendSig) evalJSON(row chunk.Row) (res json.BinaryJSON if isNull || err != nil { return res, true, err } + + // We should do the following checks to get correct values in res.Extract pathExpr, err := json.ParseJSONPathExpr(s) if err != nil { return res, true, json.ErrInvalidJSONPath.GenWithStackByArgs(s) @@ -758,7 +760,6 @@ func (b *builtinJSONArrayAppendSig) evalJSON(row chunk.Row) (res json.BinaryJSON return res, true, json.ErrInvalidJSONPathWildcard.GenWithStackByArgs(s) } - var exists bool obj, exists := res.Extract([]json.PathExpression{pathExpr}) if !exists { // If path not exists, just do nothing and no errors. @@ -766,17 +767,13 @@ func (b *builtinJSONArrayAppendSig) evalJSON(row chunk.Row) (res json.BinaryJSON } if obj.TypeCode != json.TypeCodeArray { - // JSON_ARRAY_APPEND({"a": "b"}, "$", "c") => [{"a": "b"}, "c"] - // We should convert them to a single array first. + // res.Extract will return a json object instead of an array if there is an object at path pathExpr. + // JSON_ARRAY_APPEND({"a": "b"}, "$", {"b": "c"}) => [{"a": "b"}, {"b", "c"}] + // We should wrap them to a single array first. obj = json.CreateBinary([]interface{}{obj}) } - var ( - value json.BinaryJSON - isnull bool - ) - - value, isnull, err = b.args[i+1].EvalJSON(b.ctx, row) + value, isnull, err := b.args[i+1].EvalJSON(b.ctx, row) if err != nil { return res, true, err } @@ -788,11 +785,11 @@ func (b *builtinJSONArrayAppendSig) evalJSON(row chunk.Row) (res json.BinaryJSON obj = json.MergeBinary([]json.BinaryJSON{obj, value}) res, err = res.Modify([]json.PathExpression{pathExpr}, []json.BinaryJSON{obj}, json.ModifySet) if err != nil { - // err should always be nil, the function should never return here. + // We checked pathExpr in the same way as res.Modify do. + // So err should always be nil, the function should never return here. return res, true, err } } - // res won't be nil, error should be nil. return res, false, nil } diff --git a/expression/builtin_json_test.go b/expression/builtin_json_test.go index f5507ca9eea15..67f716e645ecf 100644 --- a/expression/builtin_json_test.go +++ b/expression/builtin_json_test.go @@ -738,30 +738,31 @@ func (s *testEvaluatorSuite) TestJSONArrayAppend(c *C) { f, err := fc.getFunction(s.ctx, s.datumsToConstants(args)) // No error should return in getFunction if t.err is nil. if err != nil { + c.Assert(t.err, NotNil) c.Assert(t.err.Equal(err), Equals, true) continue } + c.Assert(f, NotNil) d, err := evalBuiltinFunc(f, chunk.Row{}) comment := Commentf("case:%v \n input:%v \n output: %s \n expected: %v \n warnings: %v \n expected error %v", i, t.input, d.GetMysqlJSON(), t.expected, s.ctx.GetSessionVars().StmtCtx.GetWarnings(), t.err) - if t.err == nil { - c.Assert(err, IsNil, comment) - c.Assert(int(s.ctx.GetSessionVars().StmtCtx.WarningCount()), Equals, 0, comment) - if t.expected == nil { - c.Assert(d.IsNull(), IsTrue, comment) - } else { - var j1 json.BinaryJSON - j1, err = json.ParseBinaryFromString(t.expected.(string)) - c.Assert(err, IsNil) - j2 := d.GetMysqlJSON() - var cmp int - cmp = json.CompareBinary(j1, j2) - c.Assert(err, IsNil, comment) - c.Assert(cmp, Equals, 0, comment) - } - } else { + if t.err != nil { c.Assert(t.err.Equal(err), Equals, true, comment) + continue + } + + c.Assert(err, IsNil, comment) + c.Assert(int(s.ctx.GetSessionVars().StmtCtx.WarningCount()), Equals, 0, comment) + + if t.expected == nil { + c.Assert(d.IsNull(), IsTrue, comment) + continue } + + j1, err := json.ParseBinaryFromString(t.expected.(string)) + + c.Assert(err, IsNil, comment) + c.Assert(json.CompareBinary(j1, d.GetMysqlJSON()), Equals, 0, comment) } }