rich7420 opened a new pull request, #3507:
URL: https://github.com/apache/iceberg-rust/pull/3507
## Which issue does this PR close?
- Part of #3482.
## What changes are included in this PR?
`PageIndexEvaluator` builds page bounds from the table field's type and the
file's physical literal. When the two differ, e.g. a `long` field read against
`INT32` pages written before an `int` -> `long` promotion, a `double` field
against `FLOAT` pages, or a `UINT_32` column mapped to `long`, the bound (e.g.
`Datum { type: Long, literal: Int }`) is incomparable with the predicate
literal. Inequality and `IN` predicates treated that as "cannot match" and
skipped the page, so matching rows were silently dropped when row selection was
enabled.
Minimal reproduction: write a Parquet file with an `INT32` column holding
100, 200, 300 and 400 in separate pages, and read it as a `long` field with `x
< 250` or `x IN (200, 400)`. The evaluator skips all four pages.
This PR treats incomparable bounds conservatively in inequality and `IN`
predicates:
- the inequality path treats an incomparable bound as a possible match;
- `IN` uses `all(lt)` / `all(gt)` instead of `!any(ge)` / `!any(le)`, so an
incomparable bound no longer prunes the page.
`eq` already keeps such pages, `not_eq` and `not_in` select all rows, and
`starts_with` / `not_starts_with` return an error on non-string bounds rather
than skipping the page. `RowGroupMetricsEvaluator` already keeps row groups in
this case. This is the same class of fix as #3314, which addressed the same
problem for decimal fields.
This fixes the incorrect page pruning for incomparable bounds, but keeps the
affected pages rather than pruning them. Widening the page bounds to the field
type can follow once #3483 lands, which adds the promotion helpers and lists
this page-index case as a follow-up. #3122 also reports this page-index
failure. The closed, unmerged PR #3123 proposed addressing it through bound
promotion alongside an Arrow row-filter fix. This PR provides a conservative
fallback for incomparable bounds and does not change the Arrow row filter.
## Are these changes tested?
Yes, with new unit tests in `page_index_evaluator.rs` that write real
Parquet page indexes and assert every page is kept:
- `long` field over `INT32` pages: `<`, `>=`, `<` with a literal above
`i32::MAX`, and `IN`
- `double` field over `FLOAT` pages: `>`, `<=` and `IN`
- `long` field over `UINT_32` pages holding a value above `i32::MAX`: `=`,
`>` and `IN`
- `timestamp` field over `INT32` pages: `<`, `>=`, `=` and `IN`
The `=` cases already pass on `main` and are kept as regression guards. All
four tests fail without the fix. Each also fails when either the inequality
change or the `IN` change alone is reverted. `cargo test -p iceberg --lib
expr::visitors` and `cargo test -p iceberg --lib arrow::` pass, and `cargo fmt
--all -- --check` and `cargo clippy -p iceberg --all-targets -- -D warnings`
are clean.
## AI Disclosure
Drafted with Claude Code. Review focus: whether the incomparable-bound
fallback covers every predicate that can prune a page. Bound promotion and
Arrow row filtering are outside this change.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
