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

types: fix uint64 overflow bug in ConvertJSONToFloat #11355

Merged
merged 5 commits into from
Jul 25, 2019

Conversation

H-ZeX
Copy link
Contributor

@H-ZeX H-ZeX commented Jul 22, 2019

What problem does this PR solve?

In the origin version ConvertIntToUint(sc, j.GetInt64(), IntergerUnsignedUpperBound(mysql.TypeLonglong), mysql.TypeLonglong) , when j.GetInt64() return neg number such as 1<<63, then ConvertIntToUint may treat it as overflow, this is a bug.

What is changed and how it works?

Replace j.GetInt64 by j.GetUint64

Check List

Tests

  • Unit test

Related changes

  • Need to cherry-pick to the release branch

H-ZeX added 2 commits July 22, 2019 12:08
Signed-off-by: H-ZeX <hzx20112012@gmail.com>
Signed-off-by: H-ZeX <hzx20112012@gmail.com>
@codecov
Copy link

codecov bot commented Jul 22, 2019

Codecov Report

Merging #11355 into master will decrease coverage by 0.2344%.
The diff coverage is 100%.

@@               Coverage Diff               @@
##             master    #11355        +/-   ##
===============================================
- Coverage   81.5114%   81.277%   -0.2345%     
===============================================
  Files           423       423                
  Lines         91278     90210      -1068     
===============================================
- Hits          74402     73320      -1082     
- Misses        11571     11590        +19     
+ Partials       5305      5300         -5

@codecov
Copy link

codecov bot commented Jul 22, 2019

Codecov Report

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

@@             Coverage Diff             @@
##             master     #11355   +/-   ##
===========================================
  Coverage   81.4748%   81.4748%           
===========================================
  Files           424        424           
  Lines         91249      91249           
===========================================
  Hits          74345      74345           
  Misses        11583      11583           
  Partials       5321       5321

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.

.

@@ -553,8 +553,7 @@ func ConvertJSONToFloat(sc *stmtctx.StatementContext, j json.BinaryJSON) (float6
case json.TypeCodeInt64:
return float64(j.GetInt64()), nil
case json.TypeCodeUint64:
u, err := ConvertIntToUint(sc, j.GetInt64(), IntergerUnsignedUpperBound(mysql.TypeLonglong), mysql.TypeLonglong)
Copy link
Contributor

@winkyao winkyao Jul 22, 2019

Choose a reason for hiding this comment

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

Should we consider ShouldClipToZero?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we need to consider this.
we extract a uint from the json, it must not < 0.

// GetInt64 gets the int64 value.
func (bj BinaryJSON) GetInt64() int64 {
	return int64(endian.Uint64(bj.Value))
}

// GetUint64 gets the uint64 value.
func (bj BinaryJSON) GetUint64() uint64 {
	return endian.Uint64(bj.Value)
}

Copy link
Contributor

Choose a reason for hiding this comment

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

need or needn't?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

needn't

@H-ZeX
Copy link
Contributor Author

H-ZeX commented Jul 24, 2019

@winkyao

@bb7133 bb7133 added the type/bugfix This PR fixes a bug. label Jul 24, 2019
@bb7133 bb7133 changed the title Fix uint convert bug types: fix uint64 convert bug Jul 24, 2019
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.

Could you please add some integration tests in integration_test.go?

@bb7133 bb7133 changed the title types: fix uint64 convert bug types: fix uint64 overflow bug in ConvertJSONToFloat Jul 24, 2019
Signed-off-by: H-ZeX <hzx20112012@gmail.com>
Copy link
Contributor

@alivxxx alivxxx left a comment

Choose a reason for hiding this comment

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

LGTM

@alivxxx alivxxx added the status/LGT1 Indicates that a PR has LGTM 1. label Jul 25, 2019
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 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 25, 2019
result = tk.MustQuery(`select cast(20170118.999 as datetime);`)
result.Check(testkit.Rows("2017-01-18 00:00:00"))

tk.MustExec(`create table tb5(a bigint(64) unsigned, b double);`)
Copy link
Member

Choose a reason for hiding this comment

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

why not directly create a json column?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If directly creat json, then I can not get a uint field(#11386 (comment)).

Copy link
Member

Choose a reason for hiding this comment

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

how about casting '9223372036854775808' to uint then cast to json?

@zz-jason zz-jason removed the status/can-merge Indicates a PR has been approved by a committer. label Jul 25, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Jul 25, 2019

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Jul 25, 2019

Ready to merge!

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

sre-bot commented Jul 25, 2019

cherry pick to release-2.1 failed

@sre-bot
Copy link
Contributor

sre-bot commented Jul 25, 2019

cherry pick to release-3.0 in PR #11433

sre-bot added a commit that referenced this pull request Jul 25, 2019
sre-bot pushed a commit that referenced this pull request Jul 26, 2019
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.

7 participants