test(trino): add MoR read tests for delete markers, custom payloads and commit-time ordering - #19295
Merged
voonhous merged 5 commits intoJul 30, 2026
Conversation
voonhous
changed the title
voonhous
marked this pull request as draft
July 15, 2026 06:14
voonhous
force-pushed
the
18898--mor-merge-semantics-read-tests
branch
2 times, most recently
from
July 15, 2026 08:39
ba38225 to
b597403
Compare
3 tasks
voonhous
force-pushed
the
18898--mor-merge-semantics-read-tests
branch
from
July 15, 2026 09:16
b597403 to
4c8ae68
Compare
3 tasks
voonhous
force-pushed
the
18898--mor-merge-semantics-read-tests
branch
from
July 15, 2026 16:49
4c8ae68 to
b57f97e
Compare
…nd commit-time ordering Fixes apache#18898 (follow-up from the RFC-105 review of HudiTrinoReaderContext.getRecordMerger): the merge-mode dispatch had no end-to-end coverage where the merger choice matters. Adds runtime-written v10 MOR tables and snapshot-read tests for: - Deletes under EVENT_TIME_ORDERING (mor_deletes): hard deletes -- which at v10 are native delete log files read back through the connector's own getFileRecordIterator with the synthetic delete-log schema, a previously untested path enabled by the required-column resolution this branch stacks on -- plus _hoodie_is_deleted soft deletes, an OBSOLETE soft delete and an OBSOLETE update (lower ordering value) that must LOSE against the base row. - COMMIT_TIME_ORDERING (mor_commit_time): a lower-ordering update that must WIN (latest write), the exact mirror of the event-time case, discriminating OverwriteWithLatestMerger from event-time merging. - Payload-driven semantics without any hudi.record-merger-impls property: AWSDmsAvroPayload (v10-translated to COMMIT_TIME + delete-key props; Op='D' log records delete rows at merge time), OverwriteNonDefaultsWithLatestAvroPayload (v10-translated to PARTIAL_UPDATE_MODE=IGNORE_DEFAULTS; null update columns keep stored values), and a user-defined SummingTestPayload riding the payload-based CUSTOM strategy whose merged value (old + new) proves the payload's combineAndGetUpdateValue executed at read time. Test rows are written with the pass-through HoodieAvroPayload (not a BaseAvroPayload) so delete-flagged rows land as data records and every merge decision happens at read time from the table config. Bugs found by these tests and fixed here: - HudiUtil.mergeRequiredColumnNames read the custom delete key/marker from raw table props, but v9+ table creation persists them PREFIXED (hoodie.record.merge.property.*). The file-group reader strips the prefix via getTableMergeProperties before DeleteContext reads the plain keys, so its required schema included the delete column while the connector's base-read prediction missed it and the base-read projection guard failed every narrow projection on such tables. Now resolved the same way the reader does: table merge properties first, raw props as fallback. Unit-tested in TestHudiMergeRequiredColumns. - buildColumnHandles matched predicted merge columns against metastore data columns case-sensitively; the prediction carries the table schema's casing (AWSDms hardcodes 'Op') while metastore names are lowercased, so the handle was never built. Matching is now case-insensitive. - buildRequiredColumnHandles emitted projection handles with the lowercased metastore name on the log projection, but hudi-common reads merge fields off log records by the table schema's exact field name (Avro lookups are case-sensitive), so e.g. the DMS delete marker was never seen on log records. Handles are now re-labeled with the required-schema field's exact name; parquet columns resolve case-insensitively either way. - count(*) over a MoR split with log files failed with "Failed to construct Avro schema": the query projects no columns, and HudiPageSource's serializer fed the empty projection to AvroHiveFileUtils.determineSchemaOrThrowException, which rejects an empty column list. HudiUtil.constructSchema now short-circuits an empty projection to an empty record schema; the page source already counts positions fine with zero channels. HudiUtilTest's empty-columns test now pins the empty-record schema instead of the former exception. - Payload-based merging read garbage on the summing table ([, -44] instead of [k1, 109]): records handed to hudi-common carried a schema reconstructed from Hive metastore types (every column a nullable union, fields in projection order), while the file-group reader tracks its required schema for those records. BaseAvroPayload round-trips the newer record through Avro binary with the tracked schema, so the structural mismatch misaligned the binary decode. HudiTrinoReaderContext now builds the log-side AND base-side serializers with requiredSchema itself, via a new HudiAvroSerializer constructor that takes the record schema explicitly and maps page channels to record fields BY NAME (the base projection's order can differ from the required schema's field order; containment holds in both directions through the existing base-read guard and generateRequiredSchema always containing the requested schema). This also aligns record layouts with the positions hudi-common derives from the tracked schema (partial-update merging, _hoodie_operation lookup). - (hudi-common, engine-agnostic) The file-group reader passed its raw input props to initRecordMerger, the MERGE_TYPE lookup and the schema handler instead of the ConfigUtils.getMergeProps view, which layers the table's prefixed merge properties (hoodie.record.merge.property.*) in as plain keys -- a regression from commit 19961e7 replacing the previous in-place putAll with a copy. DeleteContext therefore missed the translated DMS delete key/marker, the delete column was not a mandatory merge field, and Op='D' log records merged as regular data updates instead of deletes on snapshot reads. Fixed in HoodieFileGroupReader and HoodieLsmFileGroupReader alike by passing this.props downstream. Also re-ports a shim-lineage guard (trino repo commit 78209f83b50) that the RFC-105 migration squash predates: HudiAvroSerializer's default constructor now maps channels past hidden (synthesized, split-prefilled) columns instead of relying on serialize() never being called with them; with identity positions a hidden column would shift every later value into the wrong field and overrun the record. Also replaces the TODO(apache#18898) in getRecordMerger with a pointer to the new suites.
voonhous
force-pushed
the
18898--mor-merge-semantics-read-tests
branch
from
July 29, 2026 08:45
b57f97e to
1074728
Compare
voonhous
marked this pull request as ready for review
July 29, 2026 08:46
Member
Author
|
@wombatu-kun This is ready for review! Thank you! |
wombatu-kun
reviewed
Jul 29, 2026
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #19295 +/- ##
============================================
+ Coverage 74.82% 75.54% +0.71%
- Complexity 32342 32661 +319
============================================
Files 2574 2574
Lines 142978 142995 +17
Branches 17527 17566 +39
============================================
+ Hits 106990 108023 +1033
+ Misses 27922 26932 -990
+ Partials 8066 8040 -26
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
- pass the payload class to getTableMergeProperties (the no-arg overload does not exist; the module profile being off by default hid the break) - drop the buildColumnHandles case-folding and the log-projection handle re-label: appendMissingMergeRequiredColumns and the requiredSchema-stamping serializer already cover both - stop building a record schema in the page-building serializer and revert the constructSchema empty-list special case; the count(*) path stays pinned by TestHudiMorMergeModeSemantics - scope the getRecordMerger comment to the CUSTOM arm, the only return value the file-group reader dereferences for merging - split the two monolithic fixtures into per-table subclasses of AbstractMergerHudiTablesInitializer composed with CompositeHudiTablesInitializer
Member
Author
|
@wombatu-kun First round of reviews addressed! |
wombatu-kun
reviewed
Jul 30, 2026
- qualify every fixture's TABLE_NAME/RT_TABLE_NAME in both suites so each assertion names the table it checks (no mixed static imports) - replace the duplicated PARTITION_PATH constants in the delete-writing fixtures with a hoodieKey(String) helper on AbstractMergerHudiTablesInitializer - correct the getRecordMerger comment: the ordering arms ARE reachable via partialMerge on IS_PARTIAL log blocks, just not covered by these suites; restore the TODO, now pointing at follow-up issue apache#19413
voonhous
force-pushed
the
18898--mor-merge-semantics-read-tests
branch
from
July 30, 2026 05:16
e4ed279 to
a2f7145
Compare
Member
Author
|
@wombatu-kun Second round of reviews done, thank you! |
wombatu-kun
reviewed
Jul 30, 2026
…ests - rename testPrefixedDeleteKeyAndMarkerAreRequested to testPrefixedDeleteKeyIsRequested: only the delete key is requested, the marker is a precondition - reuse AWSDmsAvroPayload.OP_FIELD / DELETE_OPERATION_VALUE in the DMS fixture instead of local Op / D literals - add a non-marker Op='U' log record for k1 in the DMS fixture so a broken marker comparison fails the suite (k1 must update, not delete) - flip the five new table names to trail with _mor (deletes_mor, commit_time_mor, dms_mor, overwrite_non_defaults_mor, summing_mor) to match the existing fixtures in the package - cover the CUSTOM merge arm's delete path: the summing fixture gains a k2 base row and a hard-delete commit, pinned by testSummingPayloadHardDeleteRemovesRowOnSnapshotRead
wombatu-kun
reviewed
Jul 30, 2026
The hard-delete comments claimed the delete reaches the payload arm as an empty payload whose combineAndGetUpdateValue returns empty. In fact HoodieAvroRecordMerger.merge returns the delete on its isCommitTimeOrderingDelete short-circuit (writeClient.delete records carry the sentinel ordering value) before loadPayload runs, so no payload is constructed. Say so in the initializer javadoc, the commit comment and the test method comment.
wombatu-kun
approved these changes
Jul 30, 2026
| public class EventTimeDeletesHudiTablesInitializer | ||
| extends AbstractMergerHudiTablesInitializer | ||
| { | ||
| public static final String TABLE_NAME = "deletes_mor"; |
Contributor
There was a problem hiding this comment.
The Summary and Changelog still names the tables mor_deletes and mor_commit_time and says the TODO(apache/hudi#18898) is dropped, while head has deletes_mor/commit_time_mor and restores the TODO as #19413. Can the description be refreshed before merge (not a blocker), along with the ordering-arm justification the getRecordMerger comment has since corrected?
Member
Author
There was a problem hiding this comment.
Refreshed the description: table names now match head (deletes_mor / commit_time_mor), and the TODO paragraph now says the comment was rewritten rather than dropped -- the ordering arms are reached through partialMerge on IS_PARTIAL log blocks, tracked as #19413.
voonhous
enabled auto-merge (squash)
July 30, 2026 14:17
Collaborator
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #19288 (which stacks on #18837) -- only the head commit is new here.
Describe the issue this Pull Request addresses
Closes #18898
HudiTrinoReaderContext.getRecordMergerdispatches on the table's merge mode, but every existing MoR test uses a default payload where all mergers give the same answer. Deletes, custom payloads, and commit-time ordering -- the cases where the dispatch actually matters -- had no coverage.Summary and Changelog
Adds runtime-written MOR tables (current table version, native logs) and snapshot-read tests:
deletes_mor(event-time): hard deletes, which at v10 are native delete log files read back through the connector'sgetFileRecordIterator(previously untested, and only readable on top of feat(trino): resolve merge-required columns from the table schema #19288);_hoodie_is_deletedsoft deletes; and an obsolete delete + update with a lower ordering value that must lose to the base row.commit_time_mor: the same lower-ordering update must win here (latest write wins). The pair tells the two mergers apart.hudi.record-merger-implsset: AWSDmsOp='D'marker deletes, OverwriteNonDefaults partial updates, and a test payload that sums the old and new values -- the summed result can only come fromcombineAndGetUpdateValuerunning at read time.The fixtures extend the existing
AbstractMergerHudiTablesInitializer, one initializer per table, composed withCompositeHudiTablesInitializer.Writing these flushed out three bugs, fixed here:
hoodie.record.merge.property.*). Narrow projections on e.g. AWSDms tables tripped the base-read projection guard.Opkey).HudiAvroSerializergains a constructor that stamps the required schema onto every record it serializes, mapping page channels to record fields by name.constructSchemarejects an empty column list, failing the query. The schema (needed only byserialize()) is no longer built there.Also rewrites the
TODO(apache/hudi#18898)comment ingetRecordMerger: the CUSTOM arm's return value drives merging (combineAndGetUpdateValueat read time, covered end-to-end by the payload tests), while the ordering arms' mergers are only reached throughpartialMergewhen a log block carries IS_PARTIAL -- a path these suites do not cover, now tracked asTODO(apache/hudi#19413). The merge-mode tests pin the mode semantics themselves.Impact
Read-path fixes only: narrow projections on tables with prefixed delete-marker props no longer trip the projection guard, payload-based merges no longer risk schema-mismatch decodes, mixed-case merge columns resolve on log reads, and count(*)-style empty projections work on the merge path. No config changes;
HudiAvroSerializer(an internal util) gains a schema-carrying public constructor.Risk Level
low -- mostly tests; each fix is small and pinned by a new test.
Documentation Update
none
Contributor's checklist