Copilot commented on code in PR #2906:
URL: https://github.com/apache/iceberg-rust/pull/2906#discussion_r3868352535
##########
crates/iceberg/src/arrow/reader/predicate_visitor.rs:
##########
@@ -258,20 +264,55 @@ 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. Predicates
can only
+/// bind to primitive leaves in top-level or struct-nested positions (list/map
interiors
+/// have no accessor), so every path segment before the leaf resolves to 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();
+
+ 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 `{part}` but
found {:?}.",
Review Comment:
The error message is misleading: on a failure to downcast `current` to
`StructArray`, the message says "expected a struct at `{part}`", but `{part}`
is the *next* path segment being resolved, not the segment whose array was
actually non-struct. This makes debugging path resolution errors harder.
##########
crates/iceberg/src/arrow/reader/predicate_visitor.rs:
##########
@@ -258,20 +264,55 @@ 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. Predicates
can only
+/// bind to primitive leaves in top-level or struct-nested positions (list/map
interiors
+/// have no accessor), so every path segment before the leaf resolves to a
struct.
Review Comment:
This doc comment suggests only list/map *interior* fields lack accessors,
but `Schema::build_accessors` does not build accessors for `Type::List` or
`Type::Map` at all (including top-level list/map fields). Updating the wording
would better reflect the actual binding constraints that `project_column`
relies on.
--
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]
