New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

expression: rewrite builtin JSON functions under the new framework. #4367

Merged
merged 51 commits into from Sep 6, 2017

Conversation

Projects
None yet
4 participants
@hicqu
Contributor

hicqu commented Aug 29, 2017

to #4080

@hicqu hicqu changed the title from expression: rewrite json functions to new framework. to [WIP] expression: rewrite json functions to new framework. Aug 29, 2017

@hicqu hicqu added the status/WIP label Aug 29, 2017

@hicqu hicqu changed the title from [WIP] expression: rewrite json functions to new framework. to expression: rewrite json functions to new framework. Aug 30, 2017

@hicqu hicqu removed the status/WIP label Aug 30, 2017

@hicqu hicqu requested review from XuHuaiyu, breeswish and zz-jason Aug 30, 2017

hicqu added some commits Aug 30, 2017

fix
Show outdated Hide outdated expression/builtin_json.go
Show outdated Hide outdated expression/builtin_json.go
Show outdated Hide outdated expression/builtin_json.go
Show outdated Hide outdated expression/builtin_json.go
Show outdated Hide outdated expression/builtin_json.go
}
return JSONUnquote(args, b.ctx.GetSessionVars().StmtCtx)

This comment has been minimized.

@zz-jason

zz-jason Aug 31, 2017

Member

ditto

@zz-jason
Show outdated Hide outdated expression/builtin_json.go
if err != nil {
return d, errors.Trace(err)
}
return JSONSet(args, b.ctx.GetSessionVars().StmtCtx)

This comment has been minimized.

@zz-jason

zz-jason Aug 31, 2017

Member

ditto

@zz-jason
Show outdated Hide outdated expression/builtin_json.go
if err != nil {
return d, errors.Trace(err)
}
return JSONInsert(args, b.ctx.GetSessionVars().StmtCtx)

This comment has been minimized.

@zz-jason

zz-jason Aug 31, 2017

Member

ditto

@zz-jason

@zz-jason zz-jason changed the title from expression: rewrite json functions to new framework. to expression: rewrite builtin JSON functions under the new framework. Aug 31, 2017

@zz-jason zz-jason referenced this pull request Aug 31, 2017

Closed

rewrite builtin functions #4080

150 of 150 tasks complete
Show outdated Hide outdated expression/builtin_json.go
Show outdated Hide outdated expression/builtin_json.go
Show outdated Hide outdated expression/builtin_json.go
}
return JSONUnquote(args, b.ctx.GetSessionVars().StmtCtx)

This comment has been minimized.

@XuHuaiyu

XuHuaiyu Aug 31, 2017

Contributor

ditto

@XuHuaiyu

XuHuaiyu Aug 31, 2017

Contributor

ditto

Show outdated Hide outdated expression/builtin_json.go
Show outdated Hide outdated expression/builtin_json.go
@zz-jason

This comment has been minimized.

Show comment
Hide comment
@zz-jason

zz-jason Aug 31, 2017

Member

how about add comments for every JSON function to explain what it is used for ?
references: 12.16.1 JSON Function Reference

Member

zz-jason commented Aug 31, 2017

how about add comments for every JSON function to explain what it is used for ?
references: 12.16.1 JSON Function Reference

hicqu added some commits Aug 31, 2017

zz-jason and others added some commits Aug 31, 2017

Show outdated Hide outdated expression/builtin_json.go
Show outdated Hide outdated expression/builtin_json.go
Show outdated Hide outdated expression/builtin_json.go
Show outdated Hide outdated expression/builtin_cast.go

hicqu added some commits Sep 1, 2017

@hicqu

This comment has been minimized.

Show comment
Hide comment
@hicqu

hicqu Sep 1, 2017

Contributor

@XuHuaiyu @zz-jason , PTAL, thanks

Contributor

hicqu commented Sep 1, 2017

@XuHuaiyu @zz-jason , PTAL, thanks

