linliu-code opened a new pull request, #695:
URL: https://github.com/apache/hudi-rs/pull/695
## Description
**Stacked on #694, which is stacked on #693, #692 and #691. Review those
first.** Until they merge the diff here shows their commits too; the delta is
`8cd7a9a..HEAD`, five files.
There are two metadata-table read paths, and the one production uses never
touches the version two file group reader: it opens the base file itself, scans
the log files itself, and folds records with its own merger. So every fix to
the merge or to the HFile path reaches ordinary reads and not metadata reads,
and the metadata table stays on machinery that exists only for it.
This adds the metadata layer that would end that, above the version two
reader and owning every metadata concern: the partition, the record shape, the
decoded type, the `.` to empty string normalisation a non-partitioned table
needs. The reader below is handed a slice, a schema and a predicate, which is
the layering Hudi already has between `HoodieBackedTableMetadata` and its
generic file group reader. A named-key read pushes the predicate into the base
file read, which seeks to the blocks that can hold those keys, and into the
HFile log block read, where a log block's own index says which of its blocks to
parse. Dropping log records for keys nobody asked for is sound because the
merge is per key.
**It is not on the read path, and that is the point of this PR being what it
is.** `MetadataTable::read_files_partition` still calls the existing reader.
The new layer matches it record for record and value for value, and is **3.4x
slower end to end** — 4.24 ms against 1.25 ms on a base file plus eight log
files, measured with both readers in one process. Three causes, none of them
the layer itself: the ranged base-file open costs a head request and two range
requests before any data byte, which is pure overhead on a 7.6 KB metadata
file; version two's log scan walks every log file twice, once for headers and
once for content, where the older scanner decoded each block as it walked, and
that is 2.6 ms of the 3.0 ms difference; and the metadata payload merger
rebuilds an Arrow map array per merged pair where the older merger mutated a
map in place.
Two of those predate this change, but switching the default metadata read
onto that path is what would make them matter, so the switch waits. The layer
lands with its parity tests so the comparison is standing and checkable while
the cost is worked on, rather than being rewritten later from a description.
Scope is the `files` partition, matching what the existing reader serves.
The other five metadata partitions, the key to file group routing they need,
and a valid instant filter are separate work.
## How are the changes test-covered
- [ ] N/A
- [x] Automated tests (unit and/or integration tests)
- [ ] Manual tests
- [ ] Details are described below
Parity against the existing reader, record for record and value for value:
file names, sizes and tombstone flags, not key sets. Both readers run in one
test over one slice, on the `files` partition before its compaction so the
comparison covers a real base file plus seven log files rather than a base
alone. A key set comparison would not have been enough; that is how the fold
bug fixed in #693 stayed hidden behind an earlier key set assertion. A
named-key read is compared the same way.
A named-key read is also asserted to put fewer records through the merge
than a full read, measured with the merge map's peak entry count. That number
is honest about one mechanism and blind to the rest, which is how the 3.4x
above went unnoticed until it was measured end to end; it is kept because it is
the only thing that pins the log-side pushdown, and with the predicate withheld
from the log decoder both reads peak at four and the test fails on that number.
Every rule has a mutation that fails a named test: the named-key filter, the
record context rebuild, the key normalisation, the all-partitions rewrite, the
tombstone flag, the size, the null-valued map entry, and the predicate withheld
from the log decoder. Three of those were **not** caught on the first pass, all
for one reason: the fixture's metadata table is partitioned and holds no
tombstones. They are covered by unit tests on constructed batches now, and one
of them exposed a real divergence rather than a gap, since the file map was
normalising every key as though it were a partition when in a files record
those keys are file names.
Three findings from review are fixed here. The row filter built a set once
instead of scanning the key list per row, which the two other filter sites in
this crate already did and this one did not. A null map value is omitted rather
than defaulted, since defaulting invents a zero sized undeleted file and sends
a read at a path that does not exist. And the per-block record read reuses the
existing helper rather than carrying a second copy of how a record is built
from a key value.
The non-partitioned branch, which asks for `.`, is not covered end to end
and cannot be: no fixture here is a non-partitioned table with a metadata
table. The normalisation it depends on is unit tested instead, and both places
say so.
Run locally on both legs: default features and `--no-default-features`,
clippy clean on both with warnings denied, fmt clean, the ninety case table
read suite green, and `cargo check --workspace --all-targets --all-features`
clean including the Python and C++ bindings. No CI run has happened: fork pull
requests sit at `action_required` until a committer approves them, so all of
the above is local only.
--
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]
