fix(schema): match Java column name sanitization - #1576
Conversation
5954431 to
d7a1232
Compare
7852cfc to
f235bc1
Compare
f235bc1 to
ed16b66
Compare
Signed-off-by: Minh Vu <vuhoangminh97@gmail.com>
ed16b66 to
a777884
Compare
laskoviymishka
left a comment
There was a problem hiding this comment.
Really nice catch on the multibyte bug — casting n[0] to a rune split UTF-8 names at the first byte, so "éclair" got read as à plus an orphan continuation byte and mangled. The rune-by-rune rewrite with surrogate-pair encoding lines up with Java's AvroSchemaUtil, and the test table is genuinely thorough.
(funnily enough this brushes right up against my collation proposal over in apache/iceberg#16972 — both of us poking at Unicode semantics :D)
I'd hold this before merging, though. SanitizeColumnNames is a public function, and this PR quietly changes its contract in three ways: Nl/No characters now get escaped (correct for Java parity, but a silent change from what old iceberg-go wrote), invalid UTF-8 now errors, and colliding sanitized names now error. The collision case is the one I'd most want to talk through — Java doesn't reject those in makeCompatibleName; it lets them through and Avro catches the duplicate later. Erroring early is arguably better, but it's a deliberate choice we should make and document, not a side effect.
Things I'd like to settle in this PR before merge:
- decide whether erroring on collisions is the behavior we want (vs matching Java), and document it on
SanitizeColumnNamesalong with the new invalid-UTF-8 error - note the Nl/No escaping change in the godoc, and add a test pinning an Nl case (e.g.
"aⅡ"→"a_x2161") - extract the
'\uFFFF'BMP bound into a named constant used by both predicates andsanitize
The b.Grow hint and the missing t.Parallel() on the subtests are minor — flagged inline.
Once those are settled, happy to take another pass and approve.
| } | ||
|
|
||
| func isAvroNameStart(r rune) bool { | ||
| return r <= '\uFFFF' && (r == '_' || unicode.IsLetter(r)) |
There was a problem hiding this 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?
| } | ||
|
|
||
| func isAvroNamePart(r rune) bool { | ||
| return r <= '\uFFFF' && (isAvroNameStart(r) || unicode.IsDigit(r)) |
There was a problem hiding this 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?
| @@ -1678,15 +1698,13 @@ func sanitizeName(n string) string { | |||
| var b strings.Builder | |||
| b.Grow(len(n)) | |||
There was a problem hiding this 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.
| } | ||
|
|
||
| func (sanitizeColumnNameVisitor) Struct(_ StructType, fieldResults []NestedField) NestedField { | ||
| seen := make(map[string]int, len(fieldResults)) |
There was a problem hiding this 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?
| } | ||
|
|
||
| for _, test := range tests { | ||
| t.Run(test.name, func(t *testing.T) { |
There was a problem hiding this 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.
Signed-off-by: Hoang Minh Vu <vuhoangminh97@gmail.com>
What changed
Align column-name sanitization with Java Iceberg's Unicode behavior:
Initial digits retain the existing readable
_<digit>form.Why
The previous implementation handled the first byte of a multibyte character separately, then resumed iteration in the middle of its UTF-8 encoding. This could produce malformed output and behavior that differed from Java Iceberg.
Sanitization can also map distinct source names to the same result. Rejecting those collisions prevents schemas with ambiguous sibling fields.
Testing
Coverage includes valid ASCII names, BMP and supplementary Unicode letters and digits, emoji, punctuation, malformed UTF-8, top-level and nested collisions, and collision scoping across separate records.
go test .go vet .