LuciferYang commented on code in PR #2906:
URL: https://github.com/apache/iceberg-rust/pull/2906#discussion_r4126234678
##########
crates/iceberg/src/arrow/reader/predicate_visitor.rs:
##########
@@ -258,20 +264,58 @@ impl PredicateConverter<'_> {
}
}
-/// Gets the leaf column from the record batch for the required column index.
Only
-/// supports top-level columns for now.
+/// Walks the Parquet column path (root to leaf) through the projected record
batch to
+/// reach a primitive leaf. A single-element path returns the matching
top-level column;
+/// a longer path descends through `StructArray` children by name.
`Schema::build_accessors`
+/// builds accessors only for primitives and primitives nested in structs —
never for a list
+/// or map, nor for anything inside one — so `Reference::bind` rejects those
predicates and
+/// every path segment before the leaf here is a struct.
fn project_column(
batch: &RecordBatch,
- column_idx: usize,
+ path: &[String],
) -> std::result::Result<ArrayRef, ArrowError> {
- let column = batch.column(column_idx);
-
- match column.data_type() {
- DataType::Struct(_) => Err(ArrowError::SchemaError(
- "Does not support struct column yet.".to_string(),
- )),
- _ => Ok(column.clone()),
- }
+ let (root_name, rest) = path
+ .split_first()
+ .ok_or_else(|| ArrowError::SchemaError("Predicate column path is
empty.".to_string()))?;
+
+ let mut current = batch
+ .column_by_name(root_name)
+ .ok_or_else(|| {
+ ArrowError::SchemaError(format!(
+ "Predicate column root `{root_name}` not found in projected
record batch."
+ ))
+ })?
+ .clone();
+ let mut current_name = root_name;
+
+ for part in rest {
+ let struct_array = current
+ .as_any()
+ .downcast_ref::<StructArray>()
+ .ok_or_else(|| {
+ ArrowError::SchemaError(format!(
+ "Predicate column path expected a struct at
`{current_name}` but found {:?}.",
+ current.data_type()
+ ))
+ })?;
+ current = struct_array
+ .column_by_name(part)
+ .ok_or_else(|| {
+ ArrowError::SchemaError(format!(
+ "Predicate column nested field `{part}` not found in
struct `{current_name}`."
+ ))
+ })?
+ .clone();
Review Comment:
Done in d61f4fbbd. `project_column` now descends with
`StructArray::flatten`, which ANDs the struct's validity into each child, so a
leaf under a null parent struct reads as null. Your two cases come out as
expected now: `person.age < 1000` keeps `[1, 3]` and `person.age != 30` keeps
`[3]`. Because each level merges its own nulls, the null outer `person` over
`address` in the doubly nested case is covered too, and there are tests for
both.
##########
crates/iceberg/src/arrow/reader/predicate_visitor.rs:
##########
@@ -237,7 +234,16 @@ impl PredicateConverter<'_> {
),
))?;
Review Comment:
Done in d61f4fbbd. Switched to `if
!self.column_indices.contains(column_idx)`, so the discarded `position` is gone
and the error is only built when the leaf is actually missing.
##########
crates/iceberg/src/arrow/reader/row_filter.rs:
##########
@@ -1289,4 +1289,321 @@ mod tests {
"positional deletes must be applied correctly even when page
indexes are absent"
);
}
+
+ /// End-to-end regression for issue #2432: a predicate on a primitive leaf
nested in a
+ /// struct (`person.age > 25`) must build a row filter and prune rows,
rather than
+ /// failing because the leaf's Parquet column root is a group. Reads a
real Parquet
+ /// file so the projected `RecordBatch` shape (a `StructArray` holding the
leaf) comes
+ /// from arrow-rs, not a hand-built batch.
+ #[tokio::test]
+ async fn test_predicate_on_nested_struct_leaf_reads_real_parquet() {
Review Comment:
Added. `test_predicate_on_leaf_under_null_parent_struct` writes an optional
parent struct with a null row and asserts on a required leaf (`person.age`,
with `<` and `!=`) and an optional leaf (`person.score`, with `<`);
`test_predicate_on_leaf_under_null_outer_struct_doubly_nested` covers the null
outer struct in the doubly nested case. Both go through a new
`ids_kept_by_predicate` helper that writes one batch and returns the ids a
predicate keeps, and the two existing nested-leaf read tests use it now too.
--
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]
