voonhous commented on code in PR #19454:
URL: https://github.com/apache/hudi/pull/19454#discussion_r3694738075
##########
hudi-common/src/main/java/org/apache/hudi/common/table/log/block/HoodieLogBlock.java:
##########
@@ -468,7 +469,12 @@ private static <T> Map<T, String>
getLogMetadata(SeekableDataInputStream dis, Fu
int metadataEntrySize = dis.readInt();
byte[] metadataEntry = new byte[metadataEntrySize];
dis.readFully(metadataEntry, 0, metadataEntrySize);
- metadata.put(typeMapper.apply(metadataEntryIndex), new
String(metadataEntry));
+ // 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".
Review 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:
```suggestion
// Must match getLogMetadataBytes(), which writes these values as
UTF-8.
```
##########
hudi-common/src/test/java/org/apache/hudi/common/table/log/block/TestHoodieLogBlock.java:
##########
@@ -104,6 +105,29 @@ public void testHeaderMetadata() throws IOException {
Assertions.assertEquals("true",
b.get(HoodieLogBlock.HeaderMetadataType.IS_PARTIAL));
}
+ @Test
+ public void testHeaderMetadataWithNonAsciiSchema() throws IOException {
Review 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.
##########
hudi-common/src/test/java/org/apache/hudi/common/table/log/block/TestHoodieLogBlock.java:
##########
@@ -104,6 +105,29 @@ public void testHeaderMetadata() throws IOException {
Assertions.assertEquals("true",
b.get(HoodieLogBlock.HeaderMetadataType.IS_PARTIAL));
}
+ @Test
+ public void testHeaderMetadataWithNonAsciiSchema() throws IOException {
+ // Header metadata is written as UTF-8 (getHeaderMetadataBytes ->
StringUtils.getUTF8Bytes), so
+ // the read side must decode as UTF-8 to round-trip. Decoding with the
platform default charset
+ // corrupts non-ASCII values on any JVM whose default charset is not
UTF-8, and the corrupted
+ // schema then fails Avro parsing with "Illegal initial character".
+ String schema =
+ "{\"type\":\"record\",\"name\":\"r\",\"fields\":["
+ +
"{\"name\":\"名字\",\"type\":[\"null\",\"string\"],\"default\":null}]}";
+ Map<HoodieLogBlock.HeaderMetadataType, String> a = new HashMap<>();
+ a.put(HoodieLogBlock.HeaderMetadataType.SCHEMA, schema);
+ byte[] bytes = HoodieLogBlock.getHeaderMetadataBytes(a);
+
+ Map<HoodieLogBlock.HeaderMetadataType, String> b =
+ HoodieLogBlock.getHeaderMetadata(new
ByteArraySeekableDataInputStream(new ByteBufferBackedInputStream(bytes)));
+ Assertions.assertEquals(schema,
b.get(HoodieLogBlock.HeaderMetadataType.SCHEMA),
+ "non-ASCII header value must round-trip via UTF-8 regardless of the
JVM default charset");
+ // 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());
Review Comment:
Prefer `HoodieSchema.parse(...)` over `new Schema.Parser()` here:
- Every production consumer of the `SCHEMA` header 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-common` tests (206 uses vs 65 for
`Schema.Parser`), and keeps the test on the Avro -> `HoodieSchema` migration
path.
- `HoodieSchema` is already imported; `org.apache.avro.Schema` then is not
needed.
```suggestion
// 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());
```
##########
hudi-common/src/test/java/org/apache/hudi/common/table/log/block/TestHoodieLogBlock.java:
##########
@@ -27,6 +27,7 @@
import org.apache.hudi.io.SeekableDataInputStream;
import org.apache.hudi.storage.HoodieStorage;
+import org.apache.avro.Schema;
Review Comment:
Tied to the comment below -- if you switch to `HoodieSchema.parse`, this
import can go.
--
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]
