type: fix float over flow when converting a decimal to a float and then converting a float to an uint #10405

qw4990 · 2019-05-09T08:19:51Z

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

Unit test

codecov · 2019-05-09T08:38:47Z

Codecov Report

Merging #10405 into master will increase coverage by 0.0169%.
The diff coverage is 86.8852%.

@@               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

types/convert.go

zhouqiang-cl · 2019-05-09T14:34:22Z

/run-all-tests

zz-jason

LGTM

zhouqiang-cl · 2019-05-09T17:25:01Z

/run-all-tests

zhouqiang-cl · 2019-05-09T17:33:54Z

/run-all-tests

XuHuaiyu · 2019-05-10T02:38:25Z

types/convert.go

@@ -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) {


Add UT for this func?

UT is added.

XuHuaiyu · 2019-05-10T02:40:47Z

types/convert.go

@@ -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).


ConvertDecimalToUint converts a decimal to a uint by converting it to a string first.

XuHuaiyu · 2019-05-10T02:43:49Z

types/convert.go

+		intStr = str[:p]
+		fracStr = str[p+1:]
+	}
+	for len(intStr) > 0 && intStr[0] == '0' {


intSrt = strings.TrimLeft(intStr, "0") ?

When will there have leading "0"s?

You are right... I will update it.

XuHuaiyu · 2019-05-10T02:46:38Z

types/convert.go

+	for len(intStr) > 0 && intStr[0] == '0' {
+		intStr = intStr[1:]
+	}
+	if sc.ShouldClipToZero() && intStr[0] == '-' {


Will len(intStr) == 0 be true here ? If so, intStr[0] == '-' will cause a panic?

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.

XuHuaiyu · 2019-05-10T02:54:22Z

types/convert.go

+	}
+
+	val, err := strconv.ParseUint(intStr, 10, 64)
+	if err != nil {


return val, err

XuHuaiyu · 2019-05-10T02:58:40Z

types/convert.go

+	}
+
+	var round uint64
+	if fracStr != "" {


Can we just check fracStr[0] >= "5"?

Yes.. you are right, I will update it.

types/convert.go

qw4990 · 2019-05-15T06:54:06Z

/rebuild

zhouqiang-cl · 2019-05-15T06:54:13Z

/rebuild

qw4990 · 2019-05-15T06:59:59Z

All comments have been addressed, PTAL~

types/convert.go

zz-jason

LGTM

need another reviewer 😂

alivxxx

LGTM

qw4990 · 2019-05-15T09:58:54Z

/run-all-tests

qw4990 · 2019-05-15T11:26:56Z

/run-all-tests

…at (pingcap#10405)

qw4990 added type/bugfix This PR fixes a bug. component/expression labels May 9, 2019

qw4990 requested review from XuHuaiyu, alivxxx and zz-jason May 9, 2019 08:52

winoros reviewed May 9, 2019

View reviewed changes

types/convert.go Outdated Show resolved Hide resolved

zz-jason reviewed May 9, 2019

View reviewed changes

zz-jason added the status/LGT1 Indicates that a PR has LGTM 1. label May 9, 2019

XuHuaiyu reviewed May 10, 2019

View reviewed changes

zz-jason reviewed May 15, 2019

View reviewed changes

types/convert.go Outdated Show resolved Hide resolved

qw4990 added 9 commits May 15, 2019 16:43

fix

19e7abe

add UT

8e7630e

update comments

3697382

fix

d8fe6ec

handle scientific notation

8d21e2d

refmt

67a46c8

refmt

1993ff6

fix comments

c57c855

update error

b909e90

qw4990 force-pushed the fix-10181 branch from fc2768c to b909e90 Compare May 15, 2019 08:44

zz-jason previously approved these changes May 15, 2019

View reviewed changes

qw4990 removed the request for review from alivxxx May 15, 2019 08:59

qw4990 requested a review from alivxxx May 15, 2019 08:59

alivxxx reviewed May 15, 2019

View reviewed changes

alivxxx approved these changes May 15, 2019

View reviewed changes

qw4990 added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels May 15, 2019

Merge branch 'master' into fix-10181

842e369

qw4990 merged commit 627bd69 into pingcap:master May 15, 2019

qw4990 changed the title ~~type: fix float over flow when converting a decimal to an uint by float~~ type: fix float over flow when converting a decimal to a float and then converting a float to an uint May 20, 2019

qw4990 added a commit to qw4990/tidb that referenced this pull request Jun 6, 2019

type: fix float over flow when converting a decimal to an uint by flo…

f128b23

…at (pingcap#10405)

qw4990 mentioned this pull request Jun 6, 2019

type: fix float over flow when converting a decimal to a float and then converting a float to an uint (#10405) #10730

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

type: fix float over flow when converting a decimal to a float and then converting a float to an uint #10405

type: fix float over flow when converting a decimal to a float and then converting a float to an uint #10405

qw4990 commented May 9, 2019

codecov bot commented May 9, 2019 •

edited

Loading

zhouqiang-cl commented May 9, 2019

zz-jason left a comment

zhouqiang-cl commented May 9, 2019

zhouqiang-cl commented May 9, 2019

XuHuaiyu May 10, 2019

qw4990 May 15, 2019

XuHuaiyu May 10, 2019

qw4990 May 15, 2019

XuHuaiyu May 10, 2019

qw4990 May 10, 2019

XuHuaiyu May 10, 2019

qw4990 May 10, 2019

XuHuaiyu May 10, 2019

XuHuaiyu May 10, 2019

qw4990 May 10, 2019

qw4990 commented May 15, 2019

zhouqiang-cl commented May 15, 2019

qw4990 commented May 15, 2019

zz-jason left a comment

alivxxx left a comment

qw4990 commented May 15, 2019

qw4990 commented May 15, 2019

type: fix float over flow when converting a decimal to a float and then converting a float to an uint #10405

type: fix float over flow when converting a decimal to a float and then converting a float to an uint #10405

Conversation

qw4990 commented May 9, 2019

What problem does this PR solve?

What is changed and how it works?

Check List

codecov bot commented May 9, 2019 • edited Loading

Codecov Report

zhouqiang-cl commented May 9, 2019

zz-jason left a comment

Choose a reason for hiding this comment

zhouqiang-cl commented May 9, 2019

zhouqiang-cl commented May 9, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qw4990 commented May 15, 2019

zhouqiang-cl commented May 15, 2019

qw4990 commented May 15, 2019

zz-jason left a comment

Choose a reason for hiding this comment

alivxxx left a comment

Choose a reason for hiding this comment

qw4990 commented May 15, 2019

qw4990 commented May 15, 2019

codecov bot commented May 9, 2019 •

edited

Loading