-
Notifications
You must be signed in to change notification settings - Fork 489
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
*: fix upper-cased charset and collation name #301
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
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.
Rest LGTM
a5f2f21
to
88dcc69
Compare
addressed, PTAL @lonng @crazycs520 |
// TiDB always suppose all charsets / collations as lower-cased and try to convert them if they're not. | ||
// However, the convert is missed in some scenarios before v2.1.9, so for all those tables prior to TableInfoVersion3, their | ||
// charsets / collations will be converted to lower-case while loading from the storage. | ||
TableInfoVersion3 = uint16(3) |
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.
Should we update column version too?
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.
IMHO it's not necessary, after all string case convert is not a heavy opeartion, using only TableInfoVersion
makes the codes simple and clear.
charset/charset_test.go
Outdated
@@ -165,4 +178,5 @@ func BenchmarkGetCharsetDesc(b *testing.B) { | |||
for i := 0; i < b.N; i++ { | |||
GetCharsetDesc(cs) | |||
} | |||
|
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.
remove this blank line.
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.
done
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.
Reset LGTM
88dcc69
to
1a62285
Compare
hi @crazycs520 , please check my replys for your comments, thanks! |
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
What problem does this PR solve?
This PR aims to fix the following 3 potential issues:
Validate collate name in parser and convert the name to lower case, as how charset is handled:
parser/parser.y
Line 5887 in e2cdb85
Added a new
TableInfoVersion3
Added
utf8mb4_0900_ai_ci
inmysql
package, as a tiny fix for suspend unknown collation id 255 error #294What is changed and how it works?
As stated above
Check List
Tests
Code changes
Side effects