fix: table UUID partitions not working

[JosephLenton]

Which issue does this PR close?

Table Partitions work on writing with UUID values.

• Closes Partitions with type UUID fail to create transactions #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.

[JosephLenton]

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.

[blackmwk]

Thanks @JosephLenton for this fix!

[JosephLenton]

Hey @blackmwk those changes are done. Please take another look when you have time!

Btw whilst working on this I found a few items which I suspect to be bugs. 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 anErr 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) -- fix: typo for the builder error type datafusion#24052
• 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.
• arrow_schema_to_schema ignores the Arrow extension types, and so turns an Arrow UUID into a FixedSize(16) and not an Iceberg UUID type -- fix: arrow schema to schema supports uuids #2930

[mbutrovich]

First pass, thanks for working on this @JosephLenton!

[mbutrovich]

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.

[JosephLenton]

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.

[JosephLenton]

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.

[mbutrovich]

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?

[JosephLenton]

This I have added on my local branch and will push up.