mbutrovich commented on code in PR #2906:
URL: https://github.com/apache/iceberg-rust/pull/2906#discussion_r4126596670
##########
crates/iceberg/src/arrow/reader/predicate_visitor.rs:
##########
@@ -262,20 +270,59 @@ fn constant_bool_array(value: bool, len: usize) ->
BooleanArray {
BooleanArray::new(buffer, None)
}
-/// 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()
+ ))
+ })?;
+ // `flatten` ANDs the struct's validity into each child, so a leaf
under a null
+ // parent struct reads as null (spec: a null parent implies a null
leaf).
+ 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);
Review Comment:
Could we add a read test where one predicate references two leaves of the
same struct, such as `person.age > 25 AND person.score < 250`? Each test in
this PR references one leaf per struct, so `ProjectionMask::leaves` projects a
struct with a single child, and this lookup always lands on index 0. I changed
`columns.swap_remove(idx)` to `columns.swap_remove(0)` and every test in the PR
still passes. With that change, a six-row file with `person: optional
struct<age: required int, score: optional int>` returns ids `[1, 3, 4, 6]` for
the predicate above, where the head commit returns the correct `[1, 6]`. A test
like this would check that the child is found by name when the projected struct
has more than one child.
--
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]
