linliu-code opened a new pull request, #748:
URL: https://github.com/apache/hudi-rs/pull/748
## Description
A pushed predicate is evaluated by parquet against the base file's physical
values, before
`project_batch_to_schema` runs. That is sound only while a physical value
means what its physical
type says, which the apache/hudi#18132 logical-type repair breaks. A legacy
base file labels a
tz-aware column `timestamp-micros` while the stored i64 is milliseconds, and
the repair
reinterprets the buffer (relabels the unit, keeps the value) rather than
rescaling it. A literal
in table (millis) semantics, normalised against that label, therefore reads
every value as a 1970
instant: `ts > 2020-01-01` matches nothing and the scan drops rows that do
match. A post-scan
re-filter only subtracts rows, so nothing downstream recovers them.
Both halves of the hazard are already here: the repair arm in
`schema/batch_evolution.rs`, and the
base-read pushdown surface (`ReaderContext::row_filter_builder` /
`row_group_selector`, gated by
`base_read_pushdown_is_safe`). That gate asks whether the read merges; it
has never asked what a
column's type is.
This adds two gates, cheapest first. `batch_evolution::repair_risk_columns`
answers, from the table
schema alone, which of a predicate's columns could ever be misread. It is
computed once per scan by
whoever supplies the filter and routed in through the new
`HoodieFileGroupReaderBuilder::with_repair_risk_columns`. It is empty for
any table whose predicate
touches no tz-aware millis column, which is every table Spark wrote with
micros, so the common scan
does no per-file work and loses no pushdown. Only when it is non-empty does
`base_file_source`
compare footer types, and then only for those named columns
(`batch_evolution::reinterpreted_columns`,
no extra IO since the footer schema is already fetched just above). On a
conflict it withdraws the
parquet `RowFilter` and the row-group selector together, and counts it on
the new
`ReadVolume::pushdown_suppressed_by_repair` so the refusal is visible rather
than reading as "never
installed".
Declining rather than reconciling the literal is deliberate: rewriting a
literal into a file's
physical unit puts unit arithmetic exactly where a mistake means missing
rows, which is the failure
mode being fixed. It is also not available here, since hudi-core sees the
predicate only as an
opaque `RowFilterBuilder` closure.
Two things reviewers should know. The pushdown surface has no in-crate
production caller
(`resolve_reader_context` leaves `row_filter_builder` as `None`, and none of
`crates/jvm-ffi`,
`crates/datafusion`, `python/` or `cpp/` sets it), as that field's own doc
already states, so this
guard is embedder-facing groundwork and nothing is wired live. And it is
fail-open: an embedder that
installs a row filter without calling `with_repair_risk_columns` gets no
protection, because
hudi-core cannot derive a predicate's columns from a closure. The new
builder method is therefore
documented as required alongside `with_row_filter_builder`, not as a tuning
knob.
Not closed here: the parquet log-block path still gates only on
`mor_pk_safe`, so a record key that
is itself a mislabelled timestamp column stays exposed. That needs a
timestamp record key to matter.
The unprojected fallback path (no `required_schema`) returns before the
footer fetch and is left
ungated, since with no required schema there is no evolution and so no
reinterpretation.
The diff is 1148 added lines, over the 1000 line guideline: 282 of those are
behaviour (83 in
`engine.rs`, 132 in `batch_evolution.rs`, 30 in `reader_context.rs`, 34 in
`storage/mod.rs`, 3 in
`resolver.rs`) and the other 866 are tests. Splitting the classifiers from
their use site would land
a gate nothing consults, so the halves are not independently meaningful.
## How are the changes test-covered
- [ ] N/A
- [x] Automated tests (unit and/or integration tests)
- [ ] Manual tests
- [ ] Details are described below
`base_read_declines_pushdown_when_the_file_needs_a_reinterpreting_repair`
was observed failing
before the guard existed: the row filter was built once instead of never,
and in the
selector-installed twin the scan returned 0 of 2 rows, which is the row loss
itself. Exactly four
tests failed in that state and no others.
`base_read_keeps_pushdown_when_the_file_is_honestly_labelled` fails if the
guard over-declines, so
the pair pins both directions.
`base_read_keeps_pushdown_for_a_predicate_on_an_unaffected_column`
pins the narrowing,
`base_read_keeps_pushdown_when_no_predicate_column_is_at_risk` the unarmed gate,
`base_read_declines_pushdown_for_an_unprojected_predicate_column` gate 2's
table side, and
`repair_suppression_is_counted_without_a_row_group_selector` the
row-filter-side counter. Eleven
unit tests pin the two classifiers.
Each of the eight mechanisms was reverted in turn and every one fails at
least one test: the pair
classifier (10 tests), the table-side classifier (3), container recursion
(1), the engine gate (4,
exactly the four seen failing before the fix), gate 2's table side (1), the
counter (2), the builder
routing (1), and gate 2's candidate scoping (3, including the narrowing
test). The sweep requires
the unmutated tree to report zero failures before any mutant result is
trusted.
`make check` and `make test` are green (1708 tests), as is
`cargo clippy -p hudi-core --lib --no-default-features -- -D warnings`.
--
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]
