Skip to content

types, expression, codec: agg JSON values#21656

Merged
ti-srebot merged 2 commits intopingcap:masterfrom
morgo:fix-json-group-by
Dec 11, 2020
Merged

types, expression, codec: agg JSON values#21656
ti-srebot merged 2 commits intopingcap:masterfrom
morgo:fix-json-group-by

Conversation

@morgo
Copy link
Contributor

@morgo morgo commented Dec 11, 2020

What problem does this PR solve?

Issue Number: Fix #10467

What is changed and how it works?

MySQL can group the JSON values 3 and 3.0 together in a group by statement, versus TiDB which currently handles these separately.

This picks up the PR #15973 , but modifies it to only convert values to float when safe to do so. I think ideally the DECIMAL type should be used to compare values with fixed-point math, which depends on #9988

This means that there are some cases (which are left as comments in the integration test) which are handled correctly in MySQL, but not yet in TiDB.

Related changes

  • Need to cherry-pick to the release branch

Check List

Tests

  • Integration test

Side effects

  • Performance regression

(It's a little bit slower to do this of course.)

Release note

  • TiDB now attemps to compare integer and float values as equal when aggregating JSON values.

@morgo morgo requested a review from a team as a code owner December 11, 2020 01:01
@morgo morgo requested review from qw4990 and removed request for a team December 11, 2020 01:01
@ghost
Copy link

ghost commented Dec 11, 2020

There is no reward for this challenge pull request, so you can request a reward from @qw4990.

Details

Tip : About reward you can refs to reward-command.

Warning: None

@morgo
Copy link
Contributor Author

morgo commented Dec 11, 2020

/run-all-tests

@ichn-hu ichn-hu mentioned this pull request Dec 11, 2020
@XuHuaiyu XuHuaiyu added the type/bugfix This PR fixes a bug. label Dec 11, 2020
Copy link
Contributor

@lzmhhh123 lzmhhh123 left a comment

Choose a reason for hiding this comment

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

LGTM

@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Dec 11, 2020
Copy link
Contributor

@XuHuaiyu XuHuaiyu left a comment

Choose a reason for hiding this comment

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

LGTM

@ti-srebot ti-srebot removed the status/LGT1 Indicates that a PR has LGTM 1. label Dec 11, 2020
@ti-srebot ti-srebot added the status/LGT2 Indicates that a PR has LGTM 2. label Dec 11, 2020
@XuHuaiyu
Copy link
Contributor

/merge

@ti-srebot ti-srebot added the status/can-merge Indicates a PR has been approved by a committer. label Dec 11, 2020
@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot ti-srebot merged commit 43d9293 into pingcap:master Dec 11, 2020
@breezewish
Copy link
Member

I think TiKV need to be fixed as well

@morgo
Copy link
Contributor Author

morgo commented Dec 14, 2020

I think TiKV need to be fixed as well

I've re-opened the original issue to investigate. But in my testing, I've not been able to reproduce yet.

@morgo morgo deleted the fix-json-group-by branch December 14, 2020 02:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component/expression status/can-merge Indicates a PR has been approved by a committer. 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.

Incorrect GROUP BY for JSON values

5 participants