c-thiel commented on code in PR #2880:
URL: https://github.com/apache/iceberg-rust/pull/2880#discussion_r4446353322
##########
crates/iceberg/src/arrow/reader/projection.rs:
##########
@@ -237,6 +269,184 @@ impl ArrowReader {
}
}
+ /// Walks an Arrow `Fields` tree, recording `leaf_idx → variant_field_id`
for every
+ /// leaf that sits inside a variant column (top-level or nested in a
struct/list/map).
+ ///
+ /// The leaf numbering must match [`arrow_schema::Fields::filter_leaves`],
since the
+ /// resulting map is consulted by index in the `filter_leaves` passes
above. In
+ /// particular it mirrors `filter_leaves`' handling of
`Dictionary`/`RunEndEncoded`
+ /// (unwrapped to their value type) and `Union` (each member counted) —
otherwise a
+ /// mismatch would silently shift every variant leaf after it onto the
wrong column.
+ fn collect_variant_leaves(
Review Comment:
Thanks, this is a fair concern. For this PR I kept the walk and extended the
test the way you suggested (c2835f9c3). It now checks every leaf name that
`filter_leaves` produces, plus the exact variant mapping. The siblings are
list-of-struct, large list, fixed-size list and map, plus a `list<variant>`.
About the single-closure idea: the `filter_leaves` closure only gets the
leaf index and the leaf field, so it can't see which variant a leaf belongs to.
It would work if the tagging pass marked each variant's children first.
I'd prefer to fix this properly by choosing leaves from the Parquet
`SchemaDescriptor`, the way iceberg-java prunes the Parquet schema by field id.
That would remove the walk entirely. It changes how projection picks leaves for
every type, not only variants, so I opened #3609 for it. It also matters beyond
variants: #3393 shows how a `ListView` throws the numbering off on main today.
--
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]
