This is an automated email from the ASF dual-hosted git repository. github-merge-queue[bot] pushed a commit to branch gh-readonly-queue/main/pr-2635-ecba0b537a1f78ec8125c1a26f6dfab3f95b509e in repository https://gitbox.apache.org/repos/asf/iceberg-rust.git
commit f2409e9f5eaff1c643985013d927e17fdf16e6ec Author: Liang-Chi Hsieh <[email protected]> AuthorDate: Tue Oct 6 17:48:48 2026 +0000 fix: fill schema-added nested columns with typed NULL arrays on read (#2635) * fix: fill schema-added nested columns with typed NULL arrays on read When a column of a nested type (list, map, or a struct with nested children) is added to the table schema after data files were written, reading those older files failed with "unexpected target column type": the helpers that materialize missing columns only handled primitive types plus structs with primitive-only children, via hand-written per-type NULL branches. Build the all-NULL column with arrow's new_null_array instead, which supports every Arrow type (including arbitrarily nested ones), and drop the per-type NULL branches from create_primitive_array_repeated and create_primitive_array_single_element. Closes #2618 * test(arrow): restructure #2618 nested-null test; trim comments Address review feedback on #2635: - Extract the evolved schema into `schema_with_added_nested_columns` and reorder the test to read top-to-bottom as a story (old file -> evolved schema -> expect typed NULLs), per @kevinjqliu. - Assert the all-NULL struct still carries its full nested shape (child fields `a` and the nested list `ys`), locking in that the type-preserving new_null_array fill keeps nested children. - Trim the over-explanatory comments on the new_null_array NULL-fill paths and note that the literal `1` is new_null_array's row count. * test: cover typed null helpers and clarify struct default support * test: compare full evolved schema and link default follow-up --- .../iceberg/src/arrow/record_batch_transformer.rs | 207 ++++++++++++++++++++- crates/iceberg/src/arrow/value.rs | 201 +++++++------------- 2 files changed, 269 insertions(+), 139 deletions(-) diff --git a/crates/iceberg/src/arrow/record_batch_transformer.rs b/crates/iceberg/src/arrow/record_batch_transformer.rs index 19976a81c..fe17d336c 100644 --- a/crates/iceberg/src/arrow/record_batch_transformer.rs +++ b/crates/iceberg/src/arrow/record_batch_transformer.rs @@ -864,13 +864,24 @@ impl RecordBatchTransformer { return Err(invalid_data!("Missing required field: {}", iceberg_field.name)); } - let default_value = iceberg_field.initial_default.as_ref().and_then(|lit| { - if let Literal::Primitive(prim) = lit { - Some(prim.clone()) - } else { - None + // TODO: Support the spec-defined non-null struct initial default `{}` by + // creating a non-null struct and applying each child's initial default. + // Tracked in https://github.com/apache/iceberg-rust/issues/3261. + let default_value = match iceberg_field.initial_default.as_ref() { + None => None, + Some(Literal::Primitive(prim)) => Some(prim.clone()), + Some(_) => { + return Err(Error::new( + ErrorKind::FeatureUnsupported, + format!( + "Cannot read field {} that is absent from the data file: \ + applying a non-primitive initial-default for {} is not yet \ + supported", + iceberg_field.name, iceberg_field.field_type + ), + )); } - }); + }; ColumnSource::Add { value: default_value, @@ -1143,12 +1154,14 @@ mod test { use arrow_cast::cast; use arrow_schema::{DataType, Field, Schema as ArrowSchema}; - use super::field_with_id; + use super::{field_with_id, schema_to_arrow_schema}; use crate::arrow::build_partition_constant; use crate::arrow::record_batch_transformer::{ RecordBatchTransformer, RecordBatchTransformerBuilder, }; - use crate::spec::{Literal, NestedField, PrimitiveType, Schema, Struct, Type}; + use crate::spec::{ + ListType, Literal, MapType, NestedField, PrimitiveType, Schema, Struct, Type, + }; /// Helper to extract string values from either StringArray or RunEndEncoded<StringArray> /// Returns empty string for null values @@ -1360,6 +1373,63 @@ mod test { ); } + #[test] + fn schema_evolution_absent_struct_with_initial_default_errors() { + for required in [false, true] { + let added_field = NestedField::new( + 2, + "added_struct", + Type::Struct(crate::spec::StructType::new(vec![ + NestedField::optional(3, "child", Type::Primitive(PrimitiveType::Int)) + .with_initial_default(Literal::int(42)) + .into(), + ])), + required, + ) + .with_initial_default(Literal::Struct(Struct::from_iter(vec![None]))); + let snapshot_schema = Arc::new( + Schema::builder() + .with_fields(vec![ + NestedField::required(1, "id", Type::Primitive(PrimitiveType::Int)).into(), + added_field.into(), + ]) + .build() + .unwrap(), + ); + let mut transformer = + RecordBatchTransformerBuilder::new(snapshot_schema, &[1, 2]).build(); + let file_schema = Arc::new(ArrowSchema::new(vec![field_with_id( + "id", + DataType::Int32, + false, + 1, + )])); + let file_batch = + RecordBatch::try_new(file_schema, vec![Arc::new(Int32Array::from(vec![1, 2, 3]))]) + .unwrap(); + + let err = transformer + .process_record_batch(file_batch) + .expect_err(&format!( + "required={required}: complex default should be rejected" + )); + assert_eq!( + err.kind(), + crate::ErrorKind::FeatureUnsupported, + "required={required}" + ); + assert!( + err.to_string().contains("Cannot read field added_struct"), + "required={required}: {err}" + ); + assert!( + err.to_string() + .contains("applying a non-primitive initial-default"), + "required={required}: {err}" + ); + } + } + #[test] fn schema_evolution_adds_struct_column_with_nulls() { // Test that when a struct column is added after data files are written, @@ -1437,6 +1507,127 @@ mod test { assert!(struct_column.is_null(2)); } + /// Evolved table schema for the #2618 regression test: `id` plus three + /// later-added optional nested columns — a list, a map, and a struct that + /// itself contains a nested list (`ys`). The nested-in-struct list is the + /// case a per-type NULL-fill would miss. + fn schema_with_added_nested_columns() -> Schema { + Schema::builder() + .with_schema_id(1) + .with_fields(vec![ + NestedField::required(1, "id", Type::Primitive(PrimitiveType::Int)).into(), + NestedField::optional( + 2, + "xs", + Type::List(ListType { + element_field: NestedField::list_element( + 3, + Type::Primitive(PrimitiveType::Int), + false, + ) + .into(), + }), + ) + .into(), + NestedField::optional( + 4, + "props", + Type::Map(MapType { + key_field: NestedField::map_key_element( + 5, + Type::Primitive(PrimitiveType::String), + ) + .into(), + value_field: NestedField::map_value_element( + 6, + Type::Primitive(PrimitiveType::Int), + false, + ) + .into(), + }), + ) + .into(), + NestedField::optional( + 7, + "s", + Type::Struct(crate::spec::StructType::new(vec![ + NestedField::optional(8, "a", Type::Primitive(PrimitiveType::String)) + .into(), + NestedField::optional( + 9, + "ys", + Type::List(ListType { + element_field: NestedField::list_element( + 10, + Type::Primitive(PrimitiveType::Long), + false, + ) + .into(), + }), + ) + .into(), + ])), + ) + .into(), + ]) + .build() + .unwrap() + } + + #[test] + fn schema_evolution_adds_list_map_and_nested_struct_columns_with_nulls() { + // Regression test for https://github.com/apache/iceberg-rust/issues/2618. + // + // The story the test tells, in order: + // 1. An old data file was written with only the `id` column. + // 2. The table schema has since evolved, adding optional list / map / + // struct columns (see `schema_with_added_nested_columns`). + // 3. Reading the old file against the evolved schema must fill those + // absent columns with typed all-NULL arrays — previously this errored + // with "unexpected target column type" for the nested types. + + // (1) The old data file: just `id`. + let file_schema = Arc::new(ArrowSchema::new(vec![field_with_id( + "id", + DataType::Int32, + false, + 1, + )])); + let file_batch = + RecordBatch::try_new(file_schema, vec![Arc::new(Int32Array::from(vec![1, 2, 3]))]) + .unwrap(); + + // (2) Read it against the evolved schema, projecting id + the three added columns. + let snapshot_schema = Arc::new(schema_with_added_nested_columns()); + let projected_iceberg_field_ids = [1, 2, 4, 7]; + let mut transformer = + RecordBatchTransformerBuilder::new(snapshot_schema, &projected_iceberg_field_ids) + .build(); + let result = transformer.process_record_batch(file_batch).unwrap(); + + // (3a) `id` survives unchanged. + assert_eq!(result.num_columns(), 4); + assert_eq!(result.num_rows(), 3); + let id_column = result + .column(0) + .as_any() + .downcast_ref::<Int32Array>() + .unwrap(); + assert_eq!(id_column.values(), &[1, 2, 3]); + + // (3b) The added columns carry the evolved schema's Arrow types, including nested + // field names, nullability, and field IDs, and are all-NULL. + let expected_schema = schema_to_arrow_schema(&schema_with_added_nested_columns()).unwrap(); + assert_eq!(result.schema().as_ref(), &expected_schema); + for (idx, name) in [(1, "xs"), (2, "props"), (3, "s")] { + assert_eq!( + result.column(idx).null_count(), + 3, + "added nested column `{name}` should be all-NULL" + ); + } + } + pub fn source_record_batch() -> RecordBatch { RecordBatch::try_new( arrow_schema_promotion_addition_and_renaming_required(), diff --git a/crates/iceberg/src/arrow/value.rs b/crates/iceberg/src/arrow/value.rs index 080d1b99a..7cbebf68c 100644 --- a/crates/iceberg/src/arrow/value.rs +++ b/crates/iceberg/src/arrow/value.rs @@ -23,7 +23,7 @@ use arrow_array::{ LargeListArray, LargeStringArray, ListArray, MapArray, StringArray, StructArray, Time64MicrosecondArray, TimestampMicrosecondArray, TimestampNanosecondArray, new_null_array, }; -use arrow_buffer::{BooleanBuffer, NullBuffer}; +use arrow_buffer::BooleanBuffer; use arrow_schema::{DataType, FieldRef, TimeUnit}; use uuid::Uuid; @@ -526,24 +526,23 @@ pub(crate) fn create_primitive_array_single_element( data_type: &DataType, prim_lit: Option<&PrimitiveLiteral>, ) -> Result<ArrayRef> { + // No value: a single NULL of any (possibly nested) type. + if prim_lit.is_none() { + return Ok(new_null_array(data_type, 1)); + } match (data_type, prim_lit) { - (DataType::Null, None) => Ok(Arc::new(arrow_array::NullArray::new(1))), (DataType::Boolean, Some(PrimitiveLiteral::Boolean(v))) => { Ok(Arc::new(BooleanArray::from(vec![*v]))) } - (DataType::Boolean, None) => Ok(Arc::new(BooleanArray::from(vec![Option::<bool>::None]))), (DataType::Int32, Some(PrimitiveLiteral::Int(v))) => { Ok(Arc::new(Int32Array::from(vec![*v]))) } - (DataType::Int32, None) => Ok(Arc::new(Int32Array::from(vec![Option::<i32>::None]))), (DataType::Date32, Some(PrimitiveLiteral::Int(v))) => { Ok(Arc::new(Date32Array::from(vec![*v]))) } - (DataType::Date32, None) => Ok(Arc::new(Date32Array::from(vec![Option::<i32>::None]))), (DataType::Int64, Some(PrimitiveLiteral::Long(v))) => { Ok(Arc::new(Int64Array::from(vec![*v]))) } - (DataType::Int64, None) => Ok(Arc::new(Int64Array::from(vec![Option::<i64>::None]))), (DataType::Timestamp(TimeUnit::Microsecond, timezone), Some(PrimitiveLiteral::Long(v))) => { let array = TimestampMicrosecondArray::from(vec![*v]); if let Some(timezone) = timezone { @@ -552,14 +551,6 @@ pub(crate) fn create_primitive_array_single_element( Ok(Arc::new(array)) } } - (DataType::Timestamp(TimeUnit::Microsecond, timezone), None) => { - let array = TimestampMicrosecondArray::from(vec![Option::<i64>::None]); - if let Some(timezone) = timezone { - Ok(Arc::new(array.with_timezone(timezone.clone()))) - } else { - Ok(Arc::new(array)) - } - } (DataType::Timestamp(TimeUnit::Nanosecond, timezone), Some(PrimitiveLiteral::Long(v))) => { let array = TimestampNanosecondArray::from(vec![*v]); if let Some(timezone) = timezone { @@ -568,32 +559,18 @@ pub(crate) fn create_primitive_array_single_element( Ok(Arc::new(array)) } } - (DataType::Timestamp(TimeUnit::Nanosecond, timezone), None) => { - let array = TimestampNanosecondArray::from(vec![Option::<i64>::None]); - if let Some(timezone) = timezone { - Ok(Arc::new(array.with_timezone(timezone.clone()))) - } else { - Ok(Arc::new(array)) - } - } (DataType::Float32, Some(PrimitiveLiteral::Float(v))) => { Ok(Arc::new(Float32Array::from(vec![v.0]))) } - (DataType::Float32, None) => Ok(Arc::new(Float32Array::from(vec![Option::<f32>::None]))), (DataType::Float64, Some(PrimitiveLiteral::Double(v))) => { Ok(Arc::new(Float64Array::from(vec![v.0]))) } - (DataType::Float64, None) => Ok(Arc::new(Float64Array::from(vec![Option::<f64>::None]))), (DataType::Utf8, Some(PrimitiveLiteral::String(v))) => { Ok(Arc::new(StringArray::from(vec![v.as_str()]))) } - (DataType::Utf8, None) => Ok(Arc::new(StringArray::from(vec![Option::<&str>::None]))), (DataType::Binary, Some(PrimitiveLiteral::Binary(v))) => { Ok(Arc::new(BinaryArray::from_vec(vec![v.as_slice()]))) } - (DataType::Binary, None) => Ok(Arc::new(BinaryArray::from_opt_vec(vec![ - Option::<&[u8]>::None, - ]))), (DataType::Decimal128(precision, scale), Some(PrimitiveLiteral::Int128(v))) => { let array = Decimal128Array::from(vec![{ *v }]) .with_precision_and_scale(*precision, *scale) @@ -614,78 +591,6 @@ pub(crate) fn create_primitive_array_single_element( })?; Ok(Arc::new(array)) } - (DataType::Decimal128(precision, scale), None) => { - let array = Decimal128Array::from(vec![Option::<i128>::None]) - .with_precision_and_scale(*precision, *scale) - .map_err(|e| { - invalid_data!( - "Failed to create Decimal128Array with precision {precision} and scale {scale}: {e}" - ) - })?; - Ok(Arc::new(array)) - } - (DataType::Struct(fields), None) => { - // Create a single-element StructArray with nulls - let null_arrays: Vec<ArrayRef> = fields - .iter() - .map(|f| { - // Recursively create null arrays for struct fields - // For primitive fields in structs, use simple null arrays (not REE within struct) - match f.data_type() { - DataType::Boolean => { - Ok(Arc::new(BooleanArray::from(vec![Option::<bool>::None])) - as ArrayRef) - } - DataType::Int32 | DataType::Date32 => { - Ok(Arc::new(Int32Array::from(vec![Option::<i32>::None])) as ArrayRef) - } - DataType::Int64 => { - Ok(Arc::new(Int64Array::from(vec![Option::<i64>::None])) as ArrayRef) - } - DataType::Timestamp(TimeUnit::Microsecond, timezone) => { - let array = TimestampMicrosecondArray::from(vec![Option::<i64>::None]); - if let Some(timezone) = timezone { - Ok(Arc::new(array.with_timezone(timezone.clone())) as ArrayRef) - } else { - Ok(Arc::new(array) as ArrayRef) - } - } - DataType::Timestamp(TimeUnit::Nanosecond, timezone) => { - let array = TimestampNanosecondArray::from(vec![Option::<i64>::None]); - if let Some(timezone) = timezone { - Ok(Arc::new(array.with_timezone(timezone.clone())) as ArrayRef) - } else { - Ok(Arc::new(array) as ArrayRef) - } - } - DataType::Float32 => { - Ok(Arc::new(Float32Array::from(vec![Option::<f32>::None])) as ArrayRef) - } - DataType::Float64 => { - Ok(Arc::new(Float64Array::from(vec![Option::<f64>::None])) as ArrayRef) - } - DataType::Utf8 => { - Ok(Arc::new(StringArray::from(vec![Option::<&str>::None])) as ArrayRef) - } - DataType::Binary => { - Ok( - Arc::new(BinaryArray::from_opt_vec(vec![Option::<&[u8]>::None])) - as ArrayRef, - ) - } - _ => Err(Error::new( - ErrorKind::Unexpected, - format!("Unsupported struct field type: {:?}", f.data_type()), - )), - } - }) - .collect::<Result<Vec<_>>>()?; - Ok(Arc::new(StructArray::new( - fields.clone(), - null_arrays, - Some(NullBuffer::new_null(1)), - ))) - } _ => Err(Error::new( ErrorKind::Unexpected, format!("Unsupported constant type combination: {data_type:?} with {prim_lit:?}"), @@ -702,6 +607,10 @@ pub(crate) fn create_primitive_array_repeated( prim_lit: Option<&PrimitiveLiteral>, num_rows: usize, ) -> Result<ArrayRef> { + // No value to repeat: an all-NULL column of any (possibly nested) type. + if prim_lit.is_none() { + return Ok(new_null_array(data_type, num_rows)); + } Ok(match (data_type, prim_lit) { // --- Primitive Some arms --- (DataType::Boolean, Some(PrimitiveLiteral::Boolean(value))) => { @@ -805,37 +714,6 @@ pub(crate) fn create_primitive_array_repeated( ) } - // --- Special-case None arms --- - (DataType::Decimal128(precision, scale), None) => { - let vals: Vec<Option<i128>> = vec![None; num_rows]; - Arc::new( - Decimal128Array::from(vals) - .with_precision_and_scale(*precision, *scale) - .map_err(|e| { - invalid_data!( - "Failed to create Decimal128Array with precision {precision} and scale {scale}: {e}" - ) - })?, - ) - } - (DataType::Struct(fields), None) => { - // Create a StructArray filled with nulls, recursively creating null children - let null_arrays: Vec<ArrayRef> = fields - .iter() - .map(|field| create_primitive_array_repeated(field.data_type(), None, num_rows)) - .collect::<Result<Vec<_>>>()?; - - Arc::new(StructArray::new( - fields.clone(), - null_arrays, - Some(NullBuffer::new_null(num_rows)), - )) - } - (DataType::Null, None) => Arc::new(arrow_array::NullArray::new(num_rows)), - - // --- Catch-all null arm: use arrow-rs new_null_array for any remaining DataType --- - (dt, None) => new_null_array(dt, num_rows), - (dt, _) => { return Err(Error::new( ErrorKind::Unexpected, @@ -1754,6 +1632,67 @@ mod test { } } + #[test] + fn test_create_null_arrays_preserve_type_and_length() { + let data_types = [ + DataType::Decimal128(10, 2), + DataType::Timestamp(TimeUnit::Microsecond, Some("+00:00".into())), + DataType::Struct( + vec![ + Field::new("a", DataType::Utf8, true), + Field::new( + "ys", + DataType::List(Arc::new(Field::new("element", DataType::Int64, true))), + true, + ), + ] + .into(), + ), + DataType::Null, + ]; + + // NullArray has no validity bitmap; logical_null_count includes its implicit NULLs. + for data_type in data_types { + let single = create_primitive_array_single_element(&data_type, None) + .unwrap_or_else(|err| panic!("single, type={data_type:?}: {err}")); + assert_eq!(single.data_type(), &data_type, "single, type={data_type:?}"); + assert_eq!(single.len(), 1, "single, type={data_type:?}"); + if data_type != DataType::Null { + assert_eq!(single.null_count(), 1, "single, type={data_type:?}"); + } + assert_eq!(single.logical_null_count(), 1, "single, type={data_type:?}"); + + for num_rows in [0, 1, 3] { + let repeated = create_primitive_array_repeated(&data_type, None, num_rows) + .unwrap_or_else(|err| { + panic!("repeated, type={data_type:?}, rows={num_rows}: {err}") + }); + assert_eq!( + repeated.data_type(), + &data_type, + "repeated, type={data_type:?}, rows={num_rows}" + ); + assert_eq!( + repeated.len(), + num_rows, + "repeated, type={data_type:?}, rows={num_rows}" + ); + if data_type != DataType::Null { + assert_eq!( + repeated.null_count(), + num_rows, + "repeated, type={data_type:?}, rows={num_rows}" + ); + } + assert_eq!( + repeated.logical_null_count(), + num_rows, + "repeated, type={data_type:?}, rows={num_rows}" + ); + } + } + } + #[test] fn test_create_null_array_rejects_non_null_literal() { let literal = Some(PrimitiveLiteral::Int(1));
