laskoviymishka commented on code in PR #1576:
URL: https://github.com/apache/iceberg-go/pull/1576#discussion_r3681153955
##########
schema.go:
##########
@@ -1649,20 +1651,38 @@ func validAvroName(n string) bool {
return false
}
- if !unicode.IsLetter(rune(n[0])) && n[0] != '_' {
- return false
- }
+ for i, r := range n {
+ if i == 0 {
+ if !isAvroNameStart(r) {
+ return false
+ }
+
+ continue
+ }
- for _, r := range n[1:] {
- if !unicode.In(r, unicode.Number, unicode.Letter) && r != '_' {
+ if !isAvroNamePart(r) {
return false
}
}
return true
}
+func isAvroNameStart(r rune) bool {
+ return r <= '\uFFFF' && (r == '_' || unicode.IsLetter(r))
Review Comment:
`'\uFFFF'` (U+FFFF) shows up here, again in `isAvroNamePart`, and a third
time in `sanitize` — and the three have to stay in sync (the predicates reject
above the BMP, `sanitize` handles above it). That contract isn't obvious from
any single line.
I'd pull it into a named `const maxBMPRune rune = 0xFFFF` and use it in all
three spots. wdyt?
##########
schema.go:
##########
@@ -1718,13 +1736,26 @@ func (sanitizeColumnNameVisitor) Field(field
NestedField, fieldResult NestedFiel
if field.Name == "" {
panic(fmt.Errorf("%w: field name cannot be empty",
ErrInvalidSchema))
}
+ if !utf8.ValidString(field.Name) {
+ panic(fmt.Errorf("%w: field %d name is not valid UTF-8",
ErrInvalidSchema, field.ID))
+ }
field.Name = makeCompatibleName(field.Name)
return field
}
func (sanitizeColumnNameVisitor) Struct(_ StructType, fieldResults
[]NestedField) NestedField {
+ seen := make(map[string]int, len(fieldResults))
Review Comment:
This new collision check is the one thing I'd most want to settle before
merge.
Java doesn't reject colliding sanitized names in `makeCompatibleName` — it
lets them through and Avro's schema builder rejects the duplicate field later.
Here we panic-to-error earlier, which is arguably clearer, but it's a stricter
contract on a public function than we had before, and it's undocumented.
I'd lean toward keeping the early error since the message is better — but
either way I'd make the choice explicit in the godoc on `SanitizeColumnNames`,
alongside the new invalid-UTF-8 error. Same goes for the Nl/No and
multibyte-first-char changes: this is a public API and the behavior shifted
three ways with nothing in the doc comment. wdyt?
##########
schema.go:
##########
@@ -1678,15 +1698,13 @@ func sanitizeName(n string) string {
var b strings.Builder
b.Grow(len(n))
Review Comment:
Minor, but `b.Grow(len(n))` is a low hint for the case this PR adds — a
supplementary code point is 4 input bytes but expands to 12 (`_xXXXX_xXXXX`),
so an all-escaped name reallocates a few times. `len(n)*3` covers the common
case, or just drop the hint.
##########
schema_test.go:
##########
@@ -1536,6 +1536,130 @@ func TestSanitizeColumnNamesEmptyFieldName(t
*testing.T) {
assert.ErrorContains(t, err, "field name cannot be empty")
}
+func TestSanitizeColumnNamesMatchesJavaIceberg(t *testing.T) {
+ t.Parallel()
+
+ tests := []struct {
+ name string
+ input string
+ want string
+ }{
+ {name: "ASCII letter", input: "Field_9", want: "Field_9"},
+ {name: "underscore", input: "_field", want: "_field"},
+ {name: "ASCII digit first", input: "1field", want: "_1field"},
+ {name: "latin letter", input: "éclair", want: "éclair"},
+ {name: "CJK letters", input: "你好", want: "你好"},
+ {name: "extended latin letter", input: "Łacinka", want:
"Łacinka"},
+ {name: "Unicode digit first", input: "١field", want: "_١field"},
+ {name: "Unicode digit later", input: "a١field", want:
"a١field"},
+ {name: "supplementary letter", input: "𐐀field", want:
"_xD801_xDC00field"},
+ {name: "supplementary digit first", input: "𝟎field", want:
"_xD835_xDFCEfield"},
+ {name: "supplementary digit later", input: "a𝟎field", want:
"a_xD835_xDFCEfield"},
+ {name: "superscript number", input: "a²", want: "a_xB2"},
+ {name: "combining mark", input: "e\u0301", want: "e_x301"},
+ {name: "emoji first", input: "😀field", want:
"_xD83D_xDE00field"},
+ {name: "emoji later", input: "a😀field", want:
"a_xD83D_xDE00field"},
+ {name: "punctuation first", input: "-field", want: "_x2Dfield"},
+ {name: "punctuation later", input: "a-field", want:
"a_x2Dfield"},
+ }
+
+ for _, test := range tests {
+ t.Run(test.name, func(t *testing.T) {
Review Comment:
These subtests don't call `t.Parallel()`, but the ones in
`TestSanitizeColumnNamesRejectsCollisions` do — each case builds its own
schema, so they're safe to parallelize. Worth matching for consistency.
##########
schema.go:
##########
@@ -1649,20 +1651,38 @@ func validAvroName(n string) bool {
return false
}
- if !unicode.IsLetter(rune(n[0])) && n[0] != '_' {
- return false
- }
+ for i, r := range n {
+ if i == 0 {
+ if !isAvroNameStart(r) {
+ return false
+ }
+
+ continue
+ }
- for _, r := range n[1:] {
- if !unicode.In(r, unicode.Number, unicode.Letter) && r != '_' {
+ if !isAvroNamePart(r) {
return false
}
}
return true
}
+func isAvroNameStart(r rune) bool {
+ return r <= '\uFFFF' && (r == '_' || unicode.IsLetter(r))
+}
+
+func isAvroNamePart(r rune) bool {
+ return r <= '\uFFFF' && (isAvroNameStart(r) || unicode.IsDigit(r))
Review Comment:
agreed the Nd-only check here is the right call for Java parity —
`Character.isLetterOrDigit` returns false for Nl/No, so Java escapes those too.
The thing is the old code used `unicode.In(r, unicode.Number,
unicode.Letter)`, which accepted Nl and No. So `"aⅡ"` (U+2161, category Nl)
used to pass through untouched and now escapes to `"a_x2161"` — correct going
forward, but a silent name change for any table old iceberg-go wrote with an
Nl/No column. And the test table only pins the No case (`"a²"`), not Nl.
I'd add a case like `{input: "aⅡ", want: "a_x2161"}` with a short comment
noting the Nd-only match is intentional Java parity, so nobody "fixes" it back
to `unicode.Number` later. wdyt?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
