c-thiel commented on code in PR #2880:
URL: https://github.com/apache/iceberg-rust/pull/2880#discussion_r4446228953
##########
crates/iceberg/src/arrow/reader/projection.rs:
##########
@@ -171,16 +161,50 @@ impl ArrowReader {
arrow_schema: &ArrowSchemaRef,
type_promotion_is_valid: fn(Option<&PrimitiveType>,
Option<&PrimitiveType>) -> bool,
) -> Result<ProjectionMask> {
- let mut column_map = HashMap::new();
+ // Maps field_id → leaf column indices. `Vec` because a variant
contributes two
+ // leaves (metadata + value) under a single field id.
+ let mut column_map: HashMap<i32, Vec<usize>> = HashMap::new();
let fields = arrow_schema.fields();
// HashSet for O(1) membership checks instead of O(n) slice scans.
let leaf_field_id_set: HashSet<i32> =
leaf_field_ids.iter().copied().collect();
+ // A variant is an Iceberg leaf type but a Parquet group: its
metadata/value
+ // sub-fields carry no embedded field id, so the field-id scan below
never finds
+ // them. Iceberg-java's `PruneColumns` projects the whole variant
group unchanged
+ // (the enclosing struct/list/map re-adds the original group); we
replicate that by
+ // pre-computing, for every Arrow leaf sitting inside a variant
column, the enclosing
+ // variant's field id (numbering matches `filter_leaves`).
+ let variant_leaves = {
+ let mut out = HashMap::new();
+ let mut leaf_idx = 0usize;
+ Self::collect_variant_leaves(
+ fields,
+ &mut leaf_idx,
+ None,
+ iceberg_schema_of_task,
+ &leaf_field_id_set,
+ &mut out,
+ )?;
+ out
+ };
Review Comment:
The walk has to number every Arrow leaf exactly the way `filter_leaves`
does, because the mask is built from those numbers. That means the index has to
keep counting across siblings and nesting levels during one recursive walk.
`filter_leaves` itself does this with a mutable counter in its closure. If each
call returned its own map, it would also have to return the next index, and
every caller would have to merge maps and pass the index on. One shared counter
and one shared map turned out simpler.
To make it easier to read, I renamed the map to
`leaf_idx_to_variant_field_id` and reworded the comment above it. Longer term,
I'd like to replace this walk with a lookup based on the Parquet schema, so we
don't have to mirror arrow's numbering at all. I opened #3609 for it.
--
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]
