voonhous commented on code in PR #19295:
URL: https://github.com/apache/hudi/pull/19295#discussion_r3680149392
##########
hudi-trino/src/main/java/io/trino/plugin/hudi/reader/HudiTrinoReaderContext.java:
##########
@@ -243,9 +267,8 @@ protected Option<HoodieRecordMerger>
getRecordMerger(RecordMergeMode mergeMode,
// Dispatch on the table's merge mode, mirroring
HoodieAvroReaderContext. The Trino reader
// operates on IndexedRecord, so the Avro mergers apply directly.
Using the read-time merger
// (combineAndGetUpdateValue) rather than a fixed preCombine merger
keeps COMMIT_TIME_ORDERING
- // and custom-payload tables correct on MoR reads.
- // TODO(apache/hudi#18898): add MoR read tests for delete markers and
custom payloads to
- // exercise the EVENT_TIME_ORDERING (combineAndGetUpdateValue) and
CUSTOM branches below.
+ // and custom-payload tables correct on MoR reads;
TestHudiMorMergeModeSemantics and
+ // TestHudiMorPayloadSemantics cover these branches end-to-end
(apache/hudi#18898).
Review Comment:
You are right, `partialMerge` does make those arms reachable. Reworded the
comment to say they are just not covered by these suites, and restored the
TODO, now pointing at #19413 -- the Avro mergers do not implement
`partialMerge` yet (only `DefaultSparkRecordMerger` does), so the read support
itself is the follow-up there.
--
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]
