Copilot commented on code in PR #3658:
URL: https://github.com/apache/iceberg-rust/pull/3658#discussion_r4508448163
##########
crates/iceberg/src/arrow/reader/projection.rs:
##########
@@ -177,45 +179,44 @@ impl ArrowReader {
type_promotion_is_valid: fn(Option<&PrimitiveType>,
Option<&PrimitiveType>) -> bool,
) -> Result<ProjectionMask> {
let mut column_map = HashMap::with_capacity(leaf_field_ids.len());
- 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();
- // Pre-project only the fields that have been selected, possibly
avoiding converting
- // some Arrow types that are not yet supported.
- let projected_arrow_schema = ArrowSchema::new_with_metadata(
- fields.filter_leaves(|_, f| {
- parquet_field_id(f).is_some_and(|field_id|
leaf_field_id_set.contains(&field_id))
- }),
- arrow_schema.metadata().clone(),
- );
- let iceberg_schema = arrow_schema_to_schema(&projected_arrow_schema)?;
-
- fields.filter_leaves(|idx, field| {
- let Some(field_id) = parquet_field_id(field) else {
- return false;
- };
-
- let iceberg_field = iceberg_schema_of_task.field_by_id(field_id);
- let parquet_iceberg_field = iceberg_schema.field_by_id(field_id);
-
- if iceberg_field.is_none() || parquet_iceberg_field.is_none() {
- return false;
+ // Convert selected leaves one at a time, only converting the selected
Arrow types.
+ // Converting a pre-projected schema fails when a map keeps only its
key (#3557).
+ let mut selected_leaves = Vec::with_capacity(leaf_field_ids.len());
+ arrow_schema.fields().filter_leaves(|idx, field| {
+ if let Some(field_id) = parquet_field_id(field)
+ && leaf_field_id_set.contains(&field_id)
+ {
+ selected_leaves.push((idx, field_id, field.clone()));
}
+ false
+ });
+ // Must stay out of the map value rescue below, which would cast them
instead.
+ let mut type_rejected_leaves = HashSet::new();
+ for (idx, field_id, field) in selected_leaves {
+ let Some(iceberg_field) =
iceberg_schema_of_task.field_by_id(field_id) else {
+ continue;
+ };
+ let file_schema =
arrow_schema_to_schema(&ArrowSchema::new(vec![field]))?;
Review Comment:
Per-leaf conversion loses duplicate-ID validation for ancestor fields and
for leaves rejected before insertion into `column_map`. Two selected top-level
structs with the same parent ID and distinct child IDs now pass validation,
allowing the transformer's ID map to overwrite one struct and silently lose its
data. Validate ID uniqueness across selected leaves and their ancestor fields
before type-promotion filtering.
##########
crates/iceberg/src/arrow/reader/projection.rs:
##########
@@ -329,6 +331,83 @@ pub(super) fn find_leaf_by_field_id(
})
}
+/// Parquet leaf index ranges of one map's key and value.
+struct MapLeafRanges {
+ key: Range<usize>,
+ value: Range<usize>,
+ value_is_struct: bool,
+}
+
+/// Collects map leaf ranges in post-order, recognising maps the way
parquet-rs does.
+fn collect_map_leaf_ranges(ty: &ParquetType, next_leaf: &mut usize, maps: &mut
Vec<MapLeafRanges>) {
+ if ty.is_primitive() {
+ *next_leaf += 1;
+ return;
+ }
+ let is_map = matches!(
+ ty.get_basic_info().converted_type(),
+ ConvertedType::MAP | ConvertedType::MAP_KEY_VALUE
+ );
+ if let (true, [entries]) = (is_map, ty.get_fields())
+ && !entries.is_primitive()
+ && let [key, value] = entries.get_fields()
+ {
+ let key_start = *next_leaf;
+ collect_map_leaf_ranges(key, next_leaf, maps);
+ let value_start = *next_leaf;
+ collect_map_leaf_ranges(value, next_leaf, maps);
+ maps.push(MapLeafRanges {
+ key: key_start..value_start,
+ value: value_start..*next_leaf,
+ value_is_struct: !value.is_primitive()
+ && value.get_basic_info().converted_type() ==
ConvertedType::NONE,
+ });
+ } else {
+ for field in ty.get_fields() {
+ collect_map_leaf_ranges(field, next_leaf, maps);
+ }
+ }
+}
+
+/// Parquet reads a map only when both its key and value are projected. When
the key is selected
+/// but no value leaf matched the table (#3557), also read the value leaves:
they match no table
+/// field and come back as a struct of NULLs (#3437). Only struct values are
rescued, because a
+/// primitive value is matched by position and would be cast.
+fn add_map_value_leaves(
+ parquet_schema: &SchemaDescriptor,
+ indices: &mut Vec<usize>,
+ type_rejected_leaves: &HashSet<usize>,
+) -> Result<()> {
+ let mut maps = Vec::new();
+ collect_map_leaf_ranges(parquet_schema.root_schema(), &mut 0, &mut maps);
+ if maps.is_empty() {
+ return Ok(());
+ }
+ let mut selected: HashSet<usize> = indices.iter().copied().collect();
+ // Reversed post-order visits an outer map before the maps nested in its
value.
+ for map in maps.iter().rev() {
+ let key_read = map.key.clone().any(|idx| selected.contains(&idx));
+ let value_read = map.value.clone().any(|idx| selected.contains(&idx));
+ if !key_read || value_read || !map.value_is_struct {
+ continue;
+ }
+ let value_leaves: Vec<usize> = map
+ .value
+ .clone()
+ .filter(|idx| !type_rejected_leaves.contains(idx))
+ .collect();
Review Comment:
Rescued leaves can still be consumed when a struct child matches by ID but
its nested list element or map key/value matches by position. For example, a
matching list field with file `list<string>` element ID 7 and table `list<int>`
element ID 6 is rescued here, allowing stored strings to be cast to integers
without passing `type_promotion_is_valid`. Restrict rescue to leaves that will
be discarded, or validate their positional target types before adding them,
with regression coverage for nested lists and maps.
--
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]
