-
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
expression: fix cast unsigned integer to decimal #7792
Conversation
Hi contributor, thanks for your PR. This patch needs to be approved by someone of admins. They should reply with "/ok-to-test" to accept this PR for running test automatically. |
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.
fix lgtm
Really thanks!
f = BuildCastFunction(ctx, &Constant{Value: types.NewUintDatum(18446744073709551615), RetType: rt}, ft) | ||
res, err = f.Eval(chunk.Row{}) | ||
c.Assert(err, IsNil) | ||
u, err := res.GetMysqlDecimal().ToUint() |
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.
Here it's more likely checking cast(cast(arg as decimal) as unsigned)
.
Maybe ToString
is better?
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.
It's not like cast(cast(arg as decimal) as unsigned)
, because when cast(arg as decimal) returns signed, res.GetMysqlDecimal().ToUint()
will return range exceed 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.
LGTM
/run-all-tests |
… integer range
What problem does this PR solve?
fix #7778
What is changed and how it works?
in evalDecimal function, it not only check the cast type unsigned flag, but also check the unsigned flag of arg
Check List
Tests