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/27 19:30:06 UTC
[PR] Fix UUID partitions fail [iceberg-rust]
JosephLenton opened a new pull request, #2916:
URL: https://github.com/apache/iceberg-rust/pull/2916
## Which issue does this PR close?
Table Partitions work on writing with UUID values.
- Closes #2913
## What changes are included in this PR?
* `PrimitiveLiteral::UInt128` will now convert to a `RawLiteralEnum::String`, when the type is UUID.
* A similar case is added to `RawLiteralEnum` for UUID deserialisation.
## Are these changes tested?
I added:
- an integration test to confirm the UUID partition works.
- a second test to confirm writing all of the primitive types work.
--
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: table UUID partitions not working [iceberg-rust]
Posted by "JosephLenton (via GitHub)" <gi...@apache.org>.
JosephLenton commented on code in PR #2916:
URL: https://github.com/apache/iceberg-rust/pull/2916#discussion_r3845273841
##########
crates/iceberg/src/spec/values/serde.rs:
##########
@@ -238,7 +239,11 @@ pub(crate) mod _serde {
PrimitiveLiteral::Double(v) => RawLiteralEnum::Double(v.0),
PrimitiveLiteral::String(v) => RawLiteralEnum::String(v),
PrimitiveLiteral::UInt128(v) => {
Review Comment:
Sure thing, I'll move it back to a draft π
--
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: table UUID partitions not working [iceberg-rust]
Posted by "JosephLenton (via GitHub)" <gi...@apache.org>.
JosephLenton commented on code in PR #2916:
URL: https://github.com/apache/iceberg-rust/pull/2916#discussion_r3731731796
##########
crates/iceberg/src/spec/values/tests.rs:
##########
@@ -616,6 +634,21 @@ fn test_raw_literal_bytes_uuid_wrong_length() {
check_raw_literal_bytes_error_via_avro(bytes, &Primitive(PrimitiveType::Uuid));
}
+#[test]
+fn test_raw_literal_string_uuid_valid() {
+ let s = "f79c3e09-677c-4bbd-a479-3f349cb785e7";
+ check_raw_literal_string_serde_via_avro(
+ s,
+ Literal::uuid(Uuid::parse_str(s).unwrap()),
+ &Primitive(PrimitiveType::Uuid),
+ );
+}
+
+#[test]
+fn test_raw_literal_string_uuid_invalid() {
Review Comment:
I have added tests for them π
--
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: table UUID partitions not working [iceberg-rust]
Posted by "JosephLenton (via GitHub)" <gi...@apache.org>.
JosephLenton commented on code in PR #2916:
URL: https://github.com/apache/iceberg-rust/pull/2916#discussion_r3747949275
##########
crates/integrations/datafusion/tests/integration_datafusion_test.rs:
##########
@@ -946,3 +947,98 @@ async fn test_insert_into_partitioned() -> Result<()> {
Ok(())
}
+
+#[tokio::test]
+async fn test_insert_into_partitioned_by_uuid() -> Result<()> {
Review Comment:
This I have added on my local branch and will push up.
##########
crates/iceberg/src/spec/values/serde.rs:
##########
@@ -238,7 +239,11 @@ pub(crate) mod _serde {
PrimitiveLiteral::Double(v) => RawLiteralEnum::Double(v.0),
PrimitiveLiteral::String(v) => RawLiteralEnum::String(v),
PrimitiveLiteral::UInt128(v) => {
Review Comment:
Avro 0.21 seems to have a bug in it's Schema reading code handling `{"type": "fixed", "size": 16, "logicalType": "uuid"}`, specifically the `logicalType: uuid` part. It reads the type as `Schema::UUID` and transforms that to `Schema::Bytes`, which is wrong and mismatches in other parts causing a panic.
I'm pretty certain this is fixed in version 0.22, however that is not yet released. The release process is ongoing now.
Options are:
- to go with `{"type": "fixed", "size": 16}` for now, which passes the DataFusion integration tests and seems to work fine. This is affecting a project where I work so this might be something I will use there.
- wait until Avro 0.22 and update.
- add a patch to work around the schema reading right now.
--
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: table UUID partitions not working [iceberg-rust]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on PR #2916:
URL: https://github.com/apache/iceberg-rust/pull/2916#issuecomment-5894416553
Removed the `stale` tag. Thanks for sticking with this @JosephLenton!
--
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: table UUID partitions not working [iceberg-rust]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #2916:
URL: https://github.com/apache/iceberg-rust/pull/2916#issuecomment-5805188133
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
Re: [PR] fix: table UUID partitions not working [iceberg-rust]
Posted by "JosephLenton (via GitHub)" <gi...@apache.org>.
JosephLenton commented on PR #2916:
URL: https://github.com/apache/iceberg-rust/pull/2916#issuecomment-5894353458
Please keep this around for a little longer. It does _not_ need a review.
I plan to return to this PR this week, to try and get UUIDs working in partitions.
--
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: table UUID partitions not working [iceberg-rust]
Posted by "JosephLenton (via GitHub)" <gi...@apache.org>.
JosephLenton commented on code in PR #2916:
URL: https://github.com/apache/iceberg-rust/pull/2916#discussion_r3734433427
##########
crates/iceberg/src/spec/values/serde.rs:
##########
@@ -238,7 +239,11 @@ pub(crate) mod _serde {
PrimitiveLiteral::Double(v) => RawLiteralEnum::Double(v.0),
PrimitiveLiteral::String(v) => RawLiteralEnum::String(v),
PrimitiveLiteral::UInt128(v) => {
Review Comment:
I have made that change (to apply the work `avro/schema.rs` instead of `values/serde.rs`), and it makes a lot more sense.
However I am finding if I use `{"type": "fixed", "size": 16}` works, but `{"type": "fixed", "size": 16, "logicalType": "uuid"}` causes Avro to panic. The latter I am pretty certain I see Java using here: https://github.com/apache/iceberg/blob/0d60b2bb8780d781f4ceb69032418d222b649ae5/core/src/main/java/org/apache/iceberg/avro/TypeToSchema.java#L54-L55
I am wondering if a change is needed in Avro on this. I am going to have to go away and investigate further and come back.
--
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: table UUID partitions not working [iceberg-rust]
Posted by "blackmwk (via GitHub)" <gi...@apache.org>.
blackmwk commented on code in PR #2916:
URL: https://github.com/apache/iceberg-rust/pull/2916#discussion_r3764716821
##########
crates/iceberg/src/avro/schema.rs:
##########
@@ -289,6 +289,21 @@ pub(crate) fn avro_fixed_schema(len: usize) -> Result<AvroSchema> {
}))
}
+/// Build the Avro schema for the Iceberg `uuid` primitive type.
+///
+/// The Iceberg spec maps `uuid` to `{"type": "fixed", "size": 16, "logicalType": "uuid"}`,
+/// i.e. 16 raw bytes, which is what iceberg-java writes.
+fn avro_uuid_schema() -> Result<AvroSchema> {
Review Comment:
I'm confused, how is this different from AvroSchema::Uuid?
##########
crates/integration_tests/src/lib.rs:
##########
@@ -52,6 +54,16 @@ impl GlobalTestFixture {
GlobalTestFixture { catalog_config }
}
+
+ pub async fn rest_catalog(&self) -> RestCatalog {
Review Comment:
Why we need this?
##########
crates/iceberg/src/spec/values/tests.rs:
##########
@@ -240,7 +240,7 @@ fn json_timestamptz_ns_rejects_non_utc_offset() {
// Per the spec, timestamptz_ns single-value serialization must use offset "+00:00"; Java's
// SingleValueParser enforces the same (DateTimeUtil.isUTCTimestamptz). A non-UTC offset is not a
// valid encoding and must be rejected, not silently re-based to UTC.
- let record = serde_json::Value::String("2017-11-16T22:31:08.123456789+05:00".to_string());
+ let record = JsonValue::String("2017-11-16T22:31:08.123456789+05:00".to_string());
Review Comment:
Why is this related?
##########
crates/iceberg/src/avro/schema.rs:
##########
@@ -289,6 +289,21 @@ pub(crate) fn avro_fixed_schema(len: usize) -> Result<AvroSchema> {
}))
}
+/// Build the Avro schema for the Iceberg `uuid` primitive type.
+///
+/// The Iceberg spec maps `uuid` to `{"type": "fixed", "size": 16, "logicalType": "uuid"}`,
+/// i.e. 16 raw bytes, which is what iceberg-java writes.
+fn avro_uuid_schema() -> Result<AvroSchema> {
Review Comment:
Also this pr only changed iceberg schema to avro schema, I think we should also do vice versa?
--
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: table UUID partitions not working [iceberg-rust]
Posted by "JosephLenton (via GitHub)" <gi...@apache.org>.
JosephLenton commented on PR #2916:
URL: https://github.com/apache/iceberg-rust/pull/2916#issuecomment-5116926605
Hey @blackmwk those changes are done. Please take another look when you have time!
Btw I found about 4 items which I suspect to be bugs whilst working on this. I can add them as tickets later when I have time:
* I think UUID fields are missing the metadata `ARROW:extension:metadata=arrow.uuid`, such as here on my PR: https://github.com/apache/iceberg-rust/pull/2916/changes#diff-78166ca072bfb69d2fbd40e6e7c1bbf337c85e52f14abd742efa6a03f898d8c0R1013
* You cannot insert a UUID as a string. i.e. `INSERT ... VALUES ('aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee')` fails. You must use a hex literal instead.
* I managed to get the error `called `Result::unwrap()` on an `Err` value: Execution("type mismatch and can't cast to got Utf8 and FixedSizeBinary(16)")`, and this has a typo in it (the `cast to got ...` part).
* When inserting data into a partitioned table, if the source data is a select statement without a from, then insertion panics. i.e. `INSERT INTO my_table SELECT 123, 456;` fails if the table has a partition.
--
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: table UUID partitions not working [iceberg-rust]
Posted by "JosephLenton (via GitHub)" <gi...@apache.org>.
JosephLenton commented on PR #2916:
URL: https://github.com/apache/iceberg-rust/pull/2916#issuecomment-5095916161
A few notes ... Avro has an `avro::Value::Uuid` type so I did look into adding a `RawLiteralEnum::Uuid` type. Then the types internally could all match up. However this doesn't play well with Serde + `apache_avro::to_value`. `Uuid` will only serialise to bytes or a string.
The other thing I found is with the integration tests. I found when I had multiple tests in the same file it would fail with a Reqwest + Tokio error. I believe in `opendal-core` there is a global Reqwest client, and this will keep sessions alive across multiple tests within the same program (i.e. a single integration test). It doesn't do this if the endpoint changes (this is relevant later).
As a workaround I put my second test into a second file.
I'd be happy to try to solve this in a followup ticket if there is interest, and any other work to make test simpler. The solution is technically straight forward, but would be a chunk of code. We are talking a custom S3 storage layer, changing OpenDal core, or moving to use `test-containers` (as the endpoint will be different across tests). So I've left that out.
--
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: table UUID partitions not working [iceberg-rust]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on code in PR #2916:
URL: https://github.com/apache/iceberg-rust/pull/2916#discussion_r3752999061
##########
crates/iceberg/src/spec/values/serde.rs:
##########
@@ -238,7 +239,11 @@ pub(crate) mod _serde {
PrimitiveLiteral::Double(v) => RawLiteralEnum::Double(v.0),
PrimitiveLiteral::String(v) => RawLiteralEnum::String(v),
PrimitiveLiteral::UInt128(v) => {
Review Comment:
> Avro 0.21 seems to have a bug in it's Schema reading code handling `{"type": "fixed", "size": 16, "logicalType": "uuid"}`, specifically the `logicalType: uuid` part. It reads the type as `Schema::UUID` and transforms that to `Schema::Bytes`, which is wrong and mismatches in other parts causing a panic.
>
> I'm pretty certain this is fixed in version 0.22, however that is not yet released. The release process is ongoing now.
>
> I will come back to this PR once that is out.
Thanks for the initial round of revision @JosephLenton! Since we're blocked on a dependency, should we set this to draft? Then we can tag/request reviews from folks again when we're ready?
--
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: table UUID partitions not working [iceberg-rust]
Posted by "JosephLenton (via GitHub)" <gi...@apache.org>.
JosephLenton commented on code in PR #2916:
URL: https://github.com/apache/iceberg-rust/pull/2916#discussion_r3673524831
##########
crates/iceberg/src/spec/values/serde.rs:
##########
@@ -444,6 +449,12 @@ pub(crate) mod _serde {
},
RawLiteralEnum::String(v) => match ty {
Type::Primitive(PrimitiveType::String) => Ok(Some(Literal::string(v))),
+ Type::Primitive(PrimitiveType::Uuid) => {
Review Comment:
That is added. Let me know if it's not right, or you need anything else.
--
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: table UUID partitions not working [iceberg-rust]
Posted by "JosephLenton (via GitHub)" <gi...@apache.org>.
JosephLenton commented on code in PR #2916:
URL: https://github.com/apache/iceberg-rust/pull/2916#discussion_r3673521109
##########
crates/integration_tests/src/lib.rs:
##########
Review Comment:
I've moved it there π
--
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: table UUID partitions not working [iceberg-rust]
Posted by "JosephLenton (via GitHub)" <gi...@apache.org>.
JosephLenton commented on code in PR #2916:
URL: https://github.com/apache/iceberg-rust/pull/2916#discussion_r3731689752
##########
crates/iceberg/src/spec/values/serde.rs:
##########
@@ -444,6 +449,12 @@ pub(crate) mod _serde {
},
RawLiteralEnum::String(v) => match ty {
Type::Primitive(PrimitiveType::String) => Ok(Some(Literal::string(v))),
+ Type::Primitive(PrimitiveType::Uuid) => {
+ let uuid = Uuid::parse_str(&v).map_err(|_| {
+ invalid_err_with_reason("string", "UUID must be a valid UUID string")
Review Comment:
The leniency was not intentional. I just plugged in UUID, and forgot it accepts such a variety and others wouldn't.
Looking at Java's UUID fromString https://docs.oracle.com/en/java/javase/26/docs/api/java.base/java/util/UUID.html#toString() , I am reading it as only accepting hyphenated UUIDs. I will update this to only support hyphenated UUIDs to match the Java behaviour.
--
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: table UUID partitions not working [iceberg-rust]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on code in PR #2916:
URL: https://github.com/apache/iceberg-rust/pull/2916#discussion_r3730487337
##########
crates/iceberg/src/spec/values/tests.rs:
##########
@@ -616,6 +634,21 @@ fn test_raw_literal_bytes_uuid_wrong_length() {
check_raw_literal_bytes_error_via_avro(bytes, &Primitive(PrimitiveType::Uuid));
}
+#[test]
+fn test_raw_literal_string_uuid_valid() {
+ let s = "f79c3e09-677c-4bbd-a479-3f349cb785e7";
+ check_raw_literal_string_serde_via_avro(
+ s,
+ Literal::uuid(Uuid::parse_str(s).unwrap()),
+ &Primitive(PrimitiveType::Uuid),
+ );
+}
+
+#[test]
+fn test_raw_literal_string_uuid_invalid() {
Review Comment:
This only checks that a clearly invalid string (`"not-a-uuid"`) is rejected. Given the question above about `Uuid::parse_str`'s leniency, would it be worth also asserting what happens with a valid-but-non-canonical string (e.g. a 32-char no-hyphen UUID), so the accept/reject boundary is pinned down one way or the other?
##########
crates/iceberg/src/spec/values/serde.rs:
##########
@@ -444,6 +449,12 @@ pub(crate) mod _serde {
},
RawLiteralEnum::String(v) => match ty {
Type::Primitive(PrimitiveType::String) => Ok(Some(Literal::string(v))),
+ Type::Primitive(PrimitiveType::Uuid) => {
+ let uuid = Uuid::parse_str(&v).map_err(|_| {
+ invalid_err_with_reason("string", "UUID must be a valid UUID string")
Review Comment:
`Uuid::parse_str` accepts hyphenated, non-hyphenated, braced, and urn-prefixed forms (the uuid crate documents this as "a string of hexadecimal digits with optional hyphens"). The spec's single-value serialization for `uuid` is the 36-char canonical hyphenated string, and Java's `UUID.fromString` is stricter than this. Is the extra leniency here intentional, or should this reject anything that isn't the canonical 36-char form?
##########
crates/iceberg/src/spec/values/serde.rs:
##########
@@ -238,7 +239,11 @@ pub(crate) mod _serde {
PrimitiveLiteral::Double(v) => RawLiteralEnum::Double(v.0),
PrimitiveLiteral::String(v) => RawLiteralEnum::String(v),
PrimitiveLiteral::UInt128(v) => {
Review Comment:
The crash in #2913 traces back to `apache_avro`'s `Value::validate_internal`, which has match arms for `(Value::String, Schema::Uuid)` and `(Value::Uuid, Schema::Uuid)` but none for `(Value::Bytes, Schema::Uuid)` (apache-avro 0.21.0, `types.rs` around line 444-451). That diagnosis looks right.
But `Schema::Uuid` in apache_avro always serializes to `{"type": "string", "logicalType": "uuid"}` (`schema.rs:2201-2206`), and a `Value::Uuid` gets encoded as a length-prefixed string for that schema, not raw fixed bytes (`encode.rs:135-142`). The spec's Avro mapping for `uuid` is `{"type": "fixed", "size": 16, "logicalType": "uuid"}` (format/spec.md:936), and iceberg-java's `TypeToSchema.UUID_SCHEMA` builds exactly that, with `UUIDWriter`/`UUIDReader` calling `encoder.writeFixed`/reading a fixed 16-byte buffer.
`avro/schema.rs:237` maps `PrimitiveType::Uuid` to `AvroSchema::Uuid` for the manifest schema, so doesn't this mean a manifest with a UUID partition column written after this fix ends up with that column typed as Avro `string` on disk, not `fixed(16)`? Avro string and fixed aren't resolution-compatible, so would a Java reader (or anything with a fixed(16) reader schema) fail to read it?
Would keeping this arm as `Bytes` (unchanged) and instead building a raw `Fixed(16)` schema with a `logicalType: "uuid"` attribute in `avro/schema.rs` (same pattern `avro_decimal_schema` already uses, wrapping `Fixed` for decimals) avoid the crash without changing the wire format? `Value::Bytes` already resolves fine against `Schema::Fixed`, and apache_avro's own schema parser already folds `fixed(16)+logicalType=uuid` back into `Schema::Uuid` on read, so the existing `RawLiteralEnum::Bytes` deserialize branch for `Uuid` a few lines down wouldn't need to change either.
##########
crates/integrations/datafusion/tests/integration_datafusion_test.rs:
##########
@@ -946,3 +947,98 @@ async fn test_insert_into_partitioned() -> Result<()> {
Ok(())
}
+
+#[tokio::test]
+async fn test_insert_into_partitioned_by_uuid() -> Result<()> {
Review Comment:
This round-trips entirely through iceberg-rust's own reader, so it wouldn't catch a mismatch between the Avro type written for the UUID column and what the spec (or another implementation) expects, since iceberg-rust just reads back what it wrote. Would it be worth asserting on the raw manifest's Avro schema for the partition field here, given that's the part actually in question?
--
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: table UUID partitions not working [iceberg-rust]
Posted by "JosephLenton (via GitHub)" <gi...@apache.org>.
JosephLenton commented on code in PR #2916:
URL: https://github.com/apache/iceberg-rust/pull/2916#discussion_r3747949809
##########
crates/iceberg/src/spec/values/serde.rs:
##########
@@ -238,7 +239,11 @@ pub(crate) mod _serde {
PrimitiveLiteral::Double(v) => RawLiteralEnum::Double(v.0),
PrimitiveLiteral::String(v) => RawLiteralEnum::String(v),
PrimitiveLiteral::UInt128(v) => {
Review Comment:
Avro 0.21 seems to have a bug in it's Schema reading code handling `{"type": "fixed", "size": 16, "logicalType": "uuid"}`, specifically the `logicalType: uuid` part. It reads the type as `Schema::UUID` and transforms that to `Schema::Bytes`, which is wrong and mismatches in other parts causing a panic.
I'm pretty certain this is fixed in version 0.22, however that is not yet released. The release process is ongoing now.
I will come back to this PR once that is out.
--
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: table UUID partitions not working [iceberg-rust]
Posted by "blackmwk (via GitHub)" <gi...@apache.org>.
blackmwk commented on code in PR #2916:
URL: https://github.com/apache/iceberg-rust/pull/2916#discussion_r3664368344
##########
crates/integration_tests/src/lib.rs:
##########
Review Comment:
If you want to add such kind of test, you should add some slt test in datafusion integration.
##########
crates/integration_tests/src/lib.rs:
##########
Review Comment:
We should not add more integration tests in this mod, it only exists for compatibility.
##########
crates/iceberg/src/spec/values/serde.rs:
##########
@@ -444,6 +449,12 @@ pub(crate) mod _serde {
},
RawLiteralEnum::String(v) => match ty {
Type::Primitive(PrimitiveType::String) => Ok(Some(Literal::string(v))),
+ Type::Primitive(PrimitiveType::Uuid) => {
Review Comment:
I want to see ut for this change
--
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