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 some bugs in builtinCastIntAsRealSig and builtinCastIntAsDecimalSig that treat unsigned val as signed val #11745

Closed
wants to merge 2 commits into from

Conversation

H-ZeX
Copy link
Contributor

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

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

What problem does this PR solve?

in the original version, it will check inUnion && val<0 no matter whether the val is unsigned(the val is always int64, but in sql, it may be unsigned), so in some case, it will dirrerent from mysql5.7

What is changed and how it works?

add check of whether unsigned

Check List

  • Integration test

Code changes

no.

Side effects

no.

Related changes

  • Need to cherry-pick to the release branch

@H-ZeX H-ZeX force-pushed the fix-unsigned-bug branch from dbda87d to bdc8fd9 Compare August 15, 2019 05:29
@H-ZeX H-ZeX changed the title find some bugs in builtinCastIntAsRealSig and builtinCastIntAsDecimalSig that treat unsigned val as signed val fix some bugs in builtinCastIntAsRealSig and builtinCastIntAsDecimalSig that treat unsigned val as signed val Aug 15, 2019
Signed-off-by: H-ZeX <hzx20112012@gmail.com>
@H-ZeX H-ZeX force-pushed the fix-unsigned-bug branch from bdc8fd9 to 93d0bae Compare August 15, 2019 06:34
Signed-off-by: H-ZeX <hzx20112012@gmail.com>
@codecov
Copy link

codecov bot commented Aug 15, 2019

Codecov Report

Merging #11745 into master will decrease coverage by 0.0068%.
The diff coverage is 50%.

@@               Coverage Diff               @@
##             master    #11745        +/-   ##
===============================================
- Coverage   81.4489%   81.442%   -0.0069%     
===============================================
  Files           433       433                
  Lines         93240     93184        -56     
===============================================
- Hits          75943     75891        -52     
+ Misses        11867     11865         -2     
+ Partials       5430      5428         -2

1 similar comment
@codecov
Copy link

codecov bot commented Aug 15, 2019

Codecov Report

Merging #11745 into master will decrease coverage by 0.0068%.
The diff coverage is 50%.

@@               Coverage Diff               @@
##             master    #11745        +/-   ##
===============================================
- Coverage   81.4489%   81.442%   -0.0069%     
===============================================
  Files           433       433                
  Lines         93240     93184        -56     
===============================================
- Hits          75943     75891        -52     
+ Misses        11867     11865         -2     
+ Partials       5430      5428         -2

Copy link
Member

@zz-jason zz-jason left a comment

Choose a reason for hiding this comment

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

LGTM

@zz-jason zz-jason added the status/LGT1 Indicates that a PR has LGTM 1. label Aug 20, 2019
@zz-jason zz-jason requested a review from qw4990 August 20, 2019 17:13
@@ -463,7 +463,7 @@ func (b *builtinCastIntAsRealSig) evalReal(row chunk.Row) (res float64, isNull b
}
if !mysql.HasUnsignedFlag(b.tp.Flag) && !mysql.HasUnsignedFlag(b.args[0].GetType().Flag) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Prefer to:

if unsigned := mysql.HasUnsignedFlag(b.args[0].GetType().Flag); !mysql.HasUnsignedFlag(b.tp.Flag) && !unsigned {
} else if b.inUnion && !unsigned {
} else {
}

@@ -489,7 +489,7 @@ func (b *builtinCastIntAsDecimalSig) evalDecimal(row chunk.Row) (res *types.MyDe
}
if !mysql.HasUnsignedFlag(b.tp.Flag) && !mysql.HasUnsignedFlag(b.args[0].GetType().Flag) {
res = types.NewDecFromInt(val)
} else if b.inUnion && val < 0 {
} else if b.inUnion && !mysql.HasUnsignedFlag(b.args[0].GetType().Flag) && val < 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/expression status/LGT1 Indicates that a PR has LGTM 1. type/bugfix This PR fixes a bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants