mbutrovich commented on code in PR #2930:
URL: https://github.com/apache/iceberg-rust/pull/2930#discussion_r4097132848
##########
crates/iceberg/Cargo.toml:
##########
@@ -42,7 +42,7 @@ arrow-array = { workspace = true }
arrow-buffer = { workspace = true }
arrow-cast = { workspace = true }
arrow-ord = { workspace = true }
-arrow-schema = { workspace = true }
+arrow-schema = { workspace = true, features = ["canonical_extension_types"] }
Review Comment:
Now that `schema_to_arrow_schema` tags UUID fields with `arrow.uuid`, should
`parquet` get its `arrow_canonical_extension_types` feature too ([line
69](https://github.com/apache/iceberg-rust/blob/445db4a323de87eb226ddb8e55149842857efc83/crates/iceberg/Cargo.toml#L69))?
The `arrow-schema` feature enabled here doesn't turn on the parquet one, and
parquet's UUID mapping is behind its own `cfg(feature =
"arrow_canonical_extension_types")`.
I wrote a UUID column through `ParquetWriter` at this commit and inspected
the file. The Parquet column has no logical type, but the spec requires
`fixed_len_byte_array[16]` with the `UUID` logical type
([spec](https://github.com/apache/iceberg/blob/b5f36393227382296e28568306aa88eb570171b7/format/spec.md?plain=1#L1557)).
Reads also disagree depending on who wrote the file. Our files come back
tagged `arrow.uuid` only because arrow-rs embeds the `ARROW:schema` footer
entry. A file without that entry (Java, Spark, PyIceberg) comes back as a bare
`FixedSizeBinary(16)`, even though `schema_to_arrow_schema` for the same table
now says `arrow.uuid`.
With `features = ["async", "encryption", "arrow_canonical_extension_types"]`
on `parquet`, the writer emits the `UUID` logical type and all three cases read
back tagged. `cargo test -p iceberg --lib` still passes with that change (1580
passed). Could you add it here, with a writer test that asserts the logical
type in the Parquet schema?
##########
crates/iceberg/src/arrow/schema.rs:
##########
@@ -1399,6 +1440,7 @@ mod tests {
use std::collections::HashMap;
use std::sync::Arc;
+ use arrow_schema::extension::Uuid as DataTypeUuidExt;
Review Comment:
`use super::*` already brings in `UuidExtensionType`, and
`test_schema_to_arrow_schema_should_convert_uuid_to_arrow_uuid` uses that name
while the other tests use `DataTypeUuidExt`. Could you drop this import and use
`UuidExtensionType` everywhere so the module has one name for the type?
##########
crates/iceberg/src/arrow/schema.rs:
##########
@@ -249,6 +263,8 @@ fn visit_type<V: ArrowSchemaVisitor>(r#type: &DataType,
visitor: &mut V) -> Resu
fn visit_field<V: ArrowSchemaVisitor>(field: &FieldRef, visitor: &mut V) ->
Result<V::T> {
Review Comment:
The doc comment above `visit_field` still says it only folds the
`arrow.parquet.variant` extension. Could you update it to mention `arrow.uuid`?
##########
crates/iceberg/src/arrow/schema.rs:
##########
@@ -1705,6 +1747,74 @@ mod tests {
pretty_assertions::assert_eq!(converted_schema, schema);
}
+ #[test]
+ fn test_converting_uuid_from_arrow_to_iceberg_to_arrow_should_give_uuid() {
+ let arrow_schema = ArrowSchema::new(vec![
+ simple_field("uuid_field", DataType::FixedSizeBinary(16), false,
"1")
+ .with_extension_type(DataTypeUuidExt),
+ ]);
+ let output =
+
schema_to_arrow_schema(&arrow_schema_to_schema(&arrow_schema).unwrap()).unwrap();
+
+ assert_eq!(arrow_schema, output);
+ }
+
+ #[test]
+ fn
test_converting_uuid_from_iceberg_to_arrow_to_iceberg_should_give_uuid() {
+ let iceberg_schema = Schema::builder()
+ .with_fields(vec![
+ NestedField::optional(1, "uuid_field",
Type::Primitive(PrimitiveType::Uuid)).into(),
+ ])
+ .build()
+ .unwrap();
+ let output =
+
arrow_schema_to_schema(&schema_to_arrow_schema(&iceberg_schema).unwrap()).unwrap();
+
+ assert_eq!(iceberg_schema, output);
+ }
+
+ #[test]
+ fn
test_arrow_schema_to_schema_should_convert_uuid_when_fixed_size_binary() {
+ let converted_schema = arrow_schema_to_schema(&ArrowSchema::new(vec![
+ simple_field("uuid_field", DataType::FixedSizeBinary(16), false,
"1")
+ .with_extension_type(DataTypeUuidExt),
+ ]))
+ .unwrap();
+
+ let expected = Schema::builder()
+ .with_fields([NestedField::required(
+ 1,
+ "uuid_field",
+ Type::Primitive(PrimitiveType::Uuid),
+ )
+ .into()])
+ .build()
+ .unwrap();
+
+ pretty_assertions::assert_eq!(expected, converted_schema);
+ }
+
+ #[test]
+ fn
test_arrow_schema_to_schema_should_reject_uuid_when_not_a_fixed_size_binary() {
+ // The field must be built correctly, and then changed to the wrong
type,
+ // to avoid Arrow's own validation and panic.
+ let mut field = simple_field(
+ "incorrect_uuid_field",
+ DataType::FixedSizeBinary(16),
+ false,
+ "1",
+ )
+ .with_extension_type(DataTypeUuidExt);
+ field = field.with_data_type(DataType::Utf8);
+
+ let error =
arrow_schema_to_schema(&ArrowSchema::new(vec![field])).unwrap_err();
+
+ pretty_assertions::assert_eq!(
+ "arrow.uuid extension requires FixedSizeBinary(16) storage, found
Utf8",
+ error.message()
+ );
+ }
Review Comment:
This pins the case where the storage isn't binary at all. Could you add a
`FixedSizeBinary` with the wrong width (for example 8 or 17) tagged
`arrow.uuid`? That's the case that separates the equality check from a looser
`matches!(.., DataType::FixedSizeBinary(_))`, and nothing else covers it.
--
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]
