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: fix the corner case of CAST int as unsigned real/decimal #13637

Merged
merged 13 commits into from
Nov 26, 2019

Conversation

Deardrops
Copy link
Contributor

@Deardrops Deardrops commented Nov 20, 2019

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

This PR continue unfinished work in #11745.

What is changed and how it works?

add check of whether unsigned

Check List

Tests

  • Integration test

Code changes

  • None

Side effects

  • None

Related changes

  • Need to cherry-pick to the release branch

Release note

  • None

H-ZeX and others added 4 commits August 15, 2019 14:32
Signed-off-by: H-ZeX <hzx20112012@gmail.com>
Signed-off-by: H-ZeX <hzx20112012@gmail.com>
@Deardrops Deardrops requested a review from a team as a code owner November 20, 2019 12:38
@ghost ghost requested review from qw4990 and SunRunAway and removed request for a team November 20, 2019 12:38
@Deardrops Deardrops added component/expression needs-cherry-pick-2.1 priority/release-blocker This issue blocks a release. Please solve it ASAP. type/bugfix This PR fixes a bug. labels Nov 20, 2019
@Deardrops
Copy link
Contributor Author

/run-all-tests

@codecov
Copy link

codecov bot commented Nov 20, 2019

Codecov Report

Merging #13637 into master will decrease coverage by 0.2544%.
The diff coverage is 83.3333%.

@@               Coverage Diff                @@
##             master     #13637        +/-   ##
================================================
- Coverage   80.3222%   80.0678%   -0.2545%     
================================================
  Files           474        473         -1     
  Lines        117605     116465      -1140     
================================================
- Hits          94463      93251      -1212     
- Misses        15759      15854        +95     
+ Partials       7383       7360        -23

Copy link
Contributor

@djshow832 djshow832 left a comment

Choose a reason for hiding this comment

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

LGTM

@djshow832 djshow832 added the status/LGT1 Indicates that a PR has LGTM 1. label Nov 20, 2019
@Deardrops Deardrops removed the priority/release-blocker This issue blocks a release. Please solve it ASAP. label Nov 21, 2019
@Deardrops Deardrops requested review from winoros and removed request for winoros November 21, 2019 02:31
@Deardrops
Copy link
Contributor Author

@qw4990 PTAL

@sre-bot
Copy link
Contributor

sre-bot commented Nov 23, 2019

@djshow832, @SunRunAway, @qw4990, PTAL.

expression/builtin_cast.go Outdated Show resolved Hide resolved
expression/builtin_cast.go Outdated Show resolved Hide resolved
expression/builtin_cast.go Outdated Show resolved Hide resolved
@Deardrops
Copy link
Contributor Author

/run-all-tests

@Deardrops Deardrops added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Nov 26, 2019
@Deardrops
Copy link
Contributor Author

/run-all-tests

@@ -461,9 +461,10 @@ func (b *builtinCastIntAsRealSig) evalReal(row chunk.Row) (res float64, isNull b
if isNull || err != nil {
return res, isNull, err
}
if !mysql.HasUnsignedFlag(b.tp.Flag) && !mysql.HasUnsignedFlag(b.args[0].GetType().Flag) {
if unsignedArgs0 := mysql.HasUnsignedFlag(b.args[0].GetType().Flag); !mysql.HasUnsignedFlag(b.tp.Flag) && !unsignedArgs0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

How about:

	} else if b.inUnion && !unsignedArgs0 && val < 0 {
		// Round up to 0 if the value is negative but the expression eval type is unsigned in `UNION` statement
		// NOTE: the following expressions are equal (so choose the more efficient one):
		// `b.inUnion && mysql.HasUnsignedFlag(b.tp.Flag) && !unsignedArgs0 && val < 0`
		// `b.inUnion && !unsignedArgs0 && val < 0`
		res = 0
	} else {

@lonng lonng changed the title fix some bugs in builtinCastIntAsRealSig and builtinCastIntAsDecimalSig that treat unsigned val as signed val expression: fix the corner case of CAST int as unsigned real/decimal Nov 26, 2019
@Deardrops
Copy link
Contributor Author

/run-all-tests

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

@lonng
Copy link
Contributor

lonng commented Nov 26, 2019

/merge

@sre-bot sre-bot added the status/can-merge Indicates a PR has been approved by a committer. label Nov 26, 2019
@lonng lonng merged commit 6a17f90 into pingcap:master Nov 26, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Nov 26, 2019

cherry pick to release-2.1 failed

@sre-bot
Copy link
Contributor

sre-bot commented Nov 26, 2019

cherry pick to release-3.0 failed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/expression status/can-merge Indicates a PR has been approved by a committer. 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.

6 participants