linliu-code opened a new pull request, #660:
URL: https://github.com/apache/hudi-rs/pull/660
> **Review only — please do not merge this.** It is #639–#659 squashed into
a single diff, opened at a reviewer's request for a whole-picture read. The
reviewable increments are those 21 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`, plus the switch that reaches
it. ~31.8k lines across 59 files, 32 new modules.
**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.
## How to read it
The one thing worth understanding first is the seam, because it explains why
the other 20 PRs are shaped the way they are. `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)
├─ is_metadata_table() → existing reader (always, setting ignored)
├─ no log files / read-opt. → base-file read (nothing to merge)
├─ engine == legacy → existing reader (the default)
└─ engine == v2 → adapter → ported reader
│
all four ──┴──→ apply_eager_options(filters,
projection)
```
`Table`, DataFusion, the Python binding and the cxx bridge are **untouched**
— the ported reader was given a matching operation set precisely so the seam
could sit here. Filters and projection stay out of the adapter so both engines
run through the same `apply_eager_options` and cannot drift on what a filter
means.
Three rules keep it safe:
1. **A metadata table always uses the existing reader**, whatever the
setting says — its base files and log blocks are HFile, which the ported reader
cannot read. Permanent, not transitional.
2. **Refusals, not fallbacks.** No resolvable schema → error. Unparseable
engine name → error. `get_or_default` would have silently swallowed a typo and
read with the *other* engine, leaving a caller convinced they'd tested v2.
3. **Base-file-only slices stay on the existing path** — not a capability
limit, but the ported engine takes its schema from `hoodie.table.create.schema`
where that path takes it from the base file, and the Avro conversion models a
map as `Dictionary(Utf8, V)` (invalid Arrow). Every parquet fixture here has a
map column.
## Corresponding PRs
| PR | New lines | What |
|---|---|---|
| #639 | +83/-12 | reader context, resolver, entry point |
| #640 | +457/-5 | stats, profiling, input-split leaves |
| #641 | +798 | delete context, output converter |
| #642 | +819 | Avro schema comparison, Parquet list normalization |
| #643 | +1767 | schema evolution for merged batches |
| #644 | +465 | row serialization, record positions |
| #645 | +1095 | buffered record |
| #646 | +1038 | record merger, update processor, converter |
| #647 | +3247 | record context, row extraction |
| #648 | +1377 | schema handler |
| #649 | +2399 | spillable merge map — adds `rocksdb` |
| #650 | +1948/-5 | record buffer, merge iterator |
| #651 | +427/-51 | replace reader context with the ported one |
| #652 | +9791/-1 | log record readers, key-based buffer |
| #653 | +1728/-17 | position-based merging, buffer loader |
| #654 | +1866 | the file group reader itself |
| #655 | +173/-31 | streaming reads |
| #656 | +52 | pins an existing-reader defect (independent, off `main`) |
| #657 | +1129/-25 | 19 fixtures vs Spark snapshots |
| #658 | +306/-193 | split metadata-table reading out |
| #659 | +498/-2 | the switch described above |
The order isn't arbitrary: the module import graph was computed, two
dependency cycles removed upstream first, and the remainder topologically
sorted into 9 layers so each PR compiles against only what already landed.
## Decisions that need a maintainer's call
**1. `hoodie.record.merge.mode` is never read.** The crate reads
`hoodie.table.record.merge.strategy`, which Hudi never writes — zero
occurrences across every fixture here. What tables actually write is
`hoodie.record.merge.mode`, read nowhere. So the strategy is always *inferred*
from whether an ordering field is set: correct for event-time tables, wrong for
commit-time-ordered ones, which legitimately have none and so derive
`append_only`.
**2. `append_only` silently drops deletes** (#656). It concatenates data
batches and never consults `delete_batches`. Combined with (1): **a
commit-time-ordered MOR table returns duplicate rows and ignores deletes
today.** #656 pins this with a test rather than changing it, because the fix is
a design choice.
**3. `rocksdb` as a required dependency** (#649) — the spill backend for the
merge map. Bundles RocksDB 8.10, needs `libclang`. Licenses compatible.
**4. `base64` + `roaring`** (#644) — Hudi stores record positions as a
base64 roaring bitmap, so this is the storage format's encoding rather than a
design choice.
## Fixes to the existing read path
Three, all in shared `avro_to_arrow` / schema handling, so they fix the
**current** reader too:
- **Decimals had no Arrow conversion.** A test *asserted* that failure as
expected behavior; it now asserts the read.
- **Avro `timestamp-*` logical types lost their UTC zone** (the naive
variants are `local-timestamp-*`), so a log batch could not be concatenated
with the parquet base batch it was merged against.
- **`hoodie.table.create.schema` is stored properties-escaped** (`\:`) and
one of its two consumers wasn't unescaping it.
## Known gaps — v2 is not at parity
Opt-in until it serves everything the existing reader serves.
**Blocks enablement:** schema supply for the standalone entry
(`new_with_options` from a bare URI — what the cxx bridge uses); `append_only`,
which has no v2 equivalent and is *derived* rather than opt-in, so ordinary
tables reach it.
**Decide, don't necessarily bridge:** metadata table / HFile
(recommendation: keep permanently separate — #658 makes that structural); the
merge-mode semantics above, which also gate a differential harness, since until
both engines derive semantics identically they legitimately disagree and the
harness would report v2 being *correct* as a mismatch.
**Ported but unreachable:** position-based merge (the engine drops the
row-number column the buffer needs) and predicate pushdown (`RowFilterBuilder`
exists, nothing installs one). Neither blocks parity — the existing reader has
neither — but they're advertised capabilities of the ported code.
**Fixture coverage: 6 of 15 match the Spark gold snapshots.** The other 9
fail on the **existing reader too**, verified by reading each through
`Table::read` — shared gaps in Avro and log-format coverage, not port
regressions: Avro maps as `Dictionary(Utf8, V)` ×3, the two delete-record
ordering shapes ×6, no corrupt-block detection ×1, and empty
`hoodie.table.partition.fields` failing validation above the reader. The gap
list lives in an `#[ignore]`d test that **fails if a gap starts passing**, so
fixes get promoted rather than going unnoticed.
## Status
Local: 1115 lib + 79 table-read + 39 datafusion + 21 + 12, all green; `cargo
fmt --check` clean; build warning-free.
**CI has never run on any of these PRs** — `PR` reports `action_required`,
`CI` and `Code` report `startup_failure`, identically on all of them. The same
workflows succeed on fork PRs from committers, `main` hasn't moved, and every
PR is `MERGEABLE`, so this looks like a workflow-approval gate rather than
anything in the diffs. **A committer approving the runs would unblock it.**
Local verification ran on rustc 1.97.1 while the repo pins 1.94, so `make
check` (`-D warnings`) on the pinned toolchain is unverified.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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]
