[MINOR] Decode log block header/footer metadata as UTF-8 - #19454
[MINOR] Decode log block header/footer metadata as UTF-8#19454linliu-code wants to merge 1 commit into
Conversation
HoodieLogBlock.getLogMetadata() read each metadata entry with new String(byte[]), which applies the platform default charset. The write side (getLogMetadataBytes) serializes those values with StringUtils.getUTF8Bytes(), so the two sides disagree on any JVM whose default charset is not UTF-8. The visible consequence is a corrupted header value on read. When the value is the writer schema and it contains non-ASCII field names, the corrupted string then fails Avro parsing with "Illegal initial character". Decode with StringUtils.fromUTF8Bytes() so the read matches the write. That method already lives in the same StringUtils class this file imports getUTF8Bytes from, so no new dependency is introduced. Adds a round-trip test asserting a schema with a non-ASCII field name survives getHeaderMetadataBytes -> getHeaderMetadata and still parses as Avro. Note on the test's scope: the old and new code agree whenever the JVM default charset is already UTF-8, so this test passes before and after the change on a UTF-8 JVM. It documents the invariant rather than reproducing the failure; reproducing that requires running with a non-UTF-8 default charset, e.g. -Dfile.encoding=ISO-8859-1, under which new String(utf8Bytes) demonstrably mojibakes.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #19454 +/- ##
============================================
+ Coverage 76.58% 76.72% +0.13%
- Complexity 33595 33722 +127
============================================
Files 2576 2576
Lines 143560 143560
Branches 17607 17607
============================================
+ Hits 109949 110142 +193
+ Misses 25403 25167 -236
- Partials 8208 8251 +43
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
hudi-agent
left a comment
hudi-agent
left a comment
There was a problem hiding this comment.
Thanks for the contribution! This PR fixes a charset mismatch by decoding log block header/footer metadata as UTF-8 on the read side to match the write side (getLogMetadataBytes()), which uses StringUtils.getUTF8Bytes(). No issues flagged from this automated pass — a Hudi committer or PMC member can take it from here for a final review.
cc @yihua
voonhous
left a comment
voonhous
left a comment
There was a problem hiding this comment.
Reviewed the change and its history. Correct fix; the inline comments are all nits, nothing blocking.
Confirmed
- This completes #9634 / HUDI-6825 (Sep 2023), which moved the write side from
entry.getValue().getBytes()togetUTF8Bytes(...)but left the read at the platform default charset. Before that commit both sides used the default charset and were at least self-consistent. Worth citing #9634 in the PR description. - The method's own javadoc already documents the contract ("Write the actual bytes of the value string in UTF-8 encoding"), so the read now matches it.
getLogMetadatais the only header/footer decode site in the repo -- no parallel decoder in the native log reader or the HFile/Parquet block paths, and the footer path shares this same method.- Framing was never affected: the length prefix is written from the UTF-8
bytes.length, so only the decode was wrong. - No new failure mode:
new String(bytes, UTF_8)substitutes U+FFFD on malformed input rather than throwing, so a legacy/garbage header cannot newly raise a decoding exception.
One correction to the PR description
if a table's log blocks were written by a JVM whose default charset was not UTF-8, those header bytes are not valid UTF-8 on disk
That case cannot happen for any writer since 1.0.0 -- getUTF8Bytes is str.getBytes(StandardCharsets.UTF_8), unconditional, regardless of JVM charset. The real (much narrower) case is a table written by Hudi <= 0.14.x on a non-UTF-8 JVM. Even there, the old read was only correct when the reader's default charset happened to match the writer's, which was never guaranteed -- so this is no worse in expectation. No shim needed, but please fix the wording so reviewers are not evaluating a scenario that cannot occur.
Scope of the bug
Per JEP 400 the default charset is UTF-8 on JDK 18+, so this only bites JDK 8/11/17 with a non-UTF-8 locale (Windows-1252, or unset LANG / LC_ALL=C in containers). Corollary: the new test cannot fail on JDK 18+, and in CI its value depends on the container locale. Fine for a MINOR -- just do not count on it as a hard regression guard.
Nit that is not a problem
The raw 名字 literal in the test source is fine as-is: the root pom sets project.build.sourceEncoding=UTF-8 and there is precedent (TestHoodieSchema, TestHoodieAvroUtils). No need for \uXXXX escapes -- checkstyle's AvoidEscapedUnicodeCharacters would push back on those anyway.
Follow-up, out of scope here
HoodieTableMetaClient.java:358 has the same bug class: HoodieIndexMetadata.fromJson(new String(bytesOpt.get())) decodes index metadata read from storage with the platform default charset. Worth a separate MINOR.
| // Decode as UTF-8 to match the write side: getLogMetadataBytes() serializes these values | ||
| // with StringUtils.getUTF8Bytes(). Using new String(byte[]) here applies the platform | ||
| // default charset instead, so on any JVM whose default charset is not UTF-8 a non-ASCII | ||
| // header value (e.g. a writer schema containing non-ASCII field names) is corrupted on | ||
| // read, and downstream Avro parsing fails with "Illegal initial character". |
There was a problem hiding this comment.
Nit: 5 lines of comment for a 1-line change, and the javadoc ~40 lines above already documents the contract ("Write the actual bytes of the value string in UTF-8 encoding"). Suggest trimming to the essential pointer:
| // Decode as UTF-8 to match the write side: getLogMetadataBytes() serializes these values | |
| // with StringUtils.getUTF8Bytes(). Using new String(byte[]) here applies the platform | |
| // default charset instead, so on any JVM whose default charset is not UTF-8 a non-ASCII | |
| // header value (e.g. a writer schema containing non-ASCII field names) is corrupted on | |
| // read, and downstream Avro parsing fails with "Illegal initial character". | |
| // Must match getLogMetadataBytes(), which writes these values as UTF-8. |
| import org.apache.hudi.io.SeekableDataInputStream; | ||
| import org.apache.hudi.storage.HoodieStorage; | ||
|
|
||
| import org.apache.avro.Schema; |
There was a problem hiding this comment.
Tied to the comment below -- if you switch to HoodieSchema.parse, this import can go, we are trying to migrate away from avro.Schema.
| // The user-visible failure was Avro schema parsing, so assert the round-tripped schema parses | ||
| // and the non-ASCII field name survives. | ||
| Schema parsed = new Schema.Parser().parse(b.get(HoodieLogBlock.HeaderMetadataType.SCHEMA)); | ||
| Assertions.assertEquals("名字", parsed.getFields().get(0).name()); |
There was a problem hiding this comment.
Prefer HoodieSchema.parse(...) over new Schema.Parser() here:
- Every production consumer of the
SCHEMAheader parses that way (HoodieDataBlock:137,HoodieAvroDataBlock:111,HoodieParquetDataBlock:96), so this stays faithful to the actual downstream failure. - It is the dominant convention in
hudi-commontests (206 uses vs 65 forSchema.Parser), and keeps the test on the Avro ->HoodieSchemamigration path. HoodieSchemais already imported;org.apache.avro.Schemathen is not needed.
| // The user-visible failure was Avro schema parsing, so assert the round-tripped schema parses | |
| // and the non-ASCII field name survives. | |
| Schema parsed = new Schema.Parser().parse(b.get(HoodieLogBlock.HeaderMetadataType.SCHEMA)); | |
| Assertions.assertEquals("名字", parsed.getFields().get(0).name()); | |
| // The user-visible failure was schema parsing, so assert the round-tripped schema parses | |
| // and the non-ASCII field name survives. | |
| HoodieSchema parsed = HoodieSchema.parse(b.get(HoodieLogBlock.HeaderMetadataType.SCHEMA)); | |
| Assertions.assertEquals("名字", parsed.getFields().get(0).name()); |
| } | ||
|
|
||
| @Test | ||
| public void testHeaderMetadataWithNonAsciiSchema() throws IOException { |
There was a problem hiding this comment.
Consider folding this into testHeaderMetadata above rather than adding a method -- one extra a.put(..., SCHEMA, <non-ascii schema>) plus one assertEquals gets the same coverage. Recent PMC feedback has been to extend existing tests instead of growing the method count.
The string-equality assert is the real check; the parse assert mostly exercises the schema parser. Your call though -- a separately named test does document the charset intent better.
5 participants
Describe the issue this Pull Request addresses
HoodieLogBlock.getLogMetadata()read each metadata entry withnew String(byte[]), which applies the platform default charset. The write side,getLogMetadataBytes(), serializes those same values withStringUtils.getUTF8Bytes(). The two sides therefore disagree on any JVM whose default charset is not UTF-8.The visible consequence is a corrupted header/footer value on read. When that value is the writer schema and it contains non-ASCII field names, the corrupted string then fails Avro parsing with
Illegal initial character.StringUtils.fromUTF8Bytesalready lives in the same class this file importsgetUTF8Bytesfrom, so no new dependency is introduced.Also adds a round-trip test asserting that a schema containing a non-ASCII field name survives
getHeaderMetadataBytes→getHeaderMetadataand still parses as Avro.Summary and Changelog
Impact
Read path only, for log block header/footer metadata.
Correct on any JVM whose default charset is already UTF-8 — behaviour there is unchanged. On a JVM with a non-UTF-8 default charset, non-ASCII header values now round-trip instead of being corrupted.
One case worth calling out explicitly: if a table's log blocks were written by a JVM whose default charset was not UTF-8, those header bytes are not valid UTF-8 on disk, and this change makes them decode differently than before. That data was already being written inconsistently with
getLogMetadataBytes()'s contract, so I believe aligning the read with the documented write encoding is correct — but I'd appreciate a reviewer's view on whether any compatibility shim is wanted.Risk Level
Low risk.
Two-line change on the read side, plus a test.
Verified against the old code. The test fails before the change and passes after, when run with a non-UTF-8 default charset:
The two characters decode as six, i.e. the six UTF-8 bytes are read as six single-byte characters. On a JVM whose default charset is already UTF-8 the test passes either way, so reproducing requires
LC_ALL=C(or-Dfile.encoding=ISO-8859-1).Verified:
hudi-commoncompiles, and the new test passes (Tests run: 1, Failures: 0, Errors: 0).Documentation Update
None needed.
Contributor's checklist