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

executor: fix group_concat for cases like group_concat(123,null) #9921

Merged
merged 5 commits into from
Mar 28, 2019

Conversation

XuHuaiyu
Copy link
Contributor

What problem does this PR solve?

fix #9920

What is changed and how it works?

Make the behavior of group_concat be compatible with the MySQL manual:

It returns NULL if there are no non-NULL values

Before this commit, we regard every argument as an item of the result of group_concat.
But actually, all the arguments value in one row should be regarded as one item. If any one of the arguments is null, the group_concat result of this row is null.

Check List

Tests

  • Integration test

Code changes

  • Has exported function/method change

Side effects

N/A

Related changes

  • Need to cherry-pick to the release branch

@XuHuaiyu XuHuaiyu added the sig/execution SIG execution label Mar 27, 2019
@XuHuaiyu XuHuaiyu assigned eurekaka, qw4990 and winoros and unassigned eurekaka, qw4990 and winoros Mar 27, 2019
@codecov
Copy link

codecov bot commented Mar 27, 2019

Codecov Report

Merging #9921 into master will increase coverage by 0.0183%.
The diff coverage is 100%.

@@               Coverage Diff                @@
##             master      #9921        +/-   ##
================================================
+ Coverage   77.2162%   77.2346%   +0.0183%     
================================================
  Files           405        405                
  Lines         81668      81668                
================================================
+ Hits          63061      63076        +15     
- Misses        13911      13919         +8     
+ Partials       4696       4673        -23

Copy link
Contributor

@alivxxx alivxxx left a comment

Choose a reason for hiding this comment

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

LGTM

@alivxxx alivxxx added the status/LGT1 Indicates that a PR has LGTM 1. label Mar 28, 2019
@XuHuaiyu XuHuaiyu added the type/bugfix This PR fixes a bug. label Mar 28, 2019
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 Mar 28, 2019
@eurekaka
Copy link
Contributor

/run-all-tests

eurekaka
eurekaka previously approved these changes Mar 28, 2019
@XuHuaiyu
Copy link
Contributor Author

/run-all-tests

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sig/execution SIG execution status/LGT2 Indicates that a PR has LGTM 2. type/bugfix This PR fixes a bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

wrong result of select group_concat(123, null)
6 participants