You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "laskoviymishka (via GitHub)" <gi...@apache.org> on 2026/10/05 19:45:07 UTC
Re: [PR] fix: fill schema-added nested columns with typed NULL arrays on read [iceberg-rust]
laskoviymishka commented on code in PR #2635:
URL: https://github.com/apache/iceberg-rust/pull/2635#discussion_r4187834576
##########
crates/iceberg/src/arrow/record_batch_transformer.rs:
##########
@@ -1360,6 +1371,51 @@ 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).unwrap_err();
+ assert_eq!(err.kind(), crate::ErrorKind::FeatureUnsupported);
Review Comment:
The loop runs both `required` values but the asserts don't say which case failed. I'd thread `required` into the messages (`"required={required}"`) or split into two `#[test]`s, so a failure points at the right branch.
##########
crates/iceberg/src/arrow/value.rs:
##########
@@ -526,24 +526,24 @@ 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 (#2618). The `1` is
+ // `new_null_array`'s row count.
+ if prim_lit.is_none() {
+ return Ok(new_null_array(data_type, 1));
Review Comment:
This collapses the Decimal / Timestamp-with-tz / Struct / Null `None` arms into one `new_null_array` call, but nothing exercises either helper with `None` directly anymore — the transformer tests only reach it through the full read path. I'd add a small table-driven test over both `create_primitive_array_single_element` and `_repeated` for `Decimal128(10,2)`, `Timestamp(µs, Some("+00:00"))`, a nested `Struct`, and `Null`, asserting data type, length, and null_count. That re-pins the timezone/precision preservation the deleted arms carried, and covers `num_rows == 0`.
##########
crates/iceberg/src/arrow/value.rs:
##########
@@ -526,24 +526,24 @@ 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 (#2618). The `1` is
Review Comment:
Small thing while we're here: the inline `#2618` goes stale once this merges, and spelling out that `1` is the row count narrates the call. I'd trim to just the intent — "single NULL of any (possibly nested) type."
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org