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

fix ConvertJSONToInt unsigned bug #11483

Merged
merged 9 commits into from
Aug 1, 2019

Conversation

H-ZeX
Copy link
Contributor

@H-ZeX H-ZeX commented Jul 29, 2019

Signed-off-by: H-ZeX hzx20112012@gmail.com

What problem does this PR solve?

The origin ConvertJSONToInt will fail in this sql select convert(a2.a, unsigned int) from (select cast('"9223372036854775808"' as json) as a) as a2;

What is changed and how it works?

call StrToInt or StrToUint according to the param unsigned.

Check List

Tests

  • Unit test
  • Integration test

Code changes

No

Side effects

No.

Related changes

No.

Signed-off-by: H-ZeX <hzx20112012@gmail.com>
@H-ZeX
Copy link
Contributor Author

H-ZeX commented Jul 29, 2019

/test

@H-ZeX
Copy link
Contributor Author

H-ZeX commented Jul 29, 2019

/run-all-tests

@@ -2180,6 +2180,7 @@ func (s *testIntegrationSuite) TestBuiltin(c *C) {
result.Check(testkit.Rows("2017-01-01 00:00:00.00"))
result = tk.MustQuery(`select cast(20170118.999 as datetime);`)
result.Check(testkit.Rows("2017-01-18 00:00:00"))
tk.MustExec(`select convert(a2.a, unsigned int) from (select cast('"9223372036854775808"' as json) as a) as a2;`)
Copy link
Contributor

Choose a reason for hiding this comment

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

s/MustExec/MustQuery/

Otherwise, you will find you can pass this test in master too.

Copy link
Contributor

@AilinKid AilinKid Jul 30, 2019

Choose a reason for hiding this comment

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

s/MustExec/MustQuery/

Otherwise, you will find you can pass this test in master too.

What's difference between this two MustXXX operations?

@codecov
Copy link

codecov bot commented Jul 29, 2019

Codecov Report

Merging #11483 into master will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master     #11483   +/-   ##
===========================================
  Coverage   81.3173%   81.3173%           
===========================================
  Files           425        425           
  Lines         91668      91668           
===========================================
  Hits          74542      74542           
  Misses        11771      11771           
  Partials       5355       5355

Copy link
Contributor

@crazycs520 crazycs520 left a comment

Choose a reason for hiding this comment

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

LGTM

@H-ZeX H-ZeX added the status/LGT1 Indicates that a PR has LGTM 1. label Jul 30, 2019
Copy link
Contributor

@AilinKid AilinKid left a comment

Choose a reason for hiding this comment

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

/LGTM

@AilinKid AilinKid added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Jul 30, 2019
@winkyao winkyao added the status/can-merge Indicates a PR has been approved by a committer. label Jul 30, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Jul 30, 2019

/run-all-tests

@qw4990 qw4990 merged commit 1c0b366 into pingcap:master Aug 1, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Aug 1, 2019

cherry pick to release-2.1 failed

@sre-bot
Copy link
Contributor

sre-bot commented Aug 1, 2019

cherry pick to release-3.0 in PR #11551

H-ZeX added a commit to H-ZeX/tidb that referenced this pull request Oct 30, 2019
Signed-off-by: H-ZeX <hzx20112012@gmail.com>
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.

8 participants