c-thiel commented on code in PR #2880:
URL: https://github.com/apache/iceberg-rust/pull/2880#discussion_r4446410210


##########
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
+        };
+
+        // Recover variant identity from the Iceberg schema rather than the 
Parquet `variant`
+        // annotation: tag every variant storage struct with the 
`arrow.parquet.variant`
+        // extension so `arrow_schema_to_schema` folds it back into 
`Type::Variant` instead of
+        // descending into its id-less sub-fields. Mirrors iceberg-java's 
`TypeWithSchemaVisitor`,

Review Comment:
   Fair point, thanks. Fixed in c2835f9c3. The `TypeWithSchemaVisitor` claims 
are gone. The only Java reference left is: like `PruneColumns.variant`, 
projection keeps the whole variant group and skips the type check for it.
   
   The shredded error is now phrased as a known limitation and links the 
tracking issue: "Reading shredded variant column `v` (field id N) is not 
supported yet, see #3606". Shredded read will come as its own PR, since 
arrow-rs already has an unshredding kernel we can build on.
   
   One small note for #3606: the Java property is 
`write.parquet.shred-variants`, and it's off by default.
   



-- 
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]

Reply via email to