mbutrovich commented on code in PR #2930:
URL: https://github.com/apache/iceberg-rust/pull/2930#discussion_r3730479565
##########
crates/iceberg/src/arrow/schema.rs:
##########
@@ -584,6 +600,10 @@ impl ArrowSchemaVisitor for ArrowSchemaConverter {
// fail. The enclosing field's own id is read by the caller.
Ok(Type::Variant(VariantType))
}
+
+ fn uuid(&mut self, _field: &FieldRef) -> Result<Self::T> {
Review Comment:
`variant()` right above this (line 589) explicitly checks
`matches!(field.data_type(), DataType::Struct(_))` before accepting the
extension, with a comment noting it mirrors
`VariantExtensionType::supports_data_type`. This `uuid()` doesn't have the
equivalent check against `FixedSizeBinary(16)`, and `_field` ends up unused as
a result. `Field::extension_type_name()` is just a metadata string lookup, not
validated against the field's actual data type (unlike
`Field::try_extension_type`), so a field tagged `arrow.uuid` with a mismatched
storage type would silently become `Type::Primitive(PrimitiveType::Uuid)` here.
Would it make sense to add the same kind of check `variant()` has?
Separately: `ToArrowSchemaConverter` (the reverse direction, around line
777) maps `PrimitiveType::Uuid` to a bare `FixedSizeBinary(16)` without tagging
the field with `UuidExtensionType`, unlike `variant`'s handling in `field()`
(line 653-658), which does tag the Arrow field. Doesn't that mean an Iceberg
UUID column round-tripped through Arrow and back degrades to `Fixed(16)`? Given
this PR is specifically about making Arrow<->Iceberg UUID conversion work, is
closing the other direction in scope here too?
##########
crates/iceberg/src/arrow/schema.rs:
##########
@@ -1699,6 +1720,27 @@ mod tests {
pretty_assertions::assert_eq!(converted_schema, schema);
}
+ #[test]
+ fn test_arrow_schema_to_schema_should_convert_uuids() {
Review Comment:
This only covers the well-formed case (a `FixedSizeBinary(16)` field tagged
`arrow.uuid`). Given the question above about `uuid()` not validating the
underlying data type, would a test with a mismatched storage type (e.g. `Utf8`
tagged `arrow.uuid`) be useful here to pin down the expected behavior? Also, is
there a round-trip test anywhere (Iceberg uuid -> Arrow -> Iceberg) to confirm
the type survives, or is that not expected to work yet?
--
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]
