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: [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]
