linliu-code opened a new pull request, #662:
URL: https://github.com/apache/hudi-rs/pull/662
**Stacked on #639–#661** — their commits appear here until they merge.
**Review only the last commit.**
## The bug
`engine.rs` decides whether a predicate is safe to push, builds the filter,
and then throws it away:
```rust
let _row_filter = if self.reader_context.can_push_row_filter() { .. };
```
Every row of every base file is read and fed to the merge; the predicate is
left for whoever sits above the reader.
This was not a pre-existing gap in this crate — it is a capability lost in
the port. Upstream threads the same filter into the base read. Here base reads
go through this crate's `BaseFileReader`, which had nowhere to put one, so it
went nowhere. #661's harness cases are what surfaced it: ten of them failed
with `actual > expected` because no filtering happened at all.
## The fix
Give it somewhere to go.
- `BaseFileReadOptions` gains an optional `RowFilterBuilder`
- `ParquetBaseFileReader::apply_options` resolves it once the footer is open
and installs it on the stream builder
- The three base reads in `make_base_file_source` pass it through
Other base-file formats ignore it, which is why it is a documented
parquet-only option rather than a trait method. `RowFilterBuilder` was already
a public type in `storage`, and its doc already said "options holding it stay
`Clone`" — it was designed to live in a read-options struct.
**A builder rather than a filter**, because the predicate has to be resolved
against the *file's own* schema, which the caller does not have until the
footer is read. A builder that declines — typically because the file has none
of the columns the predicate names — reads every row rather than none. That
direction matters: the opposite would silently drop data on a file the
predicate cannot speak about.
## The safety gate, which now actually does something
Pushing a predicate below the merge is only sound when the merge cannot
change its outcome:
- **copy-on-write** — always safe, there is no merge
- **merge-on-read** — safe only when every column the predicate names is a
primary key, since primary keys are immutable across upserts (mirrors Java's
`filterIsSafeForPrimaryKey`)
- **otherwise** — do not push; the post-merge filter still runs
That gate was already written. It was untested in practice, because nothing
was ever pushed, so `harness_filter_unsafe_not_pushed_mor` passed vacuously.
I checked it is no longer vacuous by forcing the gate open:
```
- let row_filter = if self.reader_context.can_push_row_filter() {
+ let row_filter = if true {
test harness_filter_unsafe_not_pushed_mor ... FAILED (gate forced open)
test harness_filter_unsafe_not_pushed_mor ... ok (gate restored)
```
## Tests
- **10 harness cases un-ignored** and now passing — boolean, date, decimal,
float32, timestamp, logical-type and data-column predicates on CoW; primary-key
`=`, `<` and `IN` on MOR
- **1 negative case** (`harness_filter_unsafe_not_pushed_mor`) upgraded from
vacuous to meaningful, verified by the mutation above
- **2 new unit tests** on `ParquetBaseFileReader`, at the layer the option
was added, since `with_row_filter` is public API independent of the
merge-on-read engine: an always-false predicate must return 0 rows (a dropped
filter would return all of them), and a builder that declines must return every
row
Full workspace green: 1164 lib + 79 table-read + 39 datafusion + 21 + 12; 19
ignored, down from 29. `cargo fmt --check` clean; no new clippy findings in the
changed files.
🤖 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]
