-
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
types: fix mergeTypeFlag to consider UnsignedFlag #17386
Conversation
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
@windtalker PTAL |
c &= ^mysql.UnsignedFlag | ||
} | ||
|
||
return c |
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 think the unsigned flag can not be simply set or unset, because the range of tp is changed when the unsigned flag is changed. When merge unsigned int32
and signed int32
, the best fit type should be signed int64
, and things get more complex when merge unsigned int64
and signed int64
, as far as I know, the latest mysql return decimal
type, so simply set/unset the unsigned flag many cause some unknown issues, this is the reason why I does not check unsigned flag in the last pr. In order to handle the unsigned flag, maybe you need to make some changes in AggFieldType
.
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.
@windtalker Right, for case when
MySQL 8 will use DECIMAL(20, 0)
for signed int64
and unsigned int64
. I'll give a look at the detailed logic.
@smola, please update your pull request. |
1 similar comment
@smola, please update your pull request. |
Closing this PR, since it requires a different approach as @windtalker pointed out. |
What problem does this PR solve?
Issue Number: close #15771
Problem Summary: when merging type flags, unsigned is always set if left hand side is unsigned, but it should be set only if both sides are unsigned.
What is changed and how it works?
field_type.go
,mergeTypeFlag
now considersmysql.UnsignedFlag
. The flag will be set only if it is set for both sides.AggFieldType
, so the fix propagates to this function.Check List
Release note