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 error in ConvertJSONToInt error msg #11493

Merged
merged 3 commits into from
Jul 31, 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 version will call overflow with mysql.TypeDouble when json of type float convert to int overflow i64::MAX, however, it is overflow i64::MAX but not f64::MAX.

What is changed and how it works?

replace mysql.TypeDouble with mysql.TypeLonglong.

Check List

  • Unit test

Code changes

No.

Side effects

No.

Related changes

No.

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

codecov bot commented Jul 29, 2019

Codecov Report

Merging #11493 into master will decrease coverage by 0.0195%.
The diff coverage is 50%.

@@               Coverage Diff                @@
##             master     #11493        +/-   ##
================================================
- Coverage   81.3808%   81.3612%   -0.0196%     
================================================
  Files           424        424                
  Lines         90917      90902        -15     
================================================
- Hits          73989      73959        -30     
- Misses        11609      11626        +17     
+ Partials       5319       5317         -2

@codecov
Copy link

codecov bot commented Jul 29, 2019

Codecov Report

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

@@             Coverage Diff             @@
##             master     #11493   +/-   ##
===========================================
  Coverage   81.3489%   81.3489%           
===========================================
  Files           425        425           
  Lines         91389      91389           
===========================================
  Hits          74344      74344           
  Misses        11713      11713           
  Partials       5332       5332

Copy link
Contributor

@winkyao winkyao left a comment

Choose a reason for hiding this comment

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

LGTM

@winkyao winkyao added the status/LGT1 Indicates that a PR has LGTM 1. label Jul 30, 2019
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.

@H-ZeX Could you please add some tests to cover this situation?

Signed-off-by: H-ZeX <hzx20112012@gmail.com>
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.

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

sre-bot commented Jul 31, 2019

/run-all-tests

@sre-bot sre-bot merged commit ab45834 into pingcap:master Jul 31, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Jul 31, 2019

cherry pick to release-3.0 in PR #11534

@sre-bot
Copy link
Contributor

sre-bot commented Jul 31, 2019

cherry pick to release-2.1 failed

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.

4 participants