viirya commented on code in PR #2635:
URL: https://github.com/apache/iceberg-rust/pull/2635#discussion_r4189621699
##########
crates/iceberg/src/arrow/value.rs:
##########
@@ -628,23 +627,25 @@ pub(crate) fn create_primitive_array_single_element(
data_type: &DataType,
prim_lit: &Option<PrimitiveLiteral>,
) -> Result<ArrayRef> {
+ // With no value, the single element is NULL. `new_null_array` supports
every
+ // Arrow type, including nested ones (list/map/struct), which matters for
+ // columns added by schema evolution after a data file was written (#2618).
+ if prim_lit.is_none() {
Review Comment:
#3261 currently covers a missing child within an existing struct. I’ll
extend the follow-up there to include materializing a missing parent’s `{}`
default, along with the helper rename and `Literal` support. The TODO now links
to #3261 in 6c79fc72.
##########
crates/iceberg/src/arrow/record_batch_transformer.rs:
##########
@@ -1437,6 +1506,173 @@ 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 and
are all-NULL.
+ assert!(matches!(
+ result.schema().field(1).data_type(),
+ DataType::List(_)
+ ));
+ assert!(matches!(
+ result.schema().field(2).data_type(),
+ DataType::Map(_, _)
+ ));
+ 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"
+ );
+ }
+
+ // (3c) The all-NULL struct still carries its full nested shape (`a`
plus the
+ // nested list `ys`), not a degenerate empty struct — this is what the
+ // type-preserving NULL fill guarantees over an enumerate-each-type
fix.
+ let result_schema = result.schema();
+ let DataType::Struct(struct_fields) =
result_schema.field(3).data_type() else {
+ panic!("field `s` should be a struct");
+ };
+ let child_names: Vec<&str> = struct_fields.iter().map(|f|
f.name().as_str()).collect();
+ assert_eq!(child_names, vec!["a", "ys"]);
+
+ let DataType::List(xs_element) = result_schema.field(1).data_type()
else {
+ panic!("field `xs` should be a list");
+ };
+ let DataType::Map(entries, _) = result_schema.field(2).data_type()
else {
+ panic!("field `props` should be a map");
+ };
+ let DataType::Struct(map_fields) = entries.data_type() else {
+ panic!("map entries should be a struct");
+ };
+ let DataType::List(ys_element) = struct_fields[1].data_type() else {
+ panic!("field `ys` should be a list");
+ };
+ for (field, expected_id) in [
+ (xs_element, "3"),
+ (&map_fields[0], "5"),
+ (&map_fields[1], "6"),
+ (&struct_fields[0], "8"),
+ (&struct_fields[1], "9"),
+ (ys_element, "10"),
+ ] {
+ assert_eq!(
+ field
+ .metadata()
+ .get(PARQUET_FIELD_ID_META_KEY)
+ .map(String::as_str),
+ Some(expected_id),
+ "nested field `{}` should preserve its field ID",
+ field.name()
+ );
+ }
Review Comment:
Agreed — replaced the individual type, name, and field-ID checks with a full
schema comparison using `schema_to_arrow_schema`, while keeping the per-column
NULL-count assertions. This also covers nested nullability. Updated the import
as suggested; the transformer tests pass.
--
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]
