fix(reader): keep matching rows when filtering a promoted column by comphead · Pull Request #3540 · apache/iceberg-rust

comphead · 2026-10-26T17:49:56Z

Which issue does this PR close?

Part of bug(arrow): filters lose rows after numeric schema promotion #3122. This fixes the row filter, the reader's default path. The open PRs fix(reader): keep pages whose index bounds cannot be compared with the predicate #3507 and fix(reader): widen promoted page-index bounds to the field type #3521 handle the page-index part (with_row_selection_enabled(true)).

What changes are included in this PR?

Predicates are bound to the table schema, but the Arrow row filter evaluated them on batches in the file's 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. An out-of-range literal became null, so x < 2147483648 returned no rows and x IN (1, 2147483648) dropped the row where x = 1. A rounded double made x = 0.1 match a stored 0.1f32.

get_row_filter converts the filter columns to the table schema with a RecordBatchTransformer, as the reader does for its output. This also covers INT(8) columns read as int and UINT_32 columns read as long.
try_cast_literal fails instead of returning null. After the conversion it only bridges representations of one type, such as Binary and LargeBinary.

A batch that already matches the table schema passes through unchanged. Since the transformer converts every mismatch, string filters on files written with large_string (PyIceberg, for example) now cast the column instead of the literal, about 0.5 ns more per row in a release micro-benchmark.

Are these changes tested?

Yes.

test_predicate_on_promoted_column covers eight predicates on int and float files read as long and double, one per comparison method. On main it fails on the first case.
test_predicate_converter_rejects_lossy_literal_cast covers the strict cast.
Skipping the transformer, misaligning its field IDs, or relaxing the cast each fail a test. Dropping the leaf-order sort does not, since it only keeps the pass-through path.
A local probe also passes for decimal(9,2), INT(8), and UINT_32 files, which lose rows on main.
The iceberg lib suite passes, and clippy and fmt are clean.

AI Disclosure

Drafted with Claude Code (Anthropic) and reviewed by the author.

Copilot

🟢 Approval recommended

The focused fix consistently covers all eight affected comparison paths with targeted regression tests and no identified blocking issues.

No open issues 🎉

What changed in this PR

Fixes row filtering on numerically promoted columns, addressing the default-reader portion of #3122. Page-index pruning remains outside this PR’s scope.

Changes:

Casts numeric columns to the table type before comparisons, preventing literal overflow and rounding from changing filter results.
Adds regression coverage for all eight comparison paths across integer and floating-point promotions.

File	Description
`crates/iceberg/src/arrow/reader/row_filter.rs`	Adds promoted-column read tests with exact expected results.
`crates/iceberg/src/arrow/reader/predicate_visitor.rs`	Applies numeric column promotion while preserving nonnumeric literal casting.

_{Review effort: Balanced}

kevinjqliu · 2026-10-27T04:06:27Z

thanks for digging into this! the fix works, but this feels like a workaround to me 😅

I think the root cause is that predicates are bound to the table schema but evaluated against file schema batches. try_cast_literal narrows the literal to the file type, and a failed cast silently becomes null. this PR casts the column back up in each comparison, but only for numeric types, so the lossy literal cast is still there for everything else.

i think we should cast the filter batch to table types once in get_row_filter, using the same rule as RecordBatchTransformer. then try_cast_literal can error instead of returning null.

another option is ArrowReaderOptions::with_schema so parquet decodes directly into table types. not sure it supports int -> long though

Let me know what you think!

comphead · 2026-10-27T15:45:34Z

Thanks @kevinjqliu for the review, it might totally happen that batches’ schema and incoming schema are different, in datafusion, arrow and Comet we have a notion on schema_adapter which actually converges the schema and I think RecordBatchTransformer plays the same role, checking it.

ArrowReaderOptions::with_schema for int -> long is not currently supported

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.

comphead · 2026-10-27T18:32:45Z

@kevinjqliu please take another look

Copilot

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Copilot review overview

2 open issues

CastOptions { safe: false } is counterintuitive given the intent described in the comment… · New
In assert_promoted_reads, with_length(0) is ambiguous and makes the test harder to understand… · New

_{Review effort: Lite}

+    let options = CastOptions {
+        safe: false,
+        ..Default::default()
+    };
+    let literal_array = cast_with_options(literal_array, column_type, &options)?;


+        let file_size = std::fs::metadata(&path).unwrap().len();
+        let reader = ArrowReaderBuilder::new(FileIO::new_with_fs(), Runtime::current()).build();
+        for (predicate, expected) in cases {
+            let task = FileScanTask::builder()
+                .with_file_size_in_bytes(file_size)
+                .with_start(0)
+                .with_length(0)
+                .with_data_file_path(path.clone())
+                .with_data_file_format(DataFileFormat::Parquet)
+                .with_schema(schema.clone())
+                .with_project_field_ids(vec![1])
+                .with_predicate(Some(predicate.bind(schema.clone(), true).unwrap()))
+                .with_case_sensitive(true)
+                .build()


comphead force-pushed the fix-3122-promoted-predicates branch from b92d957 to 0f76a20 Compare October 26, 2026 19:50

github-actions Bot added the datafusion label Oct 26, 2026

comphead marked this pull request as ready for review October 26, 2026 22:48

Copilot AI balanced review requested due to automatic review settings October 26, 2026 22:48

Copilot started reviewing on behalf of comphead October 26, 2026 22:49

View session

Copilot AI reviewed Oct 26, 2026

View reviewed changes

comphead force-pushed the fix-3122-promoted-predicates branch from 0f76a20 to 0be54f9 Compare October 27, 2026 16:16

comphead requested a balanced review from Copilot October 27, 2026 18:32

Copilot AI reviewed Oct 27, 2026

View reviewed changes

Copilot stopped reviewing on behalf of comphead due to an error October 27, 2026 18:51
The job was not acquired by a runner even after multiple attempts.

Navigation Menu

Repository navigation

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

fix(reader): keep matching rows when filtering a promoted column#3540
comphead wants to merge 1 commit into
apache:mainfrom
comphead:fix-3122-promoted-predicates

comphead commented Oct 26, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

kevinjqliu commented Oct 27, 2026

Uh oh!

comphead commented Oct 27, 2026

Uh oh!

comphead commented Oct 27, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

comphead commented Oct 26, 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

🟢 Approval recommended

Uh oh!

kevinjqliu commented Oct 27, 2026

Uh oh!

comphead commented Oct 27, 2026

Uh oh!

comphead commented Oct 27, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

comphead commented Oct 26, 2026 •

edited

Loading