Skip to content

fix(arrow): read nested struct fields as null for files without ids or name mapping #3437

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
zhangfengcdt wants to merge 4 commits into
base: main
Choose a base branch
from
Open

fix(arrow): read nested struct fields as null for files without ids or name mapping #3437

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b928889
fix(arrow): read nested struct fields as null for files without ids o…
zhangfengcdt Oct 13, 2026
f31c686
Merge apache/main into fix/3436-nested-fields-without-ids
zhangfengcdt Oct 15, 2026
3406cf2
test(arrow): pin struct validity for id-less struct fields
zhangfengcdt Oct 15, 2026
d96f98a
Address review: map entry count error, drop redundant tests, changelog
zhangfengcdt Oct 16, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/).
* `EncryptedInputFile::metadata()` is now synchronous and derives the plaintext size from key metadata without a storage stat. Remove `.await` from calls to this method.
* AGS1 readers now require `StandardKeyMetadata::file_length` and reject missing or invalid lengths without falling back to a storage stat. AGS1-encrypted manifests, manifest lists, and Puffin files written by earlier development builds without this field must be rewritten using a build that can still read them before upgrading. This matches the Java client's read contract.
* `iceberg_catalog_loader::BoxedCatalogBuilder` has a new required method, `with_runtime`. Types implementing `CatalogBuilder` get it through the blanket impl; types implementing `BoxedCatalogBuilder` directly (for example, a wrapper around another `Box<dyn BoxedCatalogBuilder>`) must implement it, typically by forwarding the runtime to the builder they wrap.
* `ArrowReader` reads nested struct fields as null for Parquet files that have neither field ids nor a name mapping, as iceberg-java does. Earlier versions returned their stored values by position. Tables migrated without field ids need a name mapping (`schema.name-mapping.default`) that covers nested fields to read them.

## [v0.10.1] - 2026-07-28

Expand Down
23 changes: 8 additions & 15 deletions crates/iceberg/src/arrow/reader/projection.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1690,10 +1690,10 @@ message schema {
}

/// Test reading Parquet files without field IDs with nested types (struct).
/// Java's pruneColumnsFallback() projects entire top-level columns including nested content.
/// This test verifies that a top-level struct field is projected correctly with all its nested fields.
/// Java's pruneColumnsFallback() projects entire top-level columns including nested content,
/// but only top-level fields get fallback ids, so struct fields read as null.
#[tokio::test]
async fn test_read_parquet_without_field_ids_with_struct() {
async fn test_read_parquet_without_field_ids_reads_struct_fields_as_null() {
use arrow_array::{Int32Array, StructArray};
use arrow_schema::Fields;

Expand All @@ -1706,13 +1706,13 @@ message schema {
2,
"person",
Type::Struct(StructType::new(vec![
NestedField::required(
NestedField::optional(
3,
"name",
Type::Primitive(PrimitiveType::String),
)
.into(),
NestedField::required(4, "age", Type::Primitive(PrimitiveType::Int))
NestedField::optional(4, "age", Type::Primitive(PrimitiveType::Int))
.into(),
])),
)
Expand Down Expand Up @@ -1807,16 +1807,9 @@ message schema {

let person_array = batch.column(1).as_struct();
assert_eq!(person_array.num_columns(), 2);

let name_array = person_array.column(0).as_string::<i32>();
assert_eq!(name_array.value(0), "Alice");
assert_eq!(name_array.value(1), "Bob");

let age_array = person_array
.column(1)
.as_primitive::<arrow_array::types::Int32Type>();
assert_eq!(age_array.value(0), 30);
assert_eq!(age_array.value(1), 25);
assert_eq!(person_array.null_count(), 0);
assert_eq!(person_array.column(0).null_count(), 2);
assert_eq!(person_array.column(1).null_count(), 2);
}

/// Test reading Parquet files without field IDs with schema evolution - column added in the middle.
Expand Down
192 changes: 70 additions & 122 deletions crates/iceberg/src/arrow/record_batch_transformer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,19 +270,39 @@ impl PromotePlan {
}),
(DataType::Map(source_entries, _), DataType::Map(target_entries, sorted)) => {
match (source_entries.data_type(), target_entries.data_type()) {
(DataType::Struct(source_fields), DataType::Struct(target_fields)) => {
// Key and value match by position, as in iceberg-java's map readers.
(DataType::Struct(source_fields), DataType::Struct(target_fields))
if source_fields.len() == target_fields.len() =>

@comphead comphead Oct 15, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When the field counts differ, this guard falls through to the _ arm, which reports "expected struct-typed map entries" even though the entries are structs. A separate message such as "map entries have N source fields, expected M" would point at the real problem.

@zhangfengcdt zhangfengcdt Oct 16, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I split _ arm and also a count mismatch errors with "map entries have N source fields, expected M".

{
Ok(PromotePlan::Map {
entries: Self::build_struct_children(
source_fields,
target_fields,
snapshot_schema,
constant_fields,
)?,
entries: source_fields
.iter()
.zip(target_fields.iter())
.enumerate()
.map(|(source_index, (source_field, target_field))| {
Ok(ChildPlan::FromSource {
source_index,
plan: Self::build(
source_field.data_type(),
target_field.data_type(),
snapshot_schema,
constant_fields,
)?,
})
})
.collect::<Result<_>>()?,
entry_fields: target_fields.clone(),
field: target_entries.clone(),
sorted: *sorted,
})
}
(DataType::Struct(source_fields), DataType::Struct(target_fields)) => {
Err(invalid_data!(
"map entries have {} source fields, expected {}",
source_fields.len(),
target_fields.len()
))
}
_ => Err(invalid_data!(
"expected struct-typed map entries, got {source_entries:?} and {target_entries:?}"
)),
Expand Down Expand Up @@ -312,29 +332,9 @@ impl PromotePlan {
return Err(invalid_data!("duplicate field id {id} in struct"));
}
}
// Name mapping only assigns top-level ids, so fully id-less children match
// by position when the counts line up, and `build` checks each pair. This is
// a compatibility fallback until recursive name mapping (#1845): a same-type
// reorder reads positionally. Any missing id errors instead of nulling that child.
if source_by_id.len() != source_fields.len() {
if source_by_id.is_empty() && source_fields.len() == target_fields.len() {
return source_fields
.iter()
.zip(target_fields.iter())
.enumerate()
.map(|(source_index, (source_field, target_field))| {
Ok(ChildPlan::FromSource {
source_index,
plan: Self::build(
source_field.data_type(),
target_field.data_type(),
snapshot_schema,
constant_fields,
)?,
})
})
.collect();
}
// Fully id-less children (no field ids and no name mapping) match no target
// field and read as absent, as in iceberg-java. A partial set of ids errors.
if !source_by_id.is_empty() && source_by_id.len() != source_fields.len() {
return Err(invalid_data!(
"cannot reconcile struct fields by id: source fields do not all have field ids"
));
Expand Down Expand Up @@ -1427,7 +1427,7 @@ mod test {
use arrow_array::types::Int32Type;
use arrow_array::{
Array, ArrayRef, Date32Array, Float32Array, Float64Array, Int32Array, Int64Array,
LargeListArray, ListArray, MapArray, RecordBatch, StringArray, StructArray,
LargeListArray, ListArray, MapArray, RecordBatch, StringArray, StructArray, new_null_array,
};
use arrow_buffer::{NullBuffer, OffsetBuffer};
use arrow_cast::cast;
Expand Down Expand Up @@ -1702,16 +1702,28 @@ mod test {
}

#[test]
fn promote_struct_without_source_field_ids_errors() {
fn promote_struct_without_source_field_ids_reads_null() {
let nulls = NullBuffer::from(vec![true, true, false]);
let source = Arc::new(StructArray::new(
Fields::from(vec![Field::new("x", DataType::Int32, true)]),
vec![Arc::new(Int32Array::from(vec![1, 2])) as ArrayRef],
None,
vec![Arc::new(Int32Array::from(vec![1, 2, 3])) as ArrayRef],
Some(nulls.clone()),
)) as ArrayRef;

let err = promote(&source, &evolved_struct_type(), &empty_schema()).unwrap_err();
assert_eq!(err.kind(), ErrorKind::DataInvalid);
assert!(err.to_string().contains("do not all have field ids"));
// Fields read as null and the struct keeps the file's validity, as Spark's reader does.
let out = promote(&source, &evolved_struct_type(), &empty_schema()).unwrap();
let expected = StructArray::new(
Fields::from(vec![
field_with_id("x", DataType::Int32, true, 5),
field_with_id("y", DataType::Int32, true, 6),
]),
vec![
new_null_array(&DataType::Int32, 3),
new_null_array(&DataType::Int32, 3),
],
Some(nulls),
);
assert_eq!(out.as_struct(), &expected);
}

#[test]
Expand Down Expand Up @@ -1749,6 +1761,11 @@ mod test {
map_of(DataType::Int64),
"expected struct-typed map entries",
),
(
map_of(unevolved_struct_type()),
map_of(evolved_struct_type()),
"map entries have 1 source fields, expected 2",
),
] {
let err =
PromotePlan::build(&source, &target, &empty_schema(), &HashMap::new()).unwrap_err();
Expand Down Expand Up @@ -1855,64 +1872,20 @@ mod test {
}

#[test]
fn promote_idless_nested_struct_keeps_data() {
let inner = StructArray::new(
Fields::from(vec![Field::new("x", DataType::Int32, true)]),
vec![Arc::new(Int32Array::from(vec![10, 20, 30])) as ArrayRef],
None,
);
let file = StructArray::new(
Fields::from(vec![Field::new("inner", inner.data_type().clone(), true)]),
vec![Arc::new(inner) as ArrayRef],
None,
);
let out = transform_top_level(
NestedField::optional(
2,
"s",
Type::Struct(StructType::new(vec![
NestedField::optional(
3,
"inner",
Type::Struct(StructType::new(vec![
NestedField::optional(4, "x", Type::Primitive(PrimitiveType::Long))
.into(),
])),
)
.into(),
])),
),
Arc::new(file),
)
.unwrap();
let inner_fields = Fields::from(vec![field_with_id("x", DataType::Int64, true, 4)]);
let expected = StructArray::new(
Fields::from(vec![field_with_id(
"inner",
DataType::Struct(inner_fields.clone()),
true,
3,
)]),
vec![Arc::new(StructArray::new(
inner_fields,
vec![Arc::new(Int64Array::from(vec![10i64, 20, 30])) as ArrayRef],
None,
)) as ArrayRef],
fn promote_idless_map_struct_value_reads_null_fields() {
let value = StructArray::new(
Fields::from(vec![Field::new("v", DataType::Int32, true)]),
vec![Arc::new(Int32Array::from(vec![1, 2])) as ArrayRef],
None,
);
assert_eq!(out.as_struct(), &expected);
}

#[test]
fn promote_idless_map_keeps_data() {
let entries = StructArray::new(
Fields::from(vec![
Field::new("key", DataType::Utf8, false),
Field::new("value", DataType::Int32, true),
Field::new("value", value.data_type().clone(), true),
]),
vec![
Arc::new(StringArray::from(vec!["k1", "k2"])) as ArrayRef,
Arc::new(Int32Array::from(vec![1, 2])) as ArrayRef,
Arc::new(value) as ArrayRef,
],
None,
);
Expand All @@ -1931,20 +1904,27 @@ mod test {
3,
Type::Primitive(PrimitiveType::String),
4,
Type::Primitive(PrimitiveType::Int),
Type::Struct(StructType::new(vec![
NestedField::optional(5, "v", Type::Primitive(PrimitiveType::Int)).into(),
])),
)),
),
Arc::new(map),
)
.unwrap();
let value_fields = Fields::from(vec![field_with_id("v", DataType::Int32, true, 5)]);
let expected_entries = StructArray::new(
Fields::from(vec![
field_with_id("key", DataType::Utf8, false, 3),
field_with_id("value", DataType::Int32, true, 4),
field_with_id("value", DataType::Struct(value_fields.clone()), true, 4),
]),
vec![
Arc::new(StringArray::from(vec!["k1", "k2"])) as ArrayRef,
Arc::new(Int32Array::from(vec![1, 2])) as ArrayRef,
Arc::new(StructArray::new(
value_fields,
vec![new_null_array(&DataType::Int32, 2)],
None,
)) as ArrayRef,
],
None,
);
Expand All @@ -1959,42 +1939,10 @@ mod test {
None,
false,
);
// Map array equality skips the entries field name.
assert_eq!(out.data_type(), expected.data_type());
assert_eq!(out.as_map(), &expected);
}

#[test]
fn promote_idless_same_type_reorder_stays_positional() {
// Without ids a same-type reorder is undetectable; #1845 replaces this fallback.
let source = Arc::new(StructArray::new(
Fields::from(vec![
Field::new("a", DataType::Int32, true),
Field::new("b", DataType::Int32, true),
]),
vec![
Arc::new(Int32Array::from(vec![1, 2])) as ArrayRef,
Arc::new(Int32Array::from(vec![10, 20])) as ArrayRef,
],
None,
)) as ArrayRef;
let fields = Fields::from(vec![
field_with_id("b", DataType::Int32, true, 6),
field_with_id("a", DataType::Int32, true, 5),
]);

let out = promote(&source, &DataType::Struct(fields.clone()), &empty_schema()).unwrap();
let expected = StructArray::new(
fields,
vec![
Arc::new(Int32Array::from(vec![1, 2])) as ArrayRef,
Arc::new(Int32Array::from(vec![10, 20])) as ArrayRef,
],
None,
);
assert_eq!(out.as_struct(), &expected);
}

#[test]
fn promote_large_list_file_column_to_list() {
let element = Arc::new(field_with_id("element", DataType::Int32, true, 3));
Expand Down
Loading