andygrove opened a new issue, #3374: URL: https://github.com/apache/iceberg-rust/issues/3374
### Apache Iceberg Rust version main @ `776648bee4c52ece8a057cfcf0d7756416299ba8` ### Describe the bug [`apply_name_mapping_to_arrow_schema`](https://github.com/apache/iceberg-rust/blob/776648bee4c52ece8a057cfcf0d7756416299ba8/crates/iceberg/src/arrow/reader/projection.rs#L412-L460) in `crates/iceberg/src/arrow/reader/projection.rs` assigns name-mapped field ids to the top-level fields of a Parquet file's Arrow schema only. A struct, list or map column gets its own id, but the fields inside it keep none, even when the mapping lists them under `fields`. The projection works on leaf columns by field id, so it never selects those nested leaves. What the reader returns then depends on the rest of the projection. If the task also projects a top-level primitive column, the projection mask reads just that column, and `RecordBatchTransformer` fills the struct, list or map column with NULL as if the file did not contain it. If the task projects only nested columns, no leaf matches at all, `get_arrow_projection_mask_with_field_ids` falls back to `ProjectionMask::all()`, and the values come out right. So the same file returns NULL or real values depending on whether a top-level column is read alongside. That is a silent wrong result for any table with nested columns that was migrated with `migrate`, `snapshot` or `add_files`, since those set `schema.name-mapping.default`. Apache DataFusion Comet hit it in its native Iceberg scan: apache/datafusion-comet#6790. The function is the same at `bb1e4a4` (the rev Comet 1.1 pins) and on `main`. ### To Reproduce Added to the tests in `crates/iceberg/src/arrow/reader/projection.rs`, next to `test_read_parquet_with_name_mapping_uses_mapped_field_ids`: ```rust #[tokio::test] async fn name_mapping_nested_struct() { // 1: id int, 2: s struct<3: a int, 4: b int> let schema = Arc::new( Schema::builder() .with_schema_id(1) .with_fields(vec![ NestedField::optional(1, "id", Type::Primitive(PrimitiveType::Int)).into(), NestedField::optional( 2, "s", Type::Struct(StructType::new(vec![ NestedField::optional(3, "a", Type::Primitive(PrimitiveType::Int)).into(), NestedField::optional(4, "b", Type::Primitive(PrimitiveType::Int)).into(), ])), ) .into(), ]) .build() .unwrap(), ); // A Parquet file with no field ids at any level. let struct_fields = arrow_schema::Fields::from(vec![ Field::new("a", DataType::Int32, true), Field::new("b", DataType::Int32, true), ]); let arrow_schema = Arc::new(ArrowSchema::new(vec![ Field::new("id", DataType::Int32, true), Field::new("s", DataType::Struct(struct_fields.clone()), true), ])); // The mapping covers the struct's fields, as Iceberg Java's MappingUtil.create writes it. let name_mapping = Arc::new(NameMapping::new(vec![ MappedField::new(Some(1), vec!["id".to_string()], vec![]), MappedField::new(Some(2), vec!["s".to_string()], vec![ MappedField::new(Some(3), vec!["a".to_string()], vec![]), MappedField::new(Some(4), vec!["b".to_string()], vec![]), ]), ])); let tmp_dir = TempDir::new().unwrap(); let path = format!("{}/1.parquet", tmp_dir.path().to_str().unwrap()); let s_col = arrow_array::StructArray::new( struct_fields, vec![ Arc::new(arrow_array::Int32Array::from(vec![0, 1, 2])) as ArrayRef, Arc::new(arrow_array::Int32Array::from(vec![0, -1, -2])) as ArrayRef, ], None, ); let batch = RecordBatch::try_new(arrow_schema.clone(), vec![ Arc::new(arrow_array::Int32Array::from(vec![0, 1, 2])) as ArrayRef, Arc::new(s_col) as ArrayRef, ]) .unwrap(); let mut writer = ArrowWriter::try_new(File::create(&path).unwrap(), batch.schema(), None).unwrap(); writer.write(&batch).unwrap(); writer.close().unwrap(); for project_field_ids in [vec![1, 2], vec![2]] { let reader = ArrowReaderBuilder::new(FileIO::new_with_fs(), Runtime::current()).build(); let tasks = Box::pin(futures::stream::iter( vec![Ok(FileScanTask::builder() .with_file_size_in_bytes(std::fs::metadata(&path).unwrap().len()) .with_start(0) .with_length(0) .with_data_file_path(path.clone()) .with_data_file_format(DataFileFormat::Parquet) .with_schema(schema.clone()) .with_project_field_ids(project_field_ids.clone()) .with_case_sensitive(false) .with_name_mapping(Some(name_mapping.clone())) .build() .unwrap())] .into_iter(), )) as FileScanTaskStream; let batches: Vec<RecordBatch> = reader.read(tasks).unwrap().stream().try_collect().await.unwrap(); let s = batches[0].column(batches[0].num_columns() - 1).as_struct(); println!("{project_field_ids:?}: {} null structs, a={:?}, b={:?}", s.null_count(), s.column(0), s.column(1)); } } ``` | `project_field_ids` | null `s` | `s.a` | `s.b` | | --- | --- | --- | --- | | `[1, 2]` | 3 of 3 | `[null, null, null]` | `[null, null, null]` | | `[2]` | 0 | `[0, 1, 2]` | `[0, -1, -2]` | Through Comet, the same thing shows up for list element fields and map value fields. ### Expected behavior Both projections return `[0, 1, 2]` and `[0, -1, -2]`, as Iceberg Java does. **What Iceberg Java does.** `ParquetSchemaUtil.applyNameMapping` runs [`ApplyNameMapping`](https://github.com/apache/iceberg/blob/apache-iceberg-1.11.0/parquet/src/main/java/org/apache/iceberg/parquet/ApplyNameMapping.java) over the whole file schema. It calls `nameMapping.find(currentPath())` for every struct, list, map and primitive at every depth, and looks up list elements and map keys and values as `element`, `key` and `value`, whatever the file calls them. **Spec.** The [column projection rules](https://github.com/apache/iceberg/blob/apache-iceberg-1.11.0/format/spec.md?plain=1#L343-L364) say: > * Use `schema.name-mapping.default` metadata to map field id to columns without field id as described below and use the column if it is present. And for the mapping itself: > * `fields`: An optional list of field mappings for child field of structs, maps, and lists. > > ... > > * List types should contain a mapping in `fields` for `element`. > * Map types should contain mappings in `fields` for `key` and `value`. > * Struct types should contain mappings in `fields` for their child fields. **Possible fix.** Apply the mapping recursively. For each Arrow field, find its name among the current level's mapped fields, assign the id, and recurse into the matched field's `fields()` for a struct's children, a list's element (as `element`) and a map's key and value (as `key` and `value`), the way Java's visitor does. ### Willingness to contribute I would be willing to contribute a fix for this bug with guidance from the Iceberg community. --- **Related upstream work.** - #1845 (open) says the name mapping added by #1821 "might not handle nested types properly" and asks for a schema visitor. It was filed as a feature request and doesn't mention that the result is NULL values. - #2612 (merged) made projection and predicates use the name-mapped ids, for top-level columns. - #3269 (open) speeds up the same lookup and keeps it top-level. _This report was drafted with LLM assistance (Claude Code); the behaviour was verified with a Rust reproducer against iceberg-rust `main` at `776648b`, and end to end through Comet against Spark with Iceberg Java 1.5.2 and 1.11.0._ -- 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]
