-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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
type: fix float over flow when converting a decimal to a float and then converting a float to an uint #10405
Conversation
Codecov Report
@@ Coverage Diff @@
## master #10405 +/- ##
===============================================
+ Coverage 77.3061% 77.323% +0.0169%
===============================================
Files 412 412
Lines 86715 86718 +3
===============================================
+ Hits 67036 67053 +17
+ Misses 14522 14508 -14
Partials 5157 5157 |
/run-all-tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
/run-all-tests |
1 similar comment
/run-all-tests |
types/convert.go
Outdated
@@ -173,6 +173,48 @@ func ConvertFloatToUint(sc *stmtctx.StatementContext, fval float64, upperBound u | |||
return uint64(val), nil | |||
} | |||
|
|||
// ConvertDecimalToUint converts a decimal to an uint by string instead of float to avoid float overflow (#10181). | |||
func ConvertDecimalToUint(sc *stmtctx.StatementContext, d *MyDecimal, upperBound uint64, tp byte) (uint64, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add UT for this func?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UT is added.
types/convert.go
Outdated
@@ -173,6 +173,48 @@ func ConvertFloatToUint(sc *stmtctx.StatementContext, fval float64, upperBound u | |||
return uint64(val), nil | |||
} | |||
|
|||
// ConvertDecimalToUint converts a decimal to an uint by string instead of float to avoid float overflow (#10181). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ConvertDecimalToUint converts a decimal to a uint by converting it to a string first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated.
types/convert.go
Outdated
intStr = str[:p] | ||
fracStr = str[p+1:] | ||
} | ||
for len(intStr) > 0 && intStr[0] == '0' { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- intSrt = strings.TrimLeft(intStr, "0") ?
- When will there have leading "0"s?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right... I will update it.
for len(intStr) > 0 && intStr[0] == '0' { | ||
intStr = intStr[1:] | ||
} | ||
if sc.ShouldClipToZero() && intStr[0] == '-' { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will len(intStr) == 0
be true here ? If so, intStr[0] == '-' will cause a panic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume that len(intStr) > 0
here because MyDecimal
returns a valid decimal string without scientific notation.
But I will let it support scientific notation for safety.
} | ||
|
||
val, err := strconv.ParseUint(intStr, 10, 64) | ||
if err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return val, err
types/convert.go
Outdated
} | ||
|
||
var round uint64 | ||
if fracStr != "" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we just check fracStr[0] >= "5"
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes.. you are right, I will update it.
/rebuild |
1 similar comment
/rebuild |
All comments have been addressed, PTAL~ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
/run-all-tests |
/run-all-tests |
What problem does this PR solve?
Fix #10181.
When converting
9223372036854775807-0.5
from decimal to uint, we convert it to a float value first and then convert the float value to an uint value.For float,
9223372036854775807-0.5
is too large to let it keep high precision.What is changed and how it works?
Introduce
ConvertDecimalToUint
to convert a decimal to an uint by string instead of float to avoid float overflow.Check List
Tests