voonhous commented on PR #19454:
URL: https://github.com/apache/hudi/pull/19454#issuecomment-5292876864

   Pushed some commits here directly rather than round-tripping suggestions, 
since the scope grew -- say the word if you'd rather have had them as review 
comments.
   
   The header fix turned out to be one instance of a wider gap. #9634 converted 
every encode site to `getUTF8Bytes`, but no decode sweep followed, since 
`fromUTF8Bytes` only landed in #10574 four months later. The one that matters 
most is `OrcUtils.readSchema`: #9634 fixed the ORC schema *writer* and not the 
reader, so a MOR table with ORC base files and non-ASCII field names still hit 
the same Avro failure even after your fix. `RocksDBDAO` fails differently -- 
the decode is a loop condition, so a non-ASCII partition path silently returns 
no file groups rather than mojibake. Added a checkstyle rule so this cannot 
regress, which is what forced the remaining sites into the same PR.
   
   On my earlier suggestion to swap `new Schema.Parser()` for 
`HoodieSchema.parse` -- I deleted that assertion instead. The `assertEquals` 
above it already pins the string exactly, so nothing downstream of it can fail; 
swapping the parser would only have kept a dead assertion. Same reasoning for 
the test overall: a round-trip cannot fail on a UTF-8 JVM, where the old and 
new code are the same call, so it now asserts the bytes on the wire.
   
   Worth a committer's call on two things: at this size it probably wants a 
real HUDI JIRA rather than [MINOR], and the checkstyle rule is the part I would 
look at hardest since it affects every module's build.
   
   Note my first push broke CI with a checkstyle suppression that was invalid 
against the 1.0 suppressions DTD; fixed in 949876b, and the build is green 
again.
   


-- 
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]

Reply via email to