fix(reader): keep pages whose index bounds cannot be compared with the predicate by rich7420 · Pull Request #3507 · apache/iceberg-rust

rich7420 · 2026-10-22T05:45:25Z

Which issue does this PR close?

Part of Filtered scans silently drop rows for type-promoted columns (int->long, float->double) #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 is left to #3521, a draft stacked on this PR that uses the promotion helpers from #3483, now merged. #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, including a value above i32::MAX and a page mixing 100 and 3_000_000_000: =, > and IN. The test also asserts the mixed page's raw bounds, whose max read as i32 sorts below its min.
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.

…e predicate Page-index bounds are built with the table field type but 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 bounds never compare with the predicate literal. Inequality and IN predicates treated that as "cannot match" and skipped pages holding matching rows. Keep a page whenever its bound cannot be compared with the literal: the inequality check now treats an incomparable bound as a possible match, and IN uses all(lt) / all(gt) instead of !any(ge) / !any(le).

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

JeonDaehong

Checked locally on 56ece04. With the two evaluator changes reverted, all four new tests fail (the pages are skipped), and with the PR all 34 page_index_evaluator tests pass. Inline: one note for the widening follow-up now that #3483 is in, and an unsigned-bounds detail that the current UINT_32 test can't catch.

JeonDaehong · 2026-10-22T12:26:15Z

-                    if cmp_fn(&bound, datum) {
+                    // A bound that can't be compared with the literal (e.g. a
+                    // file column narrower than the field type) might match.
+                    if bound.partial_cmp(datum).is_none() || cmp_fn(&bound, datum) {


#3483 has landed now, so Datum::promote_to is available. Widening INT32 → long and FLOAT → double bounds here would keep pruning these pages instead of keeping all of them. That's fine as the follow-up you describe, but see the UINT_32 note on the test below before reusing int32_bound_to_datum.

Thanks! I'll keep this PR focused on the conservative fix and handle the promote_to widening in a follow-up, zero-extending UINT_32 bounds as you noted.

JeonDaehong · 2026-10-22T12:26:15Z

-                    (Some(min), _) if !literals.iter().any(|datum| datum.ge(&min)) => {
+                    // `all(lt)` rather than `!any(ge)`, so that a bound that
+                    // can't be compared with the literals keeps the page.
+                    (Some(min), _) if literals.iter().all(|datum| datum.lt(&min)) => {


Checked: for comparable bounds all(lt) and !any(ge) agree (Datum floats use total_cmp), and an empty literal set still prunes as before, so only the incomparable case changes.

JeonDaehong · 2026-10-22T12:26:15Z

+    fn eval_keeps_uint32_pages_of_long_field() -> Result<()> {
+        // Parquet `UINT_32` columns map to `long` but have INT32 page bounds.
+        let (metadata, _temp_file) =
+            create_parquet_file_from_values(&[100_u32, 3_000_000_000], |v| {


For the widening follow-up: UINT_32 page bounds are the raw INT32 bits with an unsigned column order, so they can't be widened with i64::from(i32). I wrote a page that mixes 100 and 3_000_000_000 through the same helper. The column index has min = 100, max = -1294967296 (ColumnOrder::TYPE_DEFINED_ORDER(UNSIGNED)), so sign-extending makes the max smaller than the min, and x > 2_000_000_000 would prune a page that holds 3_000_000_000. These pages need zero extension (u32 as i64) when the column order is unsigned.

Since every page here holds a single value, this test can't tell the two apart. Adding a mixed page would pin it before the widening lands.

Thanks for catching this! I added a page with both 100 and 3_000_000_000 and asserted its raw bounds, where the max read as i32 sorts below the min. The widening follow-up will need to zero-extend these.

A page holding both 100 and 3_000_000_000 has a raw signed max below its min, which pins the unsigned column order that a later bound widening has to zero-extend rather than sign-extend.

JeonDaehong

Thanks, 97d9426 pins the mixed UINT_32 page with its raw bounds, so the widening follow-up can't sign-extend it by accident. LGTM.

bugorz · 2026-10-24T00:12:05Z

+        })?;
+
+        assert_keeps_all_pages(&metadata, PrimitiveType::Long, 4096, [
+            Reference::new("col").less_than(Datum::long(250)),


Do you think we could cover 2 more filters: less_than_or_eq and greater_than?

Good idea, thanks! Added both in 13f1aca for the long field over INT32 pages. Each one fails without the fix.

…nd timestamp pages

JeonDaehong

13f1aca and 9e3a09b cover the remaining inequality operators on the promoted pages. Still LGTM.

A file written before an int -> long, float -> double, or decimal precision promotion keeps its old physical type, while the predicate is bound to the table type. The row filter cast the literal down to the file's Arrow type, so an out-of-range literal became null and dropped every row, and a double literal was rounded to f32. Cast a numeric column to the table type before comparing instead, as RecordBatchTransformer does for the output. Other columns keep the literal cast. Page-index pruning is left to apache#3507 and apache#3521. Part of apache#3122.

Predicates are bound to the table schema, but the row filter evaluated them on batches in the file's physical types. For a file written before an int -> long, float -> double, or decimal precision promotion, try_cast_literal cast the literal down to the file type, so an out-of-range literal became null and dropped every row, and a double literal was rounded to f32. Convert the filter columns to the table schema with a RecordBatchTransformer, as the reader does for its output, and make try_cast_literal fail instead of returning null. Page-index pruning is left to apache#3507 and apache#3521. Part of apache#3122.

Copilot AI balanced review requested due to automatic review settings October 22, 2026 05:45

Copilot AI reviewed Oct 22, 2026

rich7420 mentioned this pull request Oct 22, 2026

perf: preserve supported Iceberg residual conjuncts apache/datafusion-comet#6847

Draft

JeonDaehong reviewed Oct 22, 2026

View reviewed changes

test(reader): cover a mixed UINT_32 page in the page-index evaluator

97d9426

A page holding both 100 and 3_000_000_000 has a raw signed max below its min, which pins the unsigned column order that a later bound widening has to zero-extend rather than sign-extend.

JeonDaehong approved these changes Oct 23, 2026

View reviewed changes

bugorz reviewed Oct 24, 2026

View reviewed changes

test(reader): cover <= and > on INT32 pages of a promoted long field

13f1aca

rich7420 mentioned this pull request Oct 24, 2026

fix(reader): widen promoted page-index bounds to the field type #3521

Draft

test(reader): cover all four inequality operators on promoted float a…

9e3a09b

…nd timestamp pages

JeonDaehong approved these changes Oct 25, 2026

View reviewed changes

comphead mentioned this pull request Oct 26, 2026

fix(reader): keep matching rows when filtering a promoted column #3540

Open

Navigation Menu

Repository navigation

fix(reader): keep pages whose index bounds cannot be compared with the predicate - #3507

fix(reader): keep pages whose index bounds cannot be compared with the predicate#3507
rich7420 wants to merge 4 commits into
apache:mainfrom
rich7420:fix/page-index-type-mismatch

rich7420 commented Oct 22, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

JeonDaehong left a comment

Uh oh!

JeonDaehong Oct 22, 2026

Uh oh!

rich7420 Oct 22, 2026

Uh oh!

JeonDaehong Oct 22, 2026

Uh oh!

JeonDaehong Oct 22, 2026

Uh oh!

rich7420 Oct 22, 2026

Uh oh!

JeonDaehong left a comment

Uh oh!

bugorz Oct 24, 2026

Uh oh!

rich7420 Oct 24, 2026

Uh oh!

JeonDaehong left a comment

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

rich7420 commented Oct 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

What changes are included in this PR?

Are these changes tested?

AI Disclosure

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

JeonDaehong left a comment

Choose a reason for hiding this comment

Uh oh!

JeonDaehong Oct 22, 2026

Choose a reason for hiding this comment

Uh oh!

rich7420 Oct 22, 2026

Choose a reason for hiding this comment

Uh oh!

JeonDaehong Oct 22, 2026

Choose a reason for hiding this comment

Uh oh!

JeonDaehong Oct 22, 2026

Choose a reason for hiding this comment

Uh oh!

rich7420 Oct 22, 2026

Choose a reason for hiding this comment

Uh oh!

JeonDaehong left a comment

Choose a reason for hiding this comment

Uh oh!

bugorz Oct 24, 2026

Choose a reason for hiding this comment

Uh oh!

rich7420 Oct 24, 2026

Choose a reason for hiding this comment

Uh oh!

JeonDaehong left a comment

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

rich7420 commented Oct 22, 2026 •

edited

Loading