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

*: add builtin aggregate function json_objectagg #11154

Merged
merged 20 commits into from
Feb 4, 2020

Conversation

hg2990656
Copy link
Contributor

@hg2990656 hg2990656 commented Jul 9, 2019

What problem does this PR solve?

#7623

What is changed and how it works?

add an json_objectagg function

Test

mysql> CREATE TABLE `t` (
    ->   `a` int(11) DEFAULT NULL,
    ->   `b` varchar(100) DEFAULT NULL,
    ->   `c` decimal(3,2) DEFAULT NULL,
    ->   `d` json DEFAULT NULL
    -> );
Query OK, 0 rows affected (0.01 sec)
mysql> insert t values(1, "ab", "5.5", '{"id": 1}');
Query OK, 1 row affected (0.00 sec)

mysql> insert t values(1, "cd", "0.5", '{"id": 1}');
Query OK, 1 row affected (0.00 sec)

mysql> insert t values(2, "ef", "2.5", '{"id": 2}');
Query OK, 1 row affected (0.00 sec)
mysql> select json_objectagg(a, b) from t;
+------------------------+
| json_objectagg(a, b)   |
+------------------------+
| {"1": "cd", "2": "ef"} |
+------------------------+
1 row in set (0.00 sec)
mysql> select json_objectagg(b, c) from t;
+-----------------------------------+
| json_objectagg(b, c)              |
+-----------------------------------+
| {"ab": 5.5, "cd": 0.5, "ef": 2.5} |
+-----------------------------------+
1 row in set (0.00 sec)
mysql> select a, json_objectagg(d, b) from t group by a;
+------+-----------------------+
| a    | json_objectagg(d, b)  |
+------+-----------------------+
|    2 | {"{\"id\": 2}": "ef"} |
|    1 | {"{\"id\": 1}": "cd"} |
+------+-----------------------+
2 rows in set (0.00 sec)
mysql> select b, json_objectagg(d, b) from t group by b;
+------+-----------------------+
| b    | json_objectagg(d, b)  |
+------+-----------------------+
| cd   | {"{\"id\": 1}": "cd"} |
| ef   | {"{\"id\": 2}": "ef"} |
| ab   | {"{\"id\": 1}": "ab"} |
+------+-----------------------+
3 rows in set (0.00 sec)

Related changes

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@hg2990656 hg2990656 changed the title Json objectagg add json_objectagg Jul 10, 2019
@zz-jason
Copy link
Member

@hg2990656 Thanks for your contribution, please follow the Contribution Guide to add a proper PR title.

@zz-jason zz-jason added contribution This PR is from a community contributor. sig/execution SIG execution labels Jul 17, 2019
@zz-jason
Copy link
Member

zz-jason commented Aug 3, 2019

@hg2990656 friendly ping, any update?

@sre-bot
Copy link
Contributor

sre-bot commented Aug 3, 2019

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
Copy link
Member

@hg2990656 Hi, it has been a long time since the PR is filed. Any update to fix the CI failure?

@zz-jason zz-jason changed the title add json_objectagg *: add builtin scalar function json_objectagg Sep 1, 2019
@hg2990656
Copy link
Contributor Author

@hg2990656 Hi, it has been a long time since the PR is filed. Any update to fix the CI failure?

The problem is that the json_objectagg aggregate function is not defined in now parser.I have added it in the another PR of parser and I am modifying it, so whether if the parser is updated in the master, then this CI can be no problem

@hg2990656 hg2990656 requested review from a team as code owners December 16, 2019 08:08
@ghost ghost requested review from qw4990, XuHuaiyu, eurekaka, alivxxx and a team and removed request for a team December 16, 2019 08:08
@codecov
Copy link

codecov bot commented Dec 16, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@c36f83e). Click here to learn what that means.
The diff coverage is 0%.

@@             Coverage Diff             @@
##             master     #11154   +/-   ##
===========================================
  Coverage          ?   80.9743%           
===========================================
  Files             ?        422           
  Lines             ?      89763           
  Branches          ?          0           
===========================================
  Hits              ?      72685           
  Misses            ?      11819           
  Partials          ?       5259

@ghost ghost removed their request for review December 16, 2019 08:08
@eurekaka eurekaka removed their request for review December 16, 2019 08:11
@alivxxx alivxxx removed their request for review December 16, 2019 10:26
@hg2990656
Copy link
Contributor Author

Because json_objectagg has two arguments, but the aggfunc_test only support one argument aggfunc now, so I add some functions to test json_objectagg according to now test methods.I am not sure whether it's right for you, if it's all right, I can merge it to origin function to support both single and multiple arguments aggfuncs.
Another problem is the function appendBinary does't support many type now such as []byte, myDecimal, so I wonder to know whether I should add all of them in appendBinary to support create json whose value can be any type in json_objectagg result json.

@hg2990656 hg2990656 force-pushed the json_objectagg branch 2 times, most recently from 1230030 to 6c71a23 Compare January 15, 2020 02:30
@hg2990656
Copy link
Contributor Author

@zz-jason
Because appendBinary in json/binary.go support only servaral types to convert to []byte and append to json.So there seems to be too many types which are needed to support for json_objectagg when add the value to the json.
Another question is that the aggfunc_test doesn't support two arguments aggfuncs now, so I have added some functions to test json_objectagg according to origin test method.If it's right, I can merge them to origin test functions to support both single or multiple argument aggfuncs.

@hg2990656 hg2990656 force-pushed the json_objectagg branch 6 times, most recently from 6e0cd0f to 6133719 Compare February 4, 2020 12:22
@qw4990
Copy link
Contributor

qw4990 commented Feb 4, 2020

Friendly ping, please solve the CI problem:

[2020-02-04T12:33:59.381Z] FAIL: physical_plan_test.go:1148: testPlanSuite.TestDAGPlanBuilderWindowParallel

[2020-02-04T12:33:59.381Z] 

[2020-02-04T12:33:59.381Z] physical_plan_test.go:1159:

[2020-02-04T12:33:59.381Z]     s.doTestDAGPlanBuilderWindow(c, vars, input, output)

[2020-02-04T12:33:59.381Z] physical_plan_test.go:1191:

[2020-02-04T12:33:59.381Z]     c.Assert(err, IsNil)

[2020-02-04T12:33:59.381Z] ... value *errors.withStack = [planner:1815]Internal : Can not find access path matching 'tidb_isolation_read_engines'(value: '[tiflash]') and tidb-server config isolation-read(engines: 'tikv,tiflash,tidb'). Available values are 'tikv'. ("[planner:1815]Internal : Can not find access path matching 'tidb_isolation_read_engines'(value: '[tiflash]') and tidb-server config isolation-read(engines: 'tikv,tiflash,tidb'). Available values are 'tikv'.")

@hg2990656
Copy link
Contributor Author

Friendly ping, please solve the CI problem:

[2020-02-04T12:33:59.381Z] FAIL: physical_plan_test.go:1148: testPlanSuite.TestDAGPlanBuilderWindowParallel

[2020-02-04T12:33:59.381Z] 

[2020-02-04T12:33:59.381Z] physical_plan_test.go:1159:

[2020-02-04T12:33:59.381Z]     s.doTestDAGPlanBuilderWindow(c, vars, input, output)

[2020-02-04T12:33:59.381Z] physical_plan_test.go:1191:

[2020-02-04T12:33:59.381Z]     c.Assert(err, IsNil)

[2020-02-04T12:33:59.381Z] ... value *errors.withStack = [planner:1815]Internal : Can not find access path matching 'tidb_isolation_read_engines'(value: '[tiflash]') and tidb-server config isolation-read(engines: 'tikv,tiflash,tidb'). Available values are 'tikv'. ("[planner:1815]Internal : Can not find access path matching 'tidb_isolation_read_engines'(value: '[tiflash]') and tidb-server config isolation-read(engines: 'tikv,tiflash,tidb'). Available values are 'tikv'.")

This problem seems to be not relative to this aggregate function.What's the error?

Copy link
Contributor

@qw4990 qw4990 left a comment

Choose a reason for hiding this comment

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

LGTM

@qw4990 qw4990 added status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Feb 4, 2020
@sre-bot
Copy link
Contributor

sre-bot commented Feb 4, 2020

Your auto merge job has been accepted, waiting for 14611

@qw4990
Copy link
Contributor

qw4990 commented Feb 4, 2020

@hg2990656 Thanks for your contribution!! This is our Slack Channel about TiDB Execution Engine, on this channel you can discuss anything about TiDB's Execution Engine.
If you are interested in it, welcome to join us :D

@sre-bot
Copy link
Contributor

sre-bot commented Feb 4, 2020

/run-all-tests

@sre-bot sre-bot merged commit ebc6a2d into pingcap:master Feb 4, 2020
hsqlu pushed a commit to hsqlu/tidb that referenced this pull request Feb 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution This PR is from a community contributor. sig/execution SIG execution status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants