Skip to content

Commit f9e5715

Browse files
terry1purcellclaude
andcommitted
collate: do not register latin1_swedish_ci yet
Address review feedback on two counts. Registering the collation let GetCollationByName accept it, so DDL could create columns whose sort key TiKV and TiFlash cannot reproduce, and nothing stops an expression carrying that collation from being pushed down. Drop the newCollatorMap and newCollatorIDMap entries so the collation is rejected exactly as before this branch. The collator and its weight table stay, and the unit tests exercise the type directly. That also removes the second problem. IsCICollation gates fast paths in pkg/expression that case-fold with strings.ToLower, which assumes UTF-8: every latin1 byte >= 0x80 becomes U+FFFD, so distinct characters fold together and LOCATE reports matches that do not exist, while the 1-to-3 byte inflation breaks the surrounding rune-offset arithmetic. Folding for a byte-oriented collation has to go through the collator, as locateStringWithCollation already does. Revert the IsCICollation change rather than half-fix it; it belongs with the work that turns the collation on. The collator doc comment now lists what enabling it requires, so the next reader does not have to rediscover it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TgAxo4cK4qCpuLrhroHWy1
1 parent cbba9d8 commit f9e5715

4 files changed

Lines changed: 47 additions & 34 deletions

File tree

pkg/expression/collation_test.go

Lines changed: 7 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -179,7 +179,9 @@ func TestInferCollation(t *testing.T) {
179179
},
180180
// latin1_bin mixed with latin1_swedish_ci. At equal coercibility the _bin
181181
// collation wins regardless of argument order, so latin1_swedish_ci must be
182-
// recognized as the non-bin side (see isBinCollation).
182+
// recognized as the non-bin side (see isBinCollation). The collation is not
183+
// registered yet and so cannot reach these paths from SQL; these cases pin the
184+
// classification so it is not changed by accident before it can be.
183185
{
184186
[]Expression{
185187
newExpression(CoercibilityImplicit, UNICODE, charset.CharsetLatin1, charset.CollationLatin1),
@@ -911,19 +913,11 @@ func TestCompareString(t *testing.T) {
911913

912914
// latin1 collations are byte-oriented, so the arguments are spelled as cp1252
913915
// bytes rather than as Go literals, which the compiler would encode as UTF-8.
914-
require.Equal(t, 0, types.CompareString("a", "A", "latin1_swedish_ci"))
915-
require.Equal(t, 0, types.CompareString("\xE9", "E", "latin1_swedish_ci"))
916-
require.Equal(t, 0, types.CompareString("a ", "a ", "latin1_swedish_ci"))
917-
require.Equal(t, 0, types.CompareString("\xDC", "Y", "latin1_swedish_ci"))
918-
// Å, Ä and Ö sort after Z rather than folding onto A and O.
919-
require.NotEqual(t, 0, types.CompareString("\xC5", "A", "latin1_swedish_ci"))
920-
require.NotEqual(t, 0, types.CompareString("\xD6", "O", "latin1_swedish_ci"))
921-
require.Equal(t, 1, types.CompareString("\xC5", "Z", "latin1_swedish_ci"))
922-
// ß folds to nothing here, unlike in the utf8 collations above.
923-
require.NotEqual(t, 0, types.CompareString("\xDF", "s", "latin1_swedish_ci"))
924-
require.NotEqual(t, 0, types.CompareString("\xDF", "ss", "latin1_swedish_ci"))
925-
916+
// latin1_swedish_ci is not covered here because it is not registered yet, so
917+
// CompareString would silently fall back to another collator; its semantics are
918+
// tested directly in pkg/util/collate.
926919
require.NotEqual(t, 0, types.CompareString("a", "A", "latin1_bin"))
920+
require.NotEqual(t, 0, types.CompareString("\xE9", "E", "latin1_bin"))
927921
require.Equal(t, 0, types.CompareString("a ", "a ", "latin1_bin"))
928922

929923
require.NotEqual(t, 0, types.CompareString("a", "A", "binary"))

pkg/util/collate/collate.go

Lines changed: 2 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -251,9 +251,7 @@ func GetSupportedCollations() []*charset.Collation {
251251
newSupportedCollations := make([]*charset.Collation, 0, len(newCollatorMap))
252252
for name := range newCollatorMap {
253253
// utf8mb4_zh_pinyin_tidb_as_cs is under developing, should not be shown to user.
254-
// latin1_swedish_ci is under developing too: its sort key has no counterpart in
255-
// TiKV/TiFlash yet, so pushed-down comparisons cannot reproduce it.
256-
if name == "utf8mb4_zh_pinyin_tidb_as_cs" || name == "latin1_swedish_ci" {
254+
if name == "utf8mb4_zh_pinyin_tidb_as_cs" {
257255
continue
258256
}
259257
if coll, err := charset.GetCollationByName(name); err != nil {
@@ -313,8 +311,7 @@ func IsDefaultCollationForUTF8MB4(collate string) bool {
313311
func IsCICollation(collate string) bool {
314312
return collate == "utf8_general_ci" || collate == "utf8mb4_general_ci" ||
315313
collate == "utf8_unicode_ci" || collate == "utf8mb4_unicode_ci" || collate == "gbk_chinese_ci" ||
316-
collate == "utf8mb4_0900_ai_ci" || collate == "gb18030_chinese_ci" ||
317-
collate == "latin1_swedish_ci"
314+
collate == "utf8mb4_0900_ai_ci" || collate == "gb18030_chinese_ci"
318315
}
319316

320317
// ConvertAndGetBinCollation converts collation to binary collation
@@ -334,8 +331,6 @@ func ConvertAndGetBinCollation(collate string) string {
334331
return "gbk_bin"
335332
case "gb18030_chinese_ci":
336333
return "gb18030_bin"
337-
case "latin1_swedish_ci":
338-
return "latin1_bin"
339334
}
340335

341336
return collate
@@ -454,8 +449,6 @@ func init() {
454449
newCollatorIDMap[CollationName2ID("ascii_bin")] = &binPaddingCollator{}
455450
newCollatorMap["latin1_bin"] = &binPaddingCollator{}
456451
newCollatorIDMap[CollationName2ID("latin1_bin")] = &binPaddingCollator{}
457-
newCollatorMap["latin1_swedish_ci"] = &latin1SwedishCICollator{}
458-
newCollatorIDMap[CollationName2ID("latin1_swedish_ci")] = &latin1SwedishCICollator{}
459452
newCollatorMap["utf8mb4_bin"] = &binPaddingCollator{}
460453
newCollatorIDMap[CollationName2ID("utf8mb4_bin")] = &binPaddingCollator{}
461454
newCollatorMap["utf8_bin"] = &binPaddingCollator{}

pkg/util/collate/collate_test.go

Lines changed: 25 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -145,13 +145,21 @@ func TestUTF8CollatorKey(t *testing.T) {
145145
testKeyTable(t, collations, tests)
146146
}
147147

148+
// latin1_swedish_ci is deliberately not registered in newCollatorMap yet, so the
149+
// latin1 tests below construct it directly instead of going through GetCollator.
150+
// See the doc comment on latin1SwedishCICollator.
151+
func latin1TestCollators() (collators []Collator, names []string) {
152+
return []Collator{GetCollator("latin1_bin"), &latin1SwedishCICollator{}},
153+
[]string{"latin1_bin", "latin1_swedish_ci"}
154+
}
155+
148156
// The latin1 collators are byte-oriented, so the test data below spells the
149157
// interesting characters as explicit cp1252 bytes ("\xC4" for Ä) rather than as Go
150158
// source literals, which the compiler would encode as UTF-8.
151159
func TestLatin1CollatorCompare(t *testing.T) {
152160
SetNewCollationEnabledForTest(true)
153161
defer SetNewCollationEnabledForTest(false)
154-
collations := []string{"latin1_bin", "latin1_swedish_ci"}
162+
collators, names := latin1TestCollators()
155163
tests := []compareTable{
156164
{"a", "b", []int{-1, -1}},
157165
{"a", "A", []int{1, 0}},
@@ -187,13 +195,18 @@ func TestLatin1CollatorCompare(t *testing.T) {
187195
// limitation rather than endorsing it.
188196
{"\xC3\x84", "a\x84", []int{1, 0}},
189197
}
190-
testCompareTable(t, collations, tests)
198+
for i, collator := range collators {
199+
for _, table := range tests {
200+
comment := fmt.Sprintf("Compare Left: %q Right: %q, Using %v", table.Left, table.Right, names[i])
201+
require.Equal(t, table.Expect[i], collator.Compare(table.Left, table.Right), comment)
202+
}
203+
}
191204
}
192205

193206
func TestLatin1CollatorKey(t *testing.T) {
194207
SetNewCollationEnabledForTest(true)
195208
defer SetNewCollationEnabledForTest(false)
196-
collations := []string{"latin1_bin", "latin1_swedish_ci"}
209+
collators, names := latin1TestCollators()
197210
tests := []keyTable{
198211
{"a", [][]byte{{0x61}, {0x41}}},
199212
{"A", [][]byte{{0x41}, {0x41}}},
@@ -204,12 +217,14 @@ func TestLatin1CollatorKey(t *testing.T) {
204217
{"\xFF\xDD", [][]byte{{0xFF, 0xDD}, {0xFF, 0x59}}},
205218
{"", [][]byte{{}, {}}},
206219
}
207-
testKeyTable(t, collations, tests)
208-
209-
// One weight byte per input byte, so the sort key never outgrows the raw data.
210-
collator := GetCollator("latin1_swedish_ci")
211-
for _, test := range tests {
212-
require.LessOrEqual(t, len(collator.Key(test.Str)), collator.MaxKeyLen(test.Str), test.Str)
220+
for i, collator := range collators {
221+
for _, test := range tests {
222+
comment := fmt.Sprintf("key %q, using %v", test.Str, names[i])
223+
require.Equal(t, test.Expect[i], collator.Key(test.Str), comment)
224+
require.Equal(t, test.Expect[i], collator.ImmutableKey(test.Str), comment)
225+
// One weight byte per input byte, so the sort key never outgrows the raw data.
226+
require.LessOrEqual(t, len(collator.Key(test.Str)), collator.MaxKeyLen(test.Str), comment)
227+
}
213228
}
214229
}
215230

@@ -232,7 +247,7 @@ func TestLatin1SwedishCIPattern(t *testing.T) {
232247
{"a\\%b", '\\', "AxB", false},
233248
}
234249
for _, test := range tests {
235-
pattern := GetCollator("latin1_swedish_ci").Pattern()
250+
pattern := (&latin1SwedishCICollator{}).Pattern()
236251
pattern.Compile(test.pattern, test.escape)
237252
require.Equal(t, test.match, pattern.DoMatch(test.str),
238253
fmt.Sprintf("pattern %q, str %q", test.pattern, test.str))
@@ -283,8 +298,6 @@ func TestGetCollator(t *testing.T) {
283298
require.IsType(t, &unicodeCICollator{}, GetCollatorByID(192))
284299
require.IsType(t, &unicode0900AICICollator{}, GetCollatorByID(255))
285300
require.IsType(t, &zhPinyinTiDBASCSCollator{}, GetCollatorByID(2048))
286-
require.IsType(t, &latin1SwedishCICollator{}, GetCollator("latin1_swedish_ci"))
287-
require.IsType(t, &latin1SwedishCICollator{}, GetCollatorByID(8))
288301
require.IsType(t, &binPaddingCollator{}, GetCollatorByID(9999))
289302

290303
SetNewCollationEnabledForTest(false)

pkg/util/collate/latin1_swedish_ci.go

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,19 @@ import (
2222

2323
// latin1SwedishCICollator is the collator for latin1_swedish_ci.
2424
//
25+
// It is deliberately NOT registered in newCollatorMap/newCollatorIDMap yet, so
26+
// GetCollationByName still rejects the collation and no column can be created with
27+
// it. Enabling it needs three things that are not in place:
28+
// - TiKV and TiFlash collators, so pushed-down expression evaluation agrees with
29+
// the sort key TiDB writes, plus a guard that keeps expressions in TiDB until
30+
// the storage layer understands the collation.
31+
// - A byte-safe case fold for the collate.IsCICollation fast paths in
32+
// pkg/expression. Those apply strings.ToLower, which assumes UTF-8 and maps
33+
// every latin1 byte >= 0x80 to U+FFFD, making distinct characters compare equal.
34+
// - The documentation updates tracked in the feature request.
35+
//
36+
// Until then this type stands alone and is exercised directly by its unit tests.
37+
//
2538
// Unlike the utf8mb4 collators this one is byte-oriented rather than rune-oriented.
2639
// MySQL defines latin1_swedish_ci as a 256-entry weight table indexed by the cp1252
2740
// byte value (strings/ctype-latin1.c, sort_order_latin1), and TiDB's latin1 charset

0 commit comments

Comments
 (0)