mbutrovich commented on code in PR #2906:
URL: https://github.com/apache/iceberg-rust/pull/2906#discussion_r4125670086
##########
crates/iceberg/src/arrow/reader/predicate_visitor.rs:
##########
@@ -237,7 +234,16 @@ impl PredicateConverter<'_> {
),
))?;
Review Comment:
Now that the index isn't used, what do you think about checking membership
with `contains`? A discarded `position` reads as if the index matters, and
`ok_or` builds the formatted error even when the column is found.
```suggestion
// Confirm the leaf is among the projected columns.
if !self.column_indices.contains(column_idx) {
return Err(Error::new(
ErrorKind::DataInvalid,
format!(
"Leaf column `{}` in predicates cannot be found in
the required column indices.",
reference.field().name
),
));
}
```
##########
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:
What happens when the parent struct is null? `column_by_name` returns the
child array as stored, without the struct's validity. I wrote a file with `id`
and `person: optional struct<age: required int, score: optional int>`, three
rows with ids 1, 2, 3, where row 2 has a null `person`. At the head commit,
`person.age < 1000` keeps ids `[1, 2, 3]` and `person.age != 30` keeps `[2,
3]`. The spec says that "if a parent struct column is null it implies the leaf
column is null"
([spec](https://github.com/apache/iceberg/blob/5e7169168db3d34e29354c6f59ec4d6e420b8d2d/format/spec.md?plain=1#L1421)),
so I'd expect `[1, 3]` and `[3]`. The optional `person.score` gives the right
answer for the same predicates. If I'm reading the results right, arrow-rs
carries the parent's nulls into an optional child but leaves a required child
non-null under a null parent.
`StructArray::flatten` in arrow-array 58.4 ANDs the struct's validity into
each child. With the change below, both predicates keep the expected ids and
the tests in this PR still pass. Each level merges its own nulls into its
children, so it also covers a null `person` over `address` in the doubly nested
case.
```suggestion
// `flatten` ANDs the struct's validity into each child, so a leaf
under a null
// struct reads as null.
let (fields, mut columns) = struct_array.flatten();
let (idx, _) = fields.find(part).ok_or_else(|| {
ArrowError::SchemaError(format!(
"Predicate column nested field `{part}` not found in struct
`{current_name}`."
))
})?;
current = columns.swap_remove(idx);
```
##########
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:
Could we add a test with an optional parent struct that has null rows? It
would cover a required leaf and an optional leaf under the null struct, with
predicates that the stored child value satisfies (`<` and `!=`), and a null
outer struct in the doubly nested case. Both tests here use required structs,
so they don't reach the null-parent path. They also repeat the same
write-then-read scaffolding, so a helper that writes one batch and returns the
ids a predicate keeps would keep the new cases short.
--
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]
