rich7420 opened a new pull request, #3521: URL: https://github.com/apache/iceberg-rust/pull/3521
> [!NOTE] > **Draft, stacked on [#3507](https://github.com/apache/iceberg-rust/pull/3507).** This branch includes the #3507 commits and a merge from `main`. Only the last two commits are new here: `fix(reader): widen promoted page-index bounds to the field type` and `fix(reader): ignore NaN float page-index bounds`. > > After #3507 merges, I will rebase this branch onto `main` so that only these two commits remain, rerun the tests below, and mark the PR ready for review. Feedback on the approach is welcome in the meantime; please save detailed review until the PR is ready. ## Which issue does this PR close? - Part of [#3482](https://github.com/apache/iceberg-rust/issues/3482). ## What changes are included in this PR? This implements the widening follow-up agreed in [#3507](https://github.com/apache/iceberg-rust/pull/3507#discussion_r4349686413). That PR prevents pruning based on incomparable page-index bounds, such as `INT32` bounds read as a `long` field after an `int` -> `long` promotion. This PR widens supported bounds to the field type so inequality and `IN` predicates can prune these pages. - Signed `INT32` bounds for a `long` field and `FLOAT` bounds for a `double` field are widened with `Datum::promote_to`, added in [#3483](https://github.com/apache/iceberg-rust/pull/3483). - `UINT_32` bounds are stored as `INT32` bit patterns ordered as unsigned values. The column descriptor’s sort order identifies unsigned bounds, which are zero-extended to `long`. If this produces `min > max`, as can happen with bounds written in signed order, both bounds are discarded. - If either `FLOAT` or `DOUBLE` bound is NaN, both bounds are discarded. Legacy writers may store NaN bounds for pages that also contain other values, as documented in the [Parquet specification](https://github.com/apache/parquet-format/blob/master/src/main/thrift/parquet.thrift). `Datum` uses `total_cmp` for floating-point comparisons, so trusting these bounds can prune matching pages. Widening exposes promoted `double` fields to this risk too. [#3502](https://github.com/apache/iceberg-rust/pull/3502) added a related NaN guard for row-group inequality pruning. Discarding bounds prevents pruning based on those bounds; null-count checks and other predicates still apply. Bounds that remain incomparable, such as `INT32` bounds for a `timestamp` field, retain the conservative fallback from #3507. ## Are these changes tested? Yes. Evaluator tests in `page_index_evaluator.rs` use metadata from real Parquet files and assert exact row selections. The NaN cases substitute synthetic column indexes. - `long` field over `INT32` pages: `<`, `>=`, `<` with a literal above `i32::MAX`, `>` with `i32::MAX`, and two-value `IN`. - `double` field over `FLOAT` pages: `>`, `<=`, and two-value `IN`. - `long` field over `UINT_32` pages, including a value above `i32::MAX` and a page mixing `100` and `3_000_000_000`: `=`, `>`, `<`, and `IN`. - `FLOAT` pages read as native `float` or promoted `double`, and `DOUBLE` pages read as native `double`: pages with a NaN minimum or maximum are kept for `=`, `<`, `>`, and two-value `IN`. - `timestamp` field over `INT32` pages: the existing fallback test keeps every page for `<`, `>=`, `=`, and `IN`. Helper tests also verify that signed-order `UINT_32` bounds are discarded, correctly ordered bounds are widened, and both floating-point bounds are discarded if either is NaN while finite bounds are preserved. Mutation checks confirmed that sign-extending `UINT_32` bounds, removing the `min > max` check, and removing the NaN check each cause at least one test to fail. `cargo test -p iceberg --lib expr::visitors` and `cargo test -p iceberg --lib arrow::` passed, as did `cargo fmt --all -- --check` and `cargo clippy -p iceberg --all-targets -- -D warnings`. ## AI Disclosure Drafted with Claude Code and reviewed with Codex. Review focus: whether zero-extension and the signed-order check are correct for `UINT_32` columns, and whether discarding both bounds when either is NaN is conservative across predicates. -- 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]
