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: [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]