Show outdated Hide outdated expression/builtin_json.go
args, err := b.evalArgs(row)
if err != nil {
return d, errors.Trace(err)
func (b *builtinJSONExtractSig) evalJSON(row []types.Datum) (res json.JSON, isNull bool, err error) {

This comment has been minimized.

@zz-jason

zz-jason Sep 5, 2017

Member

you can remove the return variable name and avoid variable declarations, like var s string or var found bool, in this function

@zz-jason

zz-jason Sep 5, 2017

Member

you can remove the return variable name and avoid variable declarations, like var s string or var found bool, in this function

This comment has been minimized.

@hicqu

hicqu Sep 5, 2017

Contributor

fixed.

@hicqu

hicqu Sep 5, 2017

Contributor

fixed.

Show outdated Hide outdated expression/builtin_json.go
@hicqu

This comment has been minimized.

Show comment
Hide comment
@hicqu

hicqu Sep 5, 2017

Contributor

@XuHuaiyu @zz-jason , PTAL, thanks.

Contributor

hicqu commented Sep 5, 2017

@XuHuaiyu @zz-jason , PTAL, thanks.

@iamxy

This comment has been minimized.

Show comment
Hide comment
@iamxy

iamxy Sep 5, 2017

Member

/rebuild

Member

iamxy commented Sep 5, 2017

/rebuild

@iamxy

This comment has been minimized.

Show comment
Hide comment
@iamxy

iamxy Sep 5, 2017

Member

/run-all-test

Member

iamxy commented Sep 5, 2017

/run-all-test

@@ -955,6 +955,6 @@ var funcs = map[string]functionClass{
ast.JSONReplace: &jsonReplaceFunctionClass{baseFunctionClass{ast.JSONReplace, 3, -1}},
ast.JSONRemove: &jsonRemoveFunctionClass{baseFunctionClass{ast.JSONRemove, 2, -1}},
ast.JSONMerge: &jsonMergeFunctionClass{baseFunctionClass{ast.JSONMerge, 2, -1}},
ast.JSONObject: &jsonObjectFunctionClass{baseFunctionClass{ast.JSONObject, 2, -1}},
ast.JSONArray: &jsonArrayFunctionClass{baseFunctionClass{ast.JSONArray, 1, -1}},
ast.JSONObject: &jsonObjectFunctionClass{baseFunctionClass{ast.JSONObject, 0, -1}},

This comment has been minimized.

@XuHuaiyu

XuHuaiyu Sep 5, 2017

Contributor

bug-fix?

@XuHuaiyu

XuHuaiyu Sep 5, 2017

Contributor

bug-fix?

This comment has been minimized.

@hicqu

hicqu Sep 5, 2017

Contributor

yes, according document, the can be called without arguments.

@hicqu

hicqu Sep 5, 2017

Contributor

yes, according document, the can be called without arguments.

Show outdated Hide outdated expression/builtin_json.go
Show outdated Hide outdated expression/integration_test.go
Show outdated Hide outdated expression/integration_test.go
{`"a"`, `a`},
{`3`, `3`},
{`{"a": "b"}`, `{"a":"b"}`},
{`{"a": "b"}`, `{"a": "b"}`},
{`{"a": "b"}`, `{"a": "b"}`},
{`"hello,\"quoted string\",world"`, `hello,"quoted string",world`},

This comment has been minimized.

@XuHuaiyu

XuHuaiyu Sep 5, 2017

Contributor

since we remove prefix and suffix '"' and ''' in this PR,
why can this case run successfully before?

@XuHuaiyu

XuHuaiyu Sep 5, 2017

Contributor

since we remove prefix and suffix '"' and ''' in this PR,
why can this case run successfully before?

This comment has been minimized.

@hicqu

hicqu Sep 5, 2017

Contributor

Priviously when call json_unquote on String, we parse it. That will remove prefix and suffix " when the string is a valid JSON.

@hicqu

hicqu Sep 5, 2017

Contributor

Priviously when call json_unquote on String, we parse it. That will remove prefix and suffix " when the string is a valid JSON.

hicqu added some commits Sep 5, 2017

@hicqu

This comment has been minimized.

Show comment
Hide comment
@hicqu

hicqu Sep 6, 2017

Contributor

@XuHuaiyu , PTAL again, thanks!

Contributor

hicqu commented Sep 6, 2017

@XuHuaiyu , PTAL again, thanks!

@XuHuaiyu

This comment has been minimized.

Show comment
Hide comment
@XuHuaiyu

XuHuaiyu Sep 6, 2017

Contributor

/run-all-test

Contributor

XuHuaiyu commented Sep 6, 2017

/run-all-test

@XuHuaiyu XuHuaiyu added the status/LGT2 label Sep 6, 2017

@XuHuaiyu

LGTM

@hicqu hicqu merged commit 0b04d1c into master Sep 6, 2017

9 of 10 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
ci/circleci Your tests passed on CircleCI!
Details
jenkins-ci-tidb/build Jenkins job succeeded.
Details
jenkins-ci-tidb/common-test Jenkins job succeeded.
Details
jenkins-ci-tidb/integration-common-test Jenkins job succeeded.
Details
jenkins-ci-tidb/integration-ddl-test Jenkins job succeeded.
Details
jenkins-ci-tidb/mybatis-test Jenkins job succeeded.
Details
jenkins-ci-tidb/sqllogic-test Jenkins job succeeded.
Details
jenkins-ci-tidb/unit-test Jenkins job succeeded.
Details
license/cla Contributor License Agreement is signed.
Details

@hicqu hicqu deleted the qupeng/json-functions branch Sep 6, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment