linliu-code opened a new pull request, #699:
URL: https://github.com/apache/hudi-rs/pull/699
## Description
**Stacked on #696, which is stacked on #695, #694, #693, #692 and #691.
Review those first.** Until they merge the diff here shows their commits too;
the delta is `9340f5b..HEAD`, six files.
File group reader version 2 resolved a slice's base file format from
`hoodie.table.base.file.format` alone, ignoring the file's path. A base file
whose format only its extension names, a Lance table that never sets the
config, would have been opened with the parquet reader and died on its footer.
No read ever hit that, and why not is the load-bearing part: the capability
gate in `version_two_unsupported_reason` already resolves per path, and refused
every non-parquet format before version 2 was reached. The gate refusing first
is what kept the resolver's config-only resolution unreachable. So the cost was
never a wrong value; it was that version 1 stays alive for Lance tables, which
is the last capability gap between the two readers. Serving Lance through
version 2 rather than dropping it is a product decision recorded on the ticket,
not one taken here.
That is also why the two halves below cannot be split: narrowing the gate
alone would make the misread reachable for the first time.
`resolve_reader_context` now takes the slice's base file path and makes the
same `resolve_from_configs(configs, Some(path))` call the gate makes, so the
gate that admits a slice and the reader that opens it cannot disagree about its
format. An empty path names no format, so a log-only slice passes `None`. The
gate narrows from "anything not parquet" to HFile, naming the format it
refuses. Lance passes into version 2. HFile still falls back: version 1 refuses
it by name, which is a better answer than reaching version 2's
`HFileBaseFileReader`, which is set up for the metadata table's records rather
than a regular table's. Whether version 2 should serve a regular table's HFile
base files is a separate question from this gate.
The gate is consulted regardless of `base_file_only`, so narrowing it moves
every Lance read shape onto version 2, not only the config-absent merge-on-read
one this is named for: copy-on-write eager, snapshot eager, snapshot streaming,
read-optimized, incremental, log-only slices, and reads that ask for
position-based merge (where version 2 declines positions and merges by key,
since `engine.rs` and `buffer/loader.rs` both gate positions on the base file
being parquet). Worth stating plainly because Lance ships no Spark gold
snapshot: all six Lance fixtures sit under `EXPECTED_WITHOUT_GOLD` in
`gold_parity_tests.rs`, so this reroute is validated by hand-written assertions
rather than by parity with Spark.
`metadata/table/v2_reader.rs` also passes the slice's path now. That is a
no-op rather than a behaviour change: every metadata table sets `HFILE` in
config, and config resolves first, so the same `HFileBaseFileReader` branch is
reached as before.
Two alternatives were rejected. Resolving inside
`engine.rs::base_file_reader()` from `input_split.base_file_path` needs no
signature change, but it puts a second format-resolution point behind the gate
and leaves `ReaderContext.base_file_format` holding a value the engine ignores;
two resolution points is the defect being fixed. Removing the gate outright
would admit a regular table's HFile base files, which is separate work, and
would make the misread reachable again if the resolution ever regressed.
## How are the changes test-covered
- [ ] N/A
- [x] Automated tests (unit and/or integration tests)
- [ ] Manual tests
- [ ] Details are described below
The test that pinned the old gate is updated rather than removed.
`test_version_two_unsupported_reason_non_parquet_base_file_returns_reason`
becomes `test_version_two_unsupported_reason_refuses_hfile_and_admits_lance`,
asserting that HFile is refused by name and that Lance and parquet are both
admitted.
A second gate test,
`test_version_two_admits_the_lance_fixtures_own_base_file`, uses the Lance
fixture's own loaded configs and a real `.lance` base file path, in both the
merge and base-file-only shapes. The first test uses a synthetic
`"part/f.lance"` and the minimal props fixture, so a gate reason that depended
on a table's configuration would slip past it: such a read would quietly fall
back to version 1, and a test comparing only rows would still pass. The path is
discovered by walking the fixture rather than hardcoded, since the base file
names carry a UUID and a commit instant.
On the resolver,
`resolves_base_file_format_from_the_path_when_the_table_names_none` covers
`.lance`, `.hfile`, `.parquet` and an unrecognised extension, and first asserts
that the fixture really names no format, so it cannot pass without the path
being consulted. `an_explicit_base_file_format_outranks_the_path` configures
`hfile` rather than `parquet`, because parquet is also the default and also the
fallback when path resolution finds nothing, so a parquet expectation would
hold even if the config were ignored entirely.
End to end,
`test_v9_lance_nonhivestyle_mor_read_resolves_format_from_path_per_reader_version`
strips `hoodie.table.base.file.format` from a Lance merge-on-read fixture, so
the extension is the only thing naming the format, and reads it under each
reader version on both the eager and the streaming path, cross-checking all
four shapes against version 1's eager read. A fixture that sets the config
cannot discriminate: it passes whether or not the path reaches the resolution.
The partition column is asserted on every row rather than merely projected,
because the table is non-hive-style, the value is not recoverable from the
directory name, and a null renders here as an empty string, so a reader that
null-filled it would pass every other assertion.
`test_v9_lance_nonhivestyle_mor_read_optimized_agrees_across_reader_versions`
covers the read-optimized shape, which reaches version 2 through the
base-file-only branch of the gate.
Each half was reverted in turn to confirm a named test fails. Reverting the
gate to "anything not parquet" fails both gate tests, with the second naming
the real fixture file it can no longer reach, while the five sibling
`version_two_unsupported_reason` tests stay green. Reverting the resolution to
config-only fails the resolver test and three read tests, all three with
`Invalid Parquet file. Corrupt footer` on a `.lance` base file. One of those
three,
`test_v9_lance_nonpartitioned_cow_read_uses_extension_fallback_without_format_config`,
predates this change and becomes discriminating for the resolver half now that
the gate admits Lance into version 2 at all.
The added work is at most three suffix comparisons per file-slice read, and
only when the table names no format, since `resolve_from_configs` returns the
configured value first. Measured in release over five million calls, three
rounds: 13.38ns to 15.03ns per call with the format unset, and 96.17ns to
95.93ns with it set, which is noise around zero. Nothing moved into a per-row
or per-batch path.
Run locally: 1389 lib tests pass with default features and 1369 with
`--no-default-features`, clippy is clean on both feature sets with warnings
denied, fmt is clean, and `cargo test --no-fail-fast --all-targets
--all-features --workspace` passes on every target. `cargo test -p hudi-cpp
--doc` fails with E0464 on `main` too, which is pre-existing and outside this
gate since CI uses `--all-targets`.
No CI run has happened on this PR or on any of #691 through #697: fork pull
requests sit at `action_required` until a committer approves the workflow, 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]
