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.

Sign up for GitHub

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

Jump to bottom

types: fix the bug about the wrong query result for decimal type #22507

Merged
merged 21 commits into from
Mar 9, 2021
Merged

types: fix the bug about the wrong query result for decimal type #22507

merged 21 commits into from
Mar 9, 2021

Conversation

jyz0309
Copy link
Contributor

@jyz0309 jyz0309 commented Jan 25, 2021

What problem does this PR solve?

Issue Number: close #22496

Problem Summary:

MySQL [test]> create table t12(d decimal(15,2));
MySQL [test]> insert into t12 values('1,999.00');
MySQL [test]> select * from t12;
+------+
| d |
+------+
| 1.00 |
+------+
1 row in set (0.00 sec)

The select about decimal type show the wrong result.

What is changed and how it works?

What's Changed:
update the FromString function for MyDecimal for the situation like "1,999.00" and "1,999,999.00"

Related changes

  • Need to cherry-pick to the release branch

Check List

Tests

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

Release note

  • types: fix the bug about the wrong query result for decimal type

@jyz0309
init
a2f2d23 
Signed-off-by: jyz0309 <45495947@qq.com>
@jyz0309
add test
ec57b3f 
Signed-off-by: jyz0309 <45495947@qq.com>
@jyz0309 jyz0309 requested a review from a team as a code owner January 25, 2021 02:35
@jyz0309 jyz0309 requested review from qw4990 and removed request for a team January 25, 2021 02:35
@jyz0309
Copy link
Contributor Author

jyz0309 commented Jan 25, 2021 •
edited
Loading

@qw4990 @lysu PTAL

@jyz0309 jyz0309 requested a review from lysu January 25, 2021 07:29
@qw4990 qw4990 added the type/bugfix This PR fixes a bug. label Jan 26, 2021
@qw4990 qw4990 requested a review from XuHuaiyu January 26, 2021 02:42
@lysu
Copy link
Contributor

lysu commented Jan 26, 2021 •
edited
Loading

image

https://www.db-fiddle.com/

question: does mysql support comma number? @jyz0309 and should we break compatibility to support this - -?

@jyz0309
Copy link
Contributor Author

jyz0309 commented Jan 26, 2021

https://www.db-fiddle.com/

oh, I didn't notice this, my bad-.-

@ichn-hu ichn-hu mentioned this pull request Jan 27, 2021
Closed
@jyz0309
fix bug, return BadNumber
28c1ace 
Signed-off-by: jyz0309 <45495947@qq.com>
@jyz0309
add test
cffe865 
Signed-off-by: jyz0309 <45495947@qq.com>
@jyz0309
fix test
405bb12 
Signed-off-by: jyz0309 <45495947@qq.com>
@jyz0309
Copy link
Contributor Author

jyz0309 commented Jan 27, 2021

@lysu @XuHuaiyu PTAL

lysu
lysu reviewed Jan 28, 2021
View reviewed changes
types/mydecimal.go Outdated Show resolved Hide resolved
types/mydecimal.go Outdated Show resolved Hide resolved
types/convert_test.go Outdated Show resolved Hide resolved
@jyz0309
add test and remove useless change
d05fced 
Signed-off-by: jyz0309 <45495947@qq.com>
@jyz0309
change test func name
b4a73a2 
Signed-off-by: jyz0309 <45495947@qq.com>
@github-actions github-actions bot added the sig/execution SIG execution label Jan 29, 2021
@lysu
Copy link
Contributor

lysu commented Jan 29, 2021

LGTM

@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Jan 29, 2021
@jyz0309
Copy link
Contributor Author

jyz0309 commented Feb 4, 2021

@XuHuaiyu PTAL

