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: [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]
