viirya commented on code in PR #2635:
URL: https://github.com/apache/iceberg-rust/pull/2635#discussion_r3972443310
##########
crates/iceberg/src/arrow/value.rs:
##########
@@ -937,34 +794,6 @@ pub(crate) fn create_primitive_array_repeated(
})?,
)
}
- (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| {
- Error::new(
- ErrorKind::DataInvalid,
- format!(
- "Failed to create Decimal128Array with
precision {precision} and scale {scale}: {e}"
- ),
- )
- })?,
- )
- }
- (DataType::Struct(fields), None) => {
- // Create a StructArray filled with nulls
- 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, _) => Arc::new(arrow_array::NullArray::new(num_rows)),
Review Comment:
Good point — changed this to `(DataType::Null, Some(_))`. This makes the
reachable case explicit while preserving the existing behavior for `Null` with
a supplied literal; dropping the arm would make that case return an error.
##########
crates/iceberg/src/arrow/record_batch_transformer.rs:
##########
@@ -921,6 +923,150 @@ 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![
Review Comment:
Agreed — the extraction currently drops non-primitive defaults to `None`.
Added a TODO at that `and_then` to keep the gap visible. I'd keep the
regression test with the complex-default implementation in a follow-up, where
we can cover the non-null struct and child-default semantics together.
##########
crates/iceberg/src/arrow/record_batch_transformer.rs:
##########
@@ -921,6 +923,150 @@ 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![simple_field(
+ "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"]);
Review Comment:
Added assertions for `PARQUET_FIELD_ID` on `a`/`ys` (8/9), the top-level
list element (3), and the map key/value (5/6). I also included the nested `ys`
element (10), so the test covers metadata preservation through that extra
nesting level.
--
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]
