kevinjqliu commented on code in PR #2635:
URL: https://github.com/apache/iceberg-rust/pull/2635#discussion_r3501789342
##########
crates/iceberg/src/arrow/record_batch_transformer.rs:
##########
@@ -921,6 +923,126 @@ mod test {
assert!(struct_column.is_null(2));
}
+ #[test]
+ fn schema_evolution_adds_list_map_and_nested_struct_columns_with_nulls() {
Review Comment:
nit: this test is a bit difficult to read. do you mind ordering so its
easier to decipher the test?
we're using an evolved schema (with new optional list/map/struct types) to
read the a data file that has the older schema. And we expect that all new
optional list/map/struct types to be null.
should we also test for the shape of the null value? e.g. the null struct
still has the expected child fields?
##########
crates/iceberg/src/arrow/value.rs:
##########
@@ -653,14 +654,6 @@ pub(crate) fn create_primitive_array_single_element(
Ok(Arc::new(array))
}
}
- (DataType::Timestamp(TimeUnit::Microsecond, timezone), None) => {
Review Comment:
just double checking, do you know if `new_null_array` handles this and the
nanosecond case correctly?
##########
crates/iceberg/src/arrow/value.rs:
##########
Review Comment:
maybe a good opportunity for refactoring with
`create_primitive_array_repeated`, i see a lot of repetition. but its outside
this pr's scope 😄
##########
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).
Review Comment:
maybe we should document the `1`, i think its for `new_null_array` 's
`num_rows`
##########
crates/iceberg/src/arrow/value.rs:
##########
@@ -721,81 +700,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| {
- Error::new(
- ErrorKind::DataInvalid,
- format!(
- "Failed to create Decimal128Array with precision
{precision} and scale {scale}: {e}"
- ),
- )
- })?;
- Ok(Arc::new(array))
- }
- (DataType::Struct(fields), None) => {
Review Comment:
nice, glad we can get rid of these with the helper method!
##########
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).
Review Comment:
nit: feels like we're over explaining here, wdyt?
--
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]
