Copilot commented on code in PR #3384:
URL: https://github.com/apache/iceberg-rust/pull/3384#discussion_r4238597499
##########
crates/iceberg/src/arrow/reader/projection.rs:
##########
@@ -422,43 +427,101 @@ pub(super) fn apply_name_mapping_to_arrow_schema(
"Schema already has field IDs - name mapping should not be applied"
);
- let fields_with_mapped_ids: Vec<_> = arrow_schema
+ let fields_with_mapped_ids = arrow_schema
.fields()
.iter()
.map(|field| {
- // Look up this column name in name mapping to get the Iceberg
field ID.
- // Corresponds to Java's ApplyNameMapping visitor which calls
- // nameMapping.find(currentPath()) and returns field.withId() if
found.
- //
- // If the field isn't in the mapping, leave it WITHOUT assigning
an ID
- // (matching Java's behavior of returning the field unchanged).
- // Later, during projection, fields without IDs are filtered out.
- let mapped_field_opt = name_mapping
- .fields()
- .iter()
- .find(|f| f.names().contains(&field.name().to_string()));
-
- let mut metadata = field.metadata().clone();
-
- if let Some(mapped_field) = mapped_field_opt
- && let Some(field_id) = mapped_field.field_id()
- {
- // Field found in mapping with a field_id → assign it
- metadata.insert(PARQUET_FIELD_ID_META_KEY.to_string(),
field_id.to_string());
- }
- // If field_id is None, leave the field without an ID (will be
filtered by projection)
-
- Field::new(field.name(), field.data_type().clone(),
field.is_nullable())
- .with_metadata(metadata)
+ let mapped_field = find_mapped_field(name_mapping.fields(),
field.name());
+ apply_name_mapping_to_field(field, mapped_field)
})
- .collect();
+ .collect::<Result<Fields>>()?;
Ok(Arc::new(ArrowSchema::new_with_metadata(
fields_with_mapped_ids,
arrow_schema.metadata().clone(),
)))
}
+/// Assigns `field` the ID of `mapped_field`, its entry in the name mapping,
and assigns each
+/// nested field the ID of its entry among `mapped_field`'s children.
+///
+/// Corresponds to Java's ApplyNameMapping visitor, which calls
nameMapping.find(currentPath())
+/// for every field at every depth. As in Java, a list element is looked up as
`element` and a
+/// map key and value as `key` and `value`, whatever the file calls them.
+///
+/// A field the mapping does not cover keeps no ID, and so does everything
nested in it, which
+/// leaves it out of the projection. Java likewise returns such a field
unchanged.
+fn apply_name_mapping_to_field(
+ field: &FieldRef,
+ mapped_field: Option<&MappedField>,
+) -> Result<FieldRef> {
+ let Some(mapped_field) = mapped_field else {
+ return Ok(Arc::clone(field));
+ };
+ let map_nested = |nested: &FieldRef, name: &str| {
+ let mapped_nested =
find_mapped_field(mapped_field.fields().iter().map(Arc::as_ref), name);
+ apply_name_mapping_to_field(nested, mapped_nested)
Review Comment:
Add a regression test for a predicate on a nested field in an ID-less,
name-mapped file. This recursion now makes that leaf visible to predicate
planning and changes the result from silently filtering the wrong rows to the
explicit unsupported nested-predicate error described in the PR, but the added
tests exercise projection only; this behavior could regress without detection.
--
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]
