You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "sandugood (via GitHub)" <gi...@apache.org> on 2026/06/22 08:58:42 UTC
[PR] fix(reader): graceful missing column index [iceberg-rust]
sandugood opened a new pull request, #2693:
URL: https://github.com/apache/iceberg-rust/pull/2693
## Which issue does this PR close?
- Closes #2452 . Iterates over opened PR: #2464
## What changes are included in this PR?
Changed the `get_row_selection_for_filter_predicate` to return `Ok(None)` on cases where indices were absent instead of throwing an error.
## Are these changes tested?
Unit-tested the change in logic. As specified in the https://github.com/apache/iceberg-rust/pull/2464#discussion_r3395462759
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(reader): graceful missing column index [iceberg-rust]
Posted by "sandugood (via GitHub)" <gi...@apache.org>.
sandugood commented on PR #2693:
URL: https://github.com/apache/iceberg-rust/pull/2693#issuecomment-4766600504
It seems that the original PR didn't receive any feedback from the committer. We keep having the same bug (regarding .parquet reading in Datafusion Comet). That is why I iterated over it and introduced the proposed changes.
cc @laskoviymishka, if you have time to review this. Thanks in advance!
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(reader): graceful handling of missing column index [iceberg-rust]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
laskoviymishka commented on PR #2693:
URL: https://github.com/apache/iceberg-rust/pull/2693#issuecomment-4766813051
I'll take a look on this week
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(reader): graceful handling of missing column index [iceberg-rust]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
laskoviymishka merged PR #2693:
URL: https://github.com/apache/iceberg-rust/pull/2693
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(reader): graceful handling of missing column index [iceberg-rust]
Posted by "sandugood (via GitHub)" <gi...@apache.org>.
sandugood commented on code in PR #2693:
URL: https://github.com/apache/iceberg-rust/pull/2693#discussion_r3465590062
##########
crates/iceberg/src/arrow/reader/pipeline.rs:
##########
@@ -337,13 +337,13 @@ impl FileScanTaskReader {
}
if self.row_selection_enabled {
- row_selection = Some(ArrowReader::get_row_selection_for_filter_predicate(
+ row_selection = ArrowReader::get_row_selection_for_filter_predicate(
Review Comment:
Added a full-suite test that address:
- writing with disabled stats (so, now col/offset index available on read) and reading it with a predicate
- writing a position delete file along the previously written `.parquet` and reading it with a predicate
Both cases address the change introduced by the PR
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(reader): graceful handling of missing column index [iceberg-rust]
Posted by "sandugood (via GitHub)" <gi...@apache.org>.
sandugood commented on PR #2693:
URL: https://github.com/apache/iceberg-rust/pull/2693#issuecomment-4787269699
Thank you for your review, @laskoviymishka
Addressed the comments
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(reader): graceful handling of missing column index [iceberg-rust]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
laskoviymishka commented on code in PR #2693:
URL: https://github.com/apache/iceberg-rust/pull/2693#discussion_r3463189689
##########
crates/iceberg/Cargo.toml:
##########
@@ -75,6 +75,7 @@ serde_repr = { workspace = true }
serde_with = { workspace = true }
strum = { workspace = true, features = ["derive"] }
tokio = { workspace = true, optional = false }
+tracing = { workspace = true }
Review Comment:
`tracing = { workspace = true }` already landed on main in 47527fe4 ("log warning when available_parallelism falls back to default"), so this line is a duplicate that'll conflict on rebase. Rebasing on main should make it a no-op you can just drop.
##########
crates/iceberg/src/arrow/reader/row_filter.rs:
##########
@@ -617,4 +627,217 @@ mod tests {
assert_eq!(first_val, 100, "Task 2 should start with id=100, not id=0");
}
}
+
+ fn int_schema() -> SchemaRef {
+ Arc::new(
+ Schema::builder()
+ .with_schema_id(1)
+ .with_fields(vec![Arc::new(NestedField::required(
+ 1,
+ "x",
+ Type::Primitive(PrimitiveType::Int),
+ ))])
+ .build()
+ .unwrap(),
+ )
+ }
+
+ fn simple_predicate(schema: SchemaRef) -> BoundPredicate {
+ Reference::new("x")
+ .greater_than(crate::spec::Datum::int(0))
+ .bind(schema.clone(), false)
+ .unwrap()
+ }
+
+ fn field_id_map() -> HashMap<i32, usize> {
+ let mut m = HashMap::new();
+ m.insert(1_i32, 0_usize);
+ m
+ }
+
+ fn metadata_no_page_indexes() -> Arc<ParquetMetaData> {
+ let msg_type = parse_message_type("message schema { REQUIRED INT32 x; }").unwrap();
+ let schema_desc = Arc::new(SchemaDescriptor::new(Arc::new(msg_type)));
+ let file_meta = FileMetaData::new(2, 0, None, None, schema_desc.clone(), None);
+ Arc::new(ParquetMetaDataBuilder::new(file_meta).build())
+ }
+
+ fn metadata_column_index_only() -> Arc<ParquetMetaData> {
+ let msg_type = parse_message_type("message schema { REQUIRED INT32 x; }").unwrap();
+ let schema_desc = Arc::new(SchemaDescriptor::new(Arc::new(msg_type)));
+ let file_meta = FileMetaData::new(2, 0, None, None, schema_desc, None);
+ Arc::new(
+ ParquetMetaDataBuilder::new(file_meta)
+ .set_column_index(Some(vec![]))
+ .build(),
+ )
+ }
+
+ fn metadata_with_both_indexes() -> Arc<ParquetMetaData> {
+ let msg_type = parse_message_type("message schema { REQUIRED INT32 x; }").unwrap();
+ let schema_desc = Arc::new(SchemaDescriptor::new(Arc::new(msg_type)));
+ let file_meta = FileMetaData::new(2, 0, None, None, schema_desc, None);
+ Arc::new(
+ ParquetMetaDataBuilder::new(file_meta)
+ .set_column_index(Some(vec![]))
+ .set_offset_index(Some(vec![]))
+ .build(),
+ )
+ }
+
+ /// Testing suite regarding: https://github.com/apache/iceberg-rust/issues/2452
+ /// Testing when: both indices are absent, some present, both present
+ #[test]
+ fn test_absent_column_index_returns_ok_none() {
+ let schema = int_schema();
+ let predicate = simple_predicate(schema.clone());
+ let metadata = metadata_no_page_indexes();
+ let field_id_map = field_id_map();
+
+ let result = ArrowReader::get_row_selection_for_filter_predicate(
+ &predicate,
+ &metadata,
+ &None,
+ &field_id_map,
+ schema.clone().as_ref(),
Review Comment:
`schema.clone().as_ref()` is a redundant Arc clone right before `as_ref` — `schema.as_ref()` does the same thing. This shows up in all five direct-call tests (702, 728, 755, 778, 801).
The Makefile runs `cargo clippy --all-targets --all-features --workspace -- -D warnings`, so `clippy::redundant_clone` will fail CI here. Same root cause as `simple_predicate` taking `schema` by value then calling `.bind(schema.clone(), false)` at line 648, and `metadata_no_page_indexes` cloning `schema_desc` at 661 where the other two helpers correctly move it. A local clippy pass with `-D warnings` should surface all of them.
##########
crates/iceberg/src/arrow/reader/pipeline.rs:
##########
@@ -337,13 +337,13 @@ impl FileScanTaskReader {
}
if self.row_selection_enabled {
- row_selection = Some(ArrowReader::get_row_selection_for_filter_predicate(
+ row_selection = ArrowReader::get_row_selection_for_filter_predicate(
Review Comment:
None of the six new tests drive this wiring — they all call `get_row_selection_for_filter_predicate` directly. If this assignment regressed (back to wrapping in `Some`, or dropping the predicate), every test would still pass, yet #2452 surfaced precisely as a scan failure through this path.
I'd add one `#[tokio::test]` that writes a Parquet file without page indexes and runs the predicate scan end-to-end, asserting the correct rows come back. Ideally cover the (index absent) + (positional deletes present) case in the same test — pre-PR that combination errored, and now `row_selection` stays `None` and the delete branch takes the `None => Some(delete_row_selection)` arm. That's the correct behavior, but it's unguarded today and positional deletes are a data-correctness mechanism. wdyt?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org