You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "JosephLenton (via GitHub)" <gi...@apache.org> on 2026/07/30 15:56:31 UTC
[PR] fix: arrow schema to schema supports uuids [iceberg-rust]
JosephLenton opened a new pull request, #2930:
URL: https://github.com/apache/iceberg-rust/pull/2930
## Which issue does this PR close?
* This adds UUID support to `arrow_schema_to_schema` and `arrow_schema_to_schema_auto_assign_ids`.
* When they see a field with the extension `ARROW:extension:metadata=arrow.uuid`, then the field is converted to an Iceberg UUID type.
- Closes #2929
## What changes are included in this PR?
The visitor used for converting from Arrow to Iceberg has a UUID section added. This is similar to how it handles `variant`.
## Are these changes tested?
* A new test is added to capture UUID conversion on its own.
* UUID is added to large `arrow_schema_to_schema_auto_assign_ids` test.
## AI Disclosure
AI was used to apply the fix to the visitor, and to add a UUID case to the test `test_arrow_schema_to_schema_with_field_id`. I built a failing test first and reviewed it's code to ensure it's output worked.
The `test_arrow_schema_to_schema_should_convert_uuids` test, commit message, and the bug report, were all written without any AI at all.
--
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
Re: [PR] fix: arrow schema to schema supports uuids [iceberg-rust]
Posted by "JosephLenton (via GitHub)" <gi...@apache.org>.
JosephLenton commented on PR #2930:
URL: https://github.com/apache/iceberg-rust/pull/2930#issuecomment-5823318301
It's all good on the delays @mbutrovich . Cheers for the re-review. I will look through it and rectify those points.
--
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
Re: [PR] fix: arrow schema to schema supports uuids [iceberg-rust]
Posted by "JosephLenton (via GitHub)" <gi...@apache.org>.
JosephLenton commented on code in PR #2930:
URL: https://github.com/apache/iceberg-rust/pull/2930#discussion_r3731550278
##########
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:
I have also added UUID support for the `ToArrowSchemaConverter`.
--
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
Re: [PR] fix: arrow schema to schema supports uuids [iceberg-rust]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
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: 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
Re: [PR] fix: arrow schema to schema supports uuids [iceberg-rust]
Posted by "JosephLenton (via GitHub)" <gi...@apache.org>.
JosephLenton commented on code in PR #2930:
URL: https://github.com/apache/iceberg-rust/pull/2930#discussion_r4135809826
##########
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:
I think I have this done. I broke out all my changes into their own commits for ease of review. The change for this is done here: https://github.com/apache/iceberg-rust/pull/2930/changes/58cb38e68f8244a33b67606f5e0ebfd7faba6dcb
I did also get Claude to help me understand this and write it. Please do raise if you feel my work here is wrong.
--
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
Re: [PR] fix: arrow schema to schema supports uuids [iceberg-rust]
Posted by "JosephLenton (via GitHub)" <gi...@apache.org>.
JosephLenton commented on PR #2930:
URL: https://github.com/apache/iceberg-rust/pull/2930#issuecomment-5818739461
I am still waiting on a review for this.
--
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
Re: [PR] fix: arrow schema to schema supports uuids [iceberg-rust]
Posted by "JosephLenton (via GitHub)" <gi...@apache.org>.
JosephLenton commented on PR #2930:
URL: https://github.com/apache/iceberg-rust/pull/2930#issuecomment-6191178387
Cheers @CTTY ! I have rebased off `main`, and formatted the `Cargo.toml`.
--
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
Re: [PR] fix: arrow schema to schema supports uuids [iceberg-rust]
Posted by "JosephLenton (via GitHub)" <gi...@apache.org>.
JosephLenton commented on code in PR #2930:
URL: https://github.com/apache/iceberg-rust/pull/2930#discussion_r3731548430
##########
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:
I've added tests for both of those. For a Utf8 with an arrow uuid extension, and round trip tests.
--
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
Re: [PR] fix: arrow schema to schema supports uuids [iceberg-rust]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
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: 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
Re: [PR] fix: arrow schema to schema supports uuids [iceberg-rust]
Posted by "JosephLenton (via GitHub)" <gi...@apache.org>.
JosephLenton commented on code in PR #2930:
URL: https://github.com/apache/iceberg-rust/pull/2930#discussion_r3730801129
##########
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:
> Would it make sense to add the same kind of check variant() has?
Yes, this check should be in the logic and I will add it. đź‘Ť
> Given this PR is specifically about making Arrow<->Iceberg UUID conversion work, is closing the other direction in scope here too?
Yes, I believe it should tag it with the extension too like variant. I'll take a look at adding that as well.
Cheers!
--
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
Re: [PR] fix: arrow schema to schema supports uuids [iceberg-rust]
Posted by "CTTY (via GitHub)" <gi...@apache.org>.
CTTY merged PR #2930:
URL: https://github.com/apache/iceberg-rust/pull/2930
--
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
Re: [PR] fix: arrow schema to schema supports uuids [iceberg-rust]
Posted by "JosephLenton (via GitHub)" <gi...@apache.org>.
JosephLenton commented on PR #2930:
URL: https://github.com/apache/iceberg-rust/pull/2930#issuecomment-5208735366
Thank you for the review @mbutrovich . You should find all of your comments addressed. I made all of the changes you suggested, and brought in UUID support on `ToArrowSchemaConverter`.
When you have time if I could get another review please, that would be grand!
--
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
Re: [PR] fix: arrow schema to schema supports uuids [iceberg-rust]
Posted by "JosephLenton (via GitHub)" <gi...@apache.org>.
JosephLenton commented on PR #2930:
URL: https://github.com/apache/iceberg-rust/pull/2930#issuecomment-5894262673
Hey @mbutrovich , can I have a re-review please.
You should find I have rebased my PR off of main, and addressed all of your comments.
--
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
Re: [PR] fix: arrow schema to schema supports uuids [iceberg-rust]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #2930:
URL: https://github.com/apache/iceberg-rust/pull/2930#issuecomment-5805188374
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions.
--
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