Skip to content
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: add builtin function json_length #7739

Merged
merged 6 commits into from Sep 27, 2018

Conversation

@Kingwl
Copy link
Contributor

commented Sep 18, 2018

What problem does this PR solve?

a part of #7546

What is changed and how it works?

add a built in function json_length

Check List

Tests

  • Unit test
  • Integration test

Code changes

  • Has interface methods change

Related changes

  • Need to be included in the release note
@sre-bot

This comment has been minimized.

Copy link

commented Sep 18, 2018

Hi contributor, thanks for your PR.

This patch needs to be approved by someone of admins. They should reply with "/ok-to-test" to accept this PR for running test automatically.

@CLAassistant

This comment has been minimized.

Copy link

commented Sep 18, 2018

CLA assistant check
All committers have signed the CLA.

@Kingwl Kingwl force-pushed the Kingwl:add_json_length branch 3 times, most recently from efe52c4 to e14c2e6 Sep 18, 2018
@Kingwl Kingwl force-pushed the Kingwl:add_json_length branch from e14c2e6 to 3821748 Sep 18, 2018
@jackysp

This comment has been minimized.

Copy link
Member

commented Sep 20, 2018

/run-all-tests

@coocood

This comment has been minimized.

Copy link
Member

commented Sep 20, 2018

@Kingwl
Tried to run with your branch.
It panics when I execute:

select json_length('{"a": [1, 2, {"aa": "xx"}]}', '$.a[2].aa');
return res, true, json.ErrInvalidJSONPathWildcard
}
var exists bool
obj, exists = obj.Extract([]json.PathExpression{pathExpr})

This comment has been minimized.

Copy link
@mccxj

mccxj Sep 20, 2018

Contributor

the type of extracted obj maybe not the type of array or object.

@Kingwl Kingwl force-pushed the Kingwl:add_json_length branch from 3599f48 to d5f9755 Sep 21, 2018
if obj.Type() != "OBJECT" && obj.Type() != "ARRAY" {
return 1, false, nil
}
return int64(obj.GetElemCount()), false, nil

This comment has been minimized.

Copy link
@mccxj

mccxj Sep 21, 2018

Contributor

this line is useless.

r.Check(testkit.Rows("1"))
r = tk.MustQuery(`select json_length('{"a": 1, "b": 2}')`)
r.Check(testkit.Rows("2"))
r = tk.MustQuery(`select json_length('[1, 2, 3]')`)

This comment has been minimized.

Copy link
@mccxj

mccxj Sep 21, 2018

Contributor

merge testcases to save lines.

@mccxj mccxj referenced this pull request Sep 22, 2018
23 of 28 tasks complete
return 1, false, nil
}

if len(b.args) == 2 {

This comment has been minimized.

Copy link
@zz-jason

zz-jason Sep 25, 2018

Member

how about:

if len(b.args) == 1 {
	return int64(obj.GetElemCount()), false, nil
}
// handle the case that len(b.args) == 2
...

This comment has been minimized.

Copy link
@Kingwl

Kingwl Sep 25, 2018

Author Contributor

actually, I'm ready to extract the common path visit but in a other pr(some other pr are conflicted with that)

@zz-jason

This comment has been minimized.

Copy link
Member

commented Sep 25, 2018

LGTM

@zz-jason zz-jason closed this Sep 25, 2018
@zz-jason zz-jason reopened this Sep 25, 2018
@sre-bot

This comment has been minimized.

Copy link

commented Sep 25, 2018

Hi contributor, thanks for your PR.

This patch needs to be approved by someone of admins. They should reply with "/ok-to-test" to accept this PR for running test automatically.

@zz-jason

This comment has been minimized.

Copy link
Member

commented Sep 25, 2018

@XuHuaiyu PTAL

return res, isNull, errors.Trace(err)
}

if obj.Type() != "OBJECT" && obj.Type() != "ARRAY" {

This comment has been minimized.

Copy link
@winoros

winoros Sep 26, 2018

Member

Could we use Typecode here?

@Kingwl Kingwl force-pushed the Kingwl:add_json_length branch from 14b5b3d to d7ad4e8 Sep 26, 2018
Copy link
Member

left a comment

LGTM

@coocood

This comment has been minimized.

Copy link
Member

commented Sep 26, 2018

/run-all-tests

@zz-jason zz-jason added status/LGT2 and removed status/LGT1 labels Sep 27, 2018
@zz-jason zz-jason merged commit 9fd4072 into pingcap:master Sep 27, 2018
4 checks passed
4 checks passed
ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci-tidb/build Jenkins job succeeded.
Details
license/cla Contributor License Agreement is signed.
Details
@XuHuaiyu XuHuaiyu self-assigned this Sep 27, 2018
@XuHuaiyu

This comment has been minimized.

Copy link
Contributor

commented Sep 27, 2018

/run-integration-ddl-test

@Kingwl Kingwl deleted the Kingwl:add_json_length branch Sep 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.