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 json_arrayagg and json_objectagg #7824

Closed
wants to merge 5 commits into from

Conversation

Kingwl
Copy link
Contributor

@Kingwl Kingwl commented Sep 30, 2018

What problem does this PR solve?

#7623
#7546

What is changed and how it works?

add an aggr function

Check List

Tests

  • Unit test

This change is Reviewable

@sre-bot
Copy link
Contributor

sre-bot commented Sep 30, 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.

@shenli shenli added contribution This PR is from a community contributor. status/WIP labels Sep 30, 2018
@shenli
Copy link
Member

shenli commented Sep 30, 2018

@Kingwl Thanks!

@Kingwl
Copy link
Contributor Author

Kingwl commented Oct 8, 2018

@shenli could you give some help, please😂

@Kingwl Kingwl force-pushed the json_arrayagg branch 2 times, most recently from 62658af to 6e8d6cc Compare October 11, 2018 02:53
@Kingwl Kingwl changed the title [WIP] expression: add json_arrayagg expression: add json_arrayagg Oct 11, 2018
@zz-jason
Copy link
Member

@Kingwl What problem do you encountered?

@Kingwl Kingwl changed the title expression: add json_arrayagg expression: add json_arrayagg and json_objectagg Oct 12, 2018
@Kingwl
Copy link
Contributor Author

Kingwl commented Oct 12, 2018

where should I add the test?
or only 'integration_test'?

@zz-jason
Copy link
Member

@Kingwl how about adding test in file executor/aggregate_test.go?

@Kingwl
Copy link
Contributor Author

Kingwl commented Oct 12, 2018

@zz-jason seems a big work, could I add the JSON agg function only?😂

@zz-jason
Copy link
Member

@Kingwl sure, it’s also the recommended way. 😄

@Kingwl
Copy link
Contributor Author

Kingwl commented Oct 12, 2018

and the pingcap/tipb#105 has blocked this (and #7776) , could you take a look please?🙋🏻‍♂️

@Kingwl
Copy link
Contributor Author

Kingwl commented Oct 12, 2018

And something can not to be serialized to binary JSON (Decimal), are there any existed way to handle that?

@zz-jason
Copy link
Member

@Kingwl Why decimal can not be serialized to binary json?

@Kingwl
Copy link
Contributor Author

Kingwl commented Oct 12, 2018

json.CreateBinary does not match that, maybe convert all value to []byte? (or string? or any other type)

@Kingwl
Copy link
Contributor Author

Kingwl commented Oct 15, 2018

another question:
what kind should decimal be in json? number ? or string?

@Kingwl
Copy link
Contributor Author

Kingwl commented Nov 9, 2018

@ PTAL @zz-jason

@qw4990 qw4990 self-requested a review March 6, 2019 08:04
@qw4990
Copy link
Contributor

qw4990 commented Mar 11, 2019

Hi, @Kingwl, could you please fix these CI problems and conflicts? Thanks~

@Kingwl
Copy link
Contributor Author

Kingwl commented Mar 11, 2019

sure

@qw4990
Copy link
Contributor

qw4990 commented Mar 29, 2019

PTAL~ @Kingwl

@qw4990 qw4990 removed their request for review April 1, 2019 02:43
@zz-jason
Copy link
Member

zz-jason commented Aug 3, 2019

@Kingwl friendly ping, any update?

@Kingwl
Copy link
Contributor Author

Kingwl commented Aug 3, 2019

Sorry for the delay,I have no time to update with these recently. And the changes seem not completion, Sorry again.

@Kingwl Kingwl closed this Aug 3, 2019
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. sig/execution SIG execution
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants