Skip to content

[REVIEW ONLY, DO NOT MERGE] feat(core): port the merge-on-read file group reader (#639-#673 squashed) - #660

Open
linliu-code wants to merge 1 commit into
apache:mainfrom
linliu-code:review/mor-reader-squashed
Open

[REVIEW ONLY, DO NOT MERGE] feat(core): port the merge-on-read file group reader (#639-#673 squashed)#660
linliu-code wants to merge 1 commit into
apache:mainfrom
linliu-code:review/mor-reader-squashed

Conversation

@linliu-code

@linliu-code linliu-code commented Aug 3, 2026

Copy link
Copy Markdown

Review only — please do not merge this. It is #639#673 squashed into a single diff, opened at a reviewer's request for a whole-picture read. The reviewable increments are those PRs; merging should happen there. I'll close this once the review is done.

What this is

The merge-on-read file group reader ported from onehouseinc/hudi-rs-internal into hudi-core, the switch that reaches it, its end-to-end test harness, and the correctness work that followed once the ported reader was run against real fixtures.

75 files, +38,143 / −1,373.

Nothing changes for existing users. The ported reader is pub(crate) and reachable only via hoodie.read.merge.engine = v2, which defaults to legacy — the reader that has always served reads. Table, DataFusion, the Python binding and the cxx bridge are untouched.

New since this PR was last opened

The first version of this PR was the port itself (#639#659). Everything below landed after, and most of it is not refactoring — it is bugs the fixtures found once the reader could actually run.

Correctness against real files

Change What was wrong
Read the delete record's wrapped ordering value The union in a delete block is Hudi's wrapper schema, not a bare value; decoding it as bare produced garbage or errors
Convert an Avro map to an Arrow map Maps arrived as Dictionary(Utf8, V), which is not a valid Arrow map
Take the merge-on-read schema from the base file The reader used hoodie.table.create.schema, which records the table as created, not as it is
Read base files and parquet log blocks written with legacy 2-level lists write-old-list-structure=true files were rejected outright by parquet-rs
Read a column promoted to string Hudi permits promotions the Avro spec does not; these failed to decode
Resolve a log block up to the promoted schema Spec-legal promotions were not applied to log blocks
Bound an incremental read to its window on both engines v2 filtered per file, the legacy reader per row — v2 over-returned
Infer a pre-v9 table's merge mode the way Hudi does v2 read every v6 MOR table with event-time ordering even when its payload class says commit-time

Robustness and cost

  • Detect corrupt log blocks and resume at the next one, rather than failing the read.
  • Read a file in bounded windows, and sweep log block headers before reading content — measured at 5.3× less memory on the benchmark fixture (+200 MB → +38 MB RSS).
  • Decode Avro data and delete blocks with arrow-avro instead of the hand-rolled converter, deleting 1,073 lines of avro_to_arrow/arrow_array_reader.rs.
  • Push a safe predicate into parquet log blocks.

Observability

  • Say which merge mode a read resolved and where it came from:
    [INFO] merge mode COMMIT_TIME_ORDERING: inferred from the payload class of a
           version 6 table (payload class "…OverwriteWithLatestAvroPayload")
    
  • Name the behaviors this engine does not reproduce (reader_v2::gaps), warning on the ones that would otherwise differ silently.

The switch

The one thing worth understanding first, because it explains the shape of everything else. FileGroupReader keeps its exact public API and becomes a dispatcher, with one decision point inside read_file_slice_from_paths:

read_file_slice_from_paths(base, logs, options)
  ├─ metadata table?           → existing reader  (engine setting ignored)
  ├─ engine == v2              → ported reader    (logs handed over as none when
  │                                                read-optimized, or the slice
  │                                                has none — v2 handles both)
  ├─ no logs / read-optimized  → base-file read
  └─ engine == legacy          → existing reader  (the default)
                                       │
                          all four ────┴──→ apply_eager_options(filters, projection)

The engine is read from the raw config rather than through get_or_default, which falls back to the default when a value fails to parse. A typo in the engine name would otherwise silently read with the other engine — the one outcome this switch must not produce.

Metadata tables (HFile) have no v2 path and always take the existing reader. That is a decision, not a gap.

What this engine refuses

A table that needs something the port does not do gets an error naming it, rather than plausible wrong rows: CUSTOM merge mode or a table's own merger, schema-on-read (InternalSchema, renames), skip_merge, and sorted output. reader_v2::gaps carries the list; only position-based merge is silent, and it now warns.

Testing

1,211 unit tests plus the end-to-end harness, integration tests and gold fixtures; 3 ignored (a memory benchmark that must run one mode per process, an unread-fixture inventory, and one harness expectation still being re-derived).

Please read the test state with this caveat: CI has never run on any of these PRs. Every CI and Code run on this repository since 2026-06-05 has ended in startup_failure — silently, for every contributor — because three action refs are not on the ASF allowlist. #674 fixes that and adds the guard. Until it merges, everything here is verified locally only, and on a Rust toolchain newer than the pinned one.

🤖 Generated with Claude Code

@linliu-code
linliu-code requested a review from xushiyan as a code owner August 3, 2026 18:05
@linliu-code
linliu-code force-pushed the review/mor-reader-squashed branch 3 times, most recently from 83554db to a4773cc Compare August 7, 2026 17:26
@linliu-code linliu-code changed the title [REVIEW ONLY, DO NOT MERGE] feat(core): port the merge-on-read file group reader (#639-#659 squashed) [REVIEW ONLY, DO NOT MERGE] feat(core): port the merge-on-read file group reader (#639-#673 squashed) Aug 7, 2026
Squash of apache#639-apache#673 for review. The reviewable increments are those PRs;
merging should happen there.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@linliu-code
linliu-code force-pushed the review/mor-reader-squashed branch from a4773cc to 44039fd Compare August 7, 2026 21:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant