Skip to content

fix: table UUID partitions not working - #2916

Open
JosephLenton wants to merge 2 commits into
apache:mainfrom
JosephLenton:fix-uuid-partitions-fail
Open

fix: table UUID partitions not working#2916
JosephLenton wants to merge 2 commits into
apache:mainfrom
JosephLenton:fix-uuid-partitions-fail

Conversation

@JosephLenton

Copy link
Copy Markdown

Which issue does this PR close?

Table Partitions work on writing with UUID values.

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 JosephLenton changed the title Fix UUID partitions fail fix: table UUID partitions not working Jul 27, 2026
@JosephLenton

Copy link
Copy Markdown
Author

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.

@JosephLenton
JosephLenton force-pushed the fix-uuid-partitions-fail branch from f92395e to de83553 Compare July 27, 2026 19:44

@blackmwk blackmwk left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @JosephLenton for this fix!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should not add more integration tests in this mod, it only exists for compatibility.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want to add such kind of test, you should add some slt test in datafusion integration.

},
RawLiteralEnum::String(v) => match ty {
Type::Primitive(PrimitiveType::String) => Ok(Some(Literal::string(v))),
Type::Primitive(PrimitiveType::Uuid) => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to see ut for this change

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Partitions with type UUID fail to create transactions

2 participants