You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "fallintoplace (via GitHub)" <gi...@apache.org> on 2026/07/28 17:18:08 UTC
[PR] fix(schema): handle multibyte runes in name sanitization [iceberg-go]
fallintoplace opened a new pull request, #1576:
URL: https://github.com/apache/iceberg-go/pull/1576
## What changed
Process schema names rune by rune from the first character in both Avro-name validation and sanitization.
## Why
The previous implementation treated the first UTF-8 byte as a complete character, then ranged from the middle of a multibyte encoding. Names beginning with characters such as `é` could therefore produce malformed UTF-8.
The regression coverage includes Unicode letters, a non-letter emoji at the beginning, and a multibyte character later in the name.
## Testing
- `go test .`
- `go vet .`
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(schema): match Java column name sanitization [iceberg-go]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(schema): match Java column name sanitization [iceberg-go]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
laskoviymishka merged PR #1576:
URL: https://github.com/apache/iceberg-go/pull/1576
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org