mbutrovich commented on code in PR #2916: URL: https://github.com/apache/iceberg-rust/pull/2916#discussion_r4444193832
########## Cargo.lock: ########## @@ -24,7 +24,7 @@ version = "0.8.4" source = "registry+https://github.com/rust-lang/crates.io-index"; checksum = "b169f7a6d4742236a0a00c541b845991d0ac43e546831af1249753ab4c3aa3a0" dependencies = [ - "cfg-if 1.0.4", + "cfg-if", Review Comment: Could we drop the `Cargo.lock` changes from this PR? The diff updates many crates the fix doesn't need (for example arrow 59.3.0 to 59.3.2, removes 29 packages and adds 13). The PR builds with the lockfile from its merge base using `cargo test --locked -p iceberg --lib`, and all of the crate's lib tests pass. A lockfile refresh would be easier to review and revert as its own PR. ########## crates/iceberg/src/spec/values/serde.rs: ########## @@ -473,8 +480,20 @@ pub(crate) mod _serde { }, RawLiteralEnum::String(v) => match ty { Type::Primitive(PrimitiveType::String) => Ok(Some(Literal::string(v))), + Type::Primitive(PrimitiveType::Uuid) => Hyphenated::from_str(&v) + // The hyphenated UUID restriction is to match behaviour with Java. + .map(|hyphenated_uuid| Some(Literal::uuid(hyphenated_uuid.into_uuid()))) + .map_err(|_| { + invalid_err_with_reason("string", "UUID must be a valid UUID string") + }), Review Comment: Should this arm say that it is the read path for uuid values in manifests? apache-avro's `deserialize_any` turns a `Value::Uuid` into a string ([`de.rs`](https://github.com/apache/avro-rs/blob/ec5721cb0c80dcde56c1049a004f1d785abd88cf/avro/src/serde/de.rs#L352)), so every uuid partition value read from a manifest comes through here. The error message also rejects strings like `a1a2a3a4b1b2c1c2d1d2d3d4d5d6d7d8`, which are valid UUIDs, so naming the expected form would help whoever hits it. ```suggestion // apache-avro deserializes an Avro uuid as its string form, so uuid // values read from manifests land here. The hyphenated restriction is // to match behaviour with Java. Type::Primitive(PrimitiveType::Uuid) => Hyphenated::from_str(&v) .map(|hyphenated_uuid| Some(Literal::uuid(hyphenated_uuid.into_uuid()))) .map_err(|_| { invalid_err_with_reason( "string", "UUID must be a hyphenated 8-4-4-4-12 string", ) }), ``` ########## crates/iceberg/src/spec/values/serde.rs: ########## @@ -473,8 +480,20 @@ pub(crate) mod _serde { }, RawLiteralEnum::String(v) => match ty { Type::Primitive(PrimitiveType::String) => Ok(Some(Literal::string(v))), + Type::Primitive(PrimitiveType::Uuid) => Hyphenated::from_str(&v) + // The hyphenated UUID restriction is to match behaviour with Java. + .map(|hyphenated_uuid| Some(Literal::uuid(hyphenated_uuid.into_uuid()))) + .map_err(|_| { + invalid_err_with_reason("string", "UUID must be a valid UUID string") + }), _ => Err(invalid_err("string")), }, + RawLiteralEnum::Uuid(v) => match ty { + Type::Primitive(PrimitiveType::Uuid) => { + Ok(Some(Literal::uuid(Uuid::from_bytes(v)))) + } + _ => Err(invalid_err("uuid")), + }, Review Comment: Can this arm be reached? The custom `Deserialize` for `RawLiteralEnum` has no visitor method that builds `RawLiteralEnum::Uuid` ([`visit_str`](https://github.com/apache/iceberg-rust/blob/efc118d64d4fec71d9d8ff52317950f16ee5302a/crates/iceberg/src/spec/values/serde.rs#L234-L237) builds `String`), and `try_into` is only called on deserialized values. When I put a `panic!` in this arm, all `iceberg` lib tests still passed, including the new uuid tests. If the variant only exists for serialization, should this arm return an error with a comment saying so, so readers don't take it for the read path? ########## crates/iceberg/src/avro/schema.rs: ########## @@ -271,9 +272,7 @@ impl SchemaVisitor for SchemaToAvroSchema { PrimitiveType::TimestampNs => AvroSchema::TimestampNanos, PrimitiveType::TimestamptzNs => AvroSchema::TimestampNanos, PrimitiveType::String => AvroSchema::String, - // TODO(#2913): write the spec's `fixed[16]` uuid. The manifest reader - // already accepts both forms. - PrimitiveType::Uuid => AvroSchema::Uuid(UuidSchema::String), + PrimitiveType::Uuid => self.define_once(avro_uuid_schema()?), Review Comment: Could we add a regression test that goes through `ManifestWriter`? #2913 crashes in `ManifestWriter::write_manifest_file`, and the tests here only exercise `RawLiteral` against a one-column schema. A test modeled on [`test_parse_manifest_v2_partition`](https://github.com/apache/iceberg-rust/blob/896f9d6c50ff52fa9e43aec170d3f14bfb40b512/crates/iceberg/src/spec/manifest/mod.rs#L456-L643) with two identity-partitioned uuid columns would check the fix and the `define_once` path together, and could assert the written partition schema against the [spec's Avro mapping for `uuid`](https://github.com/apache/iceberg/blob/5e7169168db3d34e29354c6f59ec4d6e420b8d2d/format/spec.md?plain=1#L1502). I tried that locally. On the merge base it fails with the error from the issue: ``` Could not find matching type in UnionSchema { schemas: [Null, Uuid(String)] } for Bytes([247, 156, 62, ...]) ``` On this PR it passes, and the manifest's partition schema is: ```json {"name": "a", "type": ["null", {"type": "fixed", "name": "uuid_fixed", "size": 16, "logicalType": "uuid"}]}, {"name": "b", "type": ["null", "uuid_fixed"]} ``` -- 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]