XuHuaiyu
XuHuaiyu reviewed Feb 22, 2021
View reviewed changes
executor/write_test.go Outdated Show resolved Hide resolved
@jyz0309
cover sql_mode is null case
c970ac2 
Signed-off-by: jyz0309 <45495947@qq.com>
@ti-chi-bot ti-chi-bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Feb 23, 2021
@jyz0309
fix test
2abdbb7 
Signed-off-by: jyz0309 <45495947@qq.com>
XuHuaiyu
XuHuaiyu reviewed Feb 25, 2021
View reviewed changes
executor/write_test.go Outdated Show resolved Hide resolved
@ti-chi-bot ti-chi-bot removed the status/can-merge Indicates a PR has been approved by a committer. label Feb 26, 2021
@jyz0309
fix test
13391c1 
Signed-off-by: jyz0309 <45495947@qq.com>
@pingcap pingcap deleted a comment from ti-chi-bot Feb 26, 2021
@jyz0309
Copy link
Contributor Author

jyz0309 commented Feb 27, 2021

/run-all-tests

@jyz0309
Copy link
Contributor Author

jyz0309 commented Mar 1, 2021

/run-all-tests

@jyz0309
Copy link
Contributor Author

jyz0309 commented Mar 1, 2021

@lysu @XuHuaiyu it seems that the e2e-test's fault is not occured by this pr?PTAL

@jyz0309
Copy link
Contributor Author

jyz0309 commented Mar 1, 2021

/merge

@ti-chi-bot
Copy link
Member

@jyz0309: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

You only need to trigger /merge once, and if the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

If you have any questions about the PR merge process, please refer to pr process.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

@ti-chi-bot
Copy link
Member

@jyz0309: /merge is only allowed for the committers in list.

In response to this:

/merge

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

@jyz0309
Copy link
Contributor Author

jyz0309 commented Mar 1, 2021

/run-integration-compatibility-test

2 similar comments
@jyz0309
Copy link
Contributor Author

jyz0309 commented Mar 2, 2021

/run-integration-compatibility-test

@jyz0309
Copy link
Contributor Author

jyz0309 commented Mar 2, 2021

/run-integration-compatibility-test

@jyz0309
Copy link
Contributor Author

jyz0309 commented Mar 3, 2021

/run-all-tests

@jyz0309
Copy link
Contributor Author

jyz0309 commented Mar 3, 2021

/run-tics-test

@jyz0309
Copy link
Contributor Author

jyz0309 commented Mar 3, 2021

@XuHuaiyu PTAL

@XuHuaiyu
Copy link
Contributor

XuHuaiyu commented Mar 9, 2021

/merge

@ti-chi-bot
Copy link
Member

This pull request has been accepted and is ready to merge.

Commit hash: a2ea26f

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Mar 9, 2021
@jyz0309
Copy link
Contributor Author

jyz0309 commented Mar 9, 2021

/run-integration-copr-test

@ti-chi-bot ti-chi-bot merged commit aa82a03 into pingcap:master Mar 9, 2021
ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Mar 9, 2021
@jyz0309 @ti-srebot
cherry pick pingcap#22507 to release-4.0
32cf4b1 
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot ti-srebot mentioned this pull request Mar 9, 2021
Merged
@ti-srebot
Copy link
Contributor

cherry pick to release-4.0 in PR #23196

@erwadba erwadba mentioned this pull request Apr 1, 2021
Merged
ti-chi-bot pushed a commit that referenced this pull request May 13, 2021
@ti-srebot
types: fix the bug about the wrong query result for decimal type (#22507
c7bd1d0 
) (#23196)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lysu lysu lysu left review comments

@XuHuaiyu XuHuaiyu XuHuaiyu approved these changes

@qw4990 qw4990 Awaiting requested review from qw4990

Assignees
No one assigned
Labels
component/expression sig/execution SIG execution size/S Denotes a PR that changes 10-29 lines, ignoring generated files. 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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

The wrong query result for decimal type
6 participants
@jyz0309 @lysu @ti-chi-bot @XuHuaiyu @ti-srebot @qw4990