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_DEPTH #8347

Merged
merged 5 commits into from
Nov 26, 2018

Conversation

pingyu
Copy link
Contributor

@pingyu pingyu commented Nov 18, 2018

What problem does this PR solve?

Add json builtin function JSON_DEPTH described in #7546

What is changed and how it works?

Add struct builtinJSONDepthSig, and add GetElemDepth to BinaryJSON

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)

mysql> SELECT JSON_DEPTH('{}'), JSON_DEPTH('[]'), JSON_DEPTH('true');
+------------------+------------------+--------------------+
| JSON_DEPTH('{}') | JSON_DEPTH('[]') | JSON_DEPTH('true') |
+------------------+------------------+--------------------+
| 1 | 1 | 1 |
+------------------+------------------+--------------------+
1 row in set (0.00 sec)

mysql> SELECT JSON_DEPTH('[10, 20]'), JSON_DEPTH('[[], {}]');
+------------------------+------------------------+
| JSON_DEPTH('[10, 20]') | JSON_DEPTH('[[], {}]') |
+------------------------+------------------------+
| 2 | 2 |
+------------------------+------------------------+
1 row in set (0.00 sec)

mysql> SELECT JSON_DEPTH('[10, {"a": 20}]');
+-------------------------------+
| JSON_DEPTH('[10, {"a": 20}]') |
+-------------------------------+
| 3 |
+-------------------------------+
1 row in set (0.00 sec)

mysql> SELECT JSON_DEPTH(NULL);
+------------------+
| JSON_DEPTH(NULL) |
+------------------+
| NULL |
+------------------+
1 row in set (0.00 sec)

mysql> SELECT JSON_DEPTH('a');
ERROR 3140 (22032): Invalid JSON text: invalid character 'a' looking for beginning of value
mysql> SELECT JSON_DEPTH('{}') 'Result';
+--------+
| Result |
+--------+
| 1 |
+--------+
1 row in set (0.00 sec)

mysql> SELECT JSON_DEPTH('{"Name": "Homer"}') 'Result';
+--------+
| Result |
+--------+
| 2 |
+--------+
1 row in set (0.00 sec)

mysql> SET @DaTa = '{
'> "Person": {
'> "Name": "Homer",
'> "Age": 39,
'> "Hobbies": ["Eating", "Sleeping"]
'> }
'> }';
Query OK, 0 rows affected (0.00 sec)

mysql> SELECT JSON_DEPTH(@DaTa) 'Result';
+--------+
| Result |
+--------+
| 4 |
+--------+
1 row in set (0.00 sec)

Code changes

  • Has exported function/method change
    Add GetElemDepth to BinaryJSON in types/json/binary_functions.go

Side effects

  • NO

Related changes

  • NO

This change is Reviewable

@sre-bot
Copy link
Contributor

sre-bot commented Nov 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
Copy link

CLAassistant commented Nov 18, 2018

CLA assistant check
All committers have signed the CLA.

@shenli shenli added the contribution This PR is from a community contributor. label Nov 18, 2018
@shenli
Copy link
Member

shenli commented Nov 18, 2018

/ok-to-test

@shenli
Copy link
Member

shenli commented Nov 18, 2018

@pingyu Thanks!

@shenli
Copy link
Member

shenli commented Nov 18, 2018

/run-all-tests

@shenli
Copy link
Member

shenli commented Nov 19, 2018

/run-integration-common-test

@pingyu
Copy link
Contributor Author

pingyu commented Nov 19, 2018

@shenli With pleasure. I'm quite new to Go & contributing. Just tell me if I did something wrong. Thank you :)

Copy link
Member

@winoros winoros left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm
Please add tests in expression package too.

@shenli
Copy link
Member

shenli commented Nov 19, 2018

@XuHuaiyu PTAL
@pingyu Please address @winoros's comment.

@pingyu
Copy link
Contributor Author

pingyu commented Nov 19, 2018

@winoros tests in expression package added. PTAL. Thanks.
Modified expression/builtin_json_test.go ONLY. Others codes rebased from latest master.

@pingyu
Copy link
Contributor Author

pingyu commented Nov 20, 2018

@eurekaka PTAL. Thanks.
But I have no idea of how to manually test. And no new UT codes in distsql_builtin_test.go either. No similar codes existed made much difficulties to me. I'm still reading the distsql package.
Any suggestion ?

@eurekaka
Copy link
Contributor

Currently, UT in distsql_builtin_test.go only covers Constant and Column expression, you can reference the TestEval to add the test for ScalarFunction.

@pingyu
Copy link
Contributor Author

pingyu commented Nov 21, 2018

@eurekaka Got it. Thanks.

}

argTps := make([]types.EvalType, 0, len(args))
argTps = append(argTps, types.ETJson)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

function json_depth() only accepts a single argument: https://github.com/pingcap/tidb/blob/master/expression/builtin.go#L596

how about:

bf := newBaseBuiltinFuncWithTp(ctx, args, types.ETInt, types.ETJson)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK.

Improve codes of function `getFunction` of jsonDepthFunctionClass
@pingyu
Copy link
Contributor Author

pingyu commented Nov 24, 2018

@eurekaka @zz-jason PTAL. Thanks.

Copy link
Member

@zz-jason zz-jason left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@eurekaka eurekaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@eurekaka eurekaka added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Nov 26, 2018
@eurekaka
Copy link
Contributor

/run-all-tests

@zz-jason
Copy link
Member

/run-common-test

Copy link
Member

@zz-jason zz-jason left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 3 of 4 files at r1, 3 of 3 files at r3.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @eurekaka)

Copy link
Contributor

@eurekaka eurekaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 3 of 4 files at r1, 3 of 3 files at r3.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @eurekaka)

morgo added a commit to pingcap/docs that referenced this pull request Nov 26, 2018
Just approved, will be merged in minutes: pingcap/tidb#8347
@zz-jason zz-jason merged commit 1f46efe into pingcap:master Nov 26, 2018
lilin90 pushed a commit to pingcap/docs that referenced this pull request Nov 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/expression contribution This PR is from a community contributor. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants