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

expression: remove unnecessary convertIntToUint. fix error in err msg(should be bigint but not double) #11640

Merged
merged 6 commits into from
Aug 12, 2019

Conversation

H-ZeX
Copy link
Contributor

@H-ZeX H-ZeX commented Aug 6, 2019

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

What problem does this PR solve?

In some of the code, we get a uint from the row which is stored in int, however, we needn't casting it from int to uint when it is uint, this will make some bugs.

In the origin builtinCastStringAsDecimalSig::evalDecimal, the res is always not neg, this is a bug.

What is changed and how it works?

Check List

Tests

  • Unit test
  • Integration test

Code changes

NO.

Side effects

NO.

Related changes

  • Need to cherry-pick to the release branch
  • Need to be included in the release note

Signed-off-by: H-ZeX <hzx20112012@gmail.com>
Signed-off-by: H-ZeX <hzx20112012@gmail.com>
@H-ZeX H-ZeX force-pushed the fix-bug-of-builtin_cast branch from d262020 to 746396e Compare August 10, 2019 06:07
@codecov
Copy link

codecov bot commented Aug 10, 2019

Codecov Report

Merging #11640 into master will decrease coverage by 0.459%.
The diff coverage is 87.5%.

@@               Coverage Diff                @@
##             master     #11640        +/-   ##
================================================
- Coverage   81.6732%   81.2142%   -0.4591%     
================================================
  Files           429        426         -3     
  Lines         93688      91830      -1858     
================================================
- Hits          76518      74579      -1939     
- Misses        11772      11889       +117     
+ Partials       5398       5362        -36

@codecov
Copy link

codecov bot commented Aug 10, 2019

Codecov Report

Merging #11640 into master will decrease coverage by 0.4113%.
The diff coverage is 88.8888%.

@@               Coverage Diff               @@
##             master    #11640        +/-   ##
===============================================
- Coverage   81.8533%   81.442%   -0.4114%     
===============================================
  Files           430       429         -1     
  Lines         94800     92618      -2182     
===============================================
- Hits          77597     75430      -2167     
- Misses        11773     11798        +25     
+ Partials       5430      5390        -40

Copy link
Contributor

@lonng lonng 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 Aug 10, 2019
@zz-jason zz-jason requested review from XuHuaiyu and qw4990 August 11, 2019 09:37
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.

Address the comment makes this a LGTM~

types/convert.go Outdated
@@ -98,6 +98,8 @@ func IntergerSignedLowerBound(intType byte) int64 {
}

// ConvertFloatToInt converts a float64 value to a int value.
//
Copy link
Contributor

Choose a reason for hiding this comment

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

We can remove this useless line.

H-ZeX added 2 commits August 12, 2019 10:28
Signed-off-by: H-ZeX <hzx20112012@gmail.com>
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

@lonng
Copy link
Contributor

lonng commented Aug 12, 2019

/run-all-tests

@zz-jason zz-jason added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Aug 12, 2019
@zz-jason zz-jason merged commit bc4a324 into pingcap:master Aug 12, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Aug 12, 2019

cherry pick to release-2.1 failed

@sre-bot
Copy link
Contributor

sre-bot commented Aug 12, 2019

cherry pick to release-3.0 in PR #11707

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/expression 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.

5 participants