Rollback UUID support in SQL Layer #3293

g31pranjal · 2025-04-03T17:03:25Z

This PR reverts the SQL layer support for UUID that was introduced in #3198. However, the Record Layer support for UUID stays as it is. This makes this compatible with releases < 4.2.3.0 which was required to maintain plan continuation compatibility for plans serialized with non-UUID Type.

Also adds a yaml-test for explicitly defining the RecordMetadata using proto, and using the TupleField.UUID message in it. Owing to difference in behavior in how this and versions (4.2.3.0 and 4.2.4.0) interprets TupleField.UUID, this PR also blocklists these versions from yaml-testing.

fixes: #3285

alecgrieser

There are a few extra tests that this could probably use. My main question is whether this sets us up well for future work that will use UUIDs, and I'm not sure. In particular, using the message type in MessageTuple means that we can't base the decision on the query plan, which I think will have negative implications on query semantics with continuations.

yaml-tests/src/main/proto/uuid_proto_test.proto

alecgrieser · 2025-04-08T11:32:01Z

...-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/typing/Type.java

-            } else if (TupleFieldsProto.UUID.getDescriptor().equals(messageDescriptor)) {
-                return Type.uuidType(isNullable);


If we just did this revert here and the test changes (and maybe the parser changes), would that be sufficient? Maybe we'd also need to change the way a MessageTuple gets interpreted so that it choses whether to return a struct or UUID based on the type information.

I'm thinking that because the main problem we're trying to solve is that the new Tuple.UUID primitive type was appearing in plans, which means that plans weren't continuable between 4.2.3.0 and earlier, and reverting this line here makes it so that when we generate the Types from the meta-data, we don't use the new UUID type. But leaving the getUuid and setUuid implementations would prepare clients for the future, in that when we enable this line (in say 4.3), they'd be able to use the client to access UUID fields correctly.

Regardless, do we have any direct tests showing what the Type of a UUID field will be interpreted as? I think we may get that indirectly from the new yaml test, but it may be good to have an explicit test of that somewhere.

Yeah, that makes sense. We will indeed need to change the way message translation happens for UUID, which is expected to return struct for UUID if the returned type is STRUCT (old versions) and return a JAVA UUID if the return type is sql.Types.OTHER (since there is no explicit UUID in sql.Types). To achieve this, I have a temporary fix in RowStruct that returns the expected type and supposedly handles both the cases.

Regardless, do we have any direct tests showing what the Type of a UUID field will be interpreted as? I think we may get that indirectly from the new yaml test, but it may be good to have an explicit test of that somewhere.

Added in TypeTest.java

That's not a bad test, but I was actually thinking more of a test that would show that the ResultSet returns a UUID or a struct, as appropriate. I have some thoughts in the other comment I left here, but the kind of test I was thinking of may be a bit more work than we'd like.

...elational-core/src/main/java/com/apple/foundationdb/relational/recordlayer/MessageTuple.java

yaml-tests/src/test/resources/uuid-proto.yamsql

alecgrieser · 2025-04-08T15:52:19Z

yaml-tests/src/test/resources/uuid-proto.yamsql

+  connect: "jdbc:embed:/FRL/UUID_PROTO_YAML?schema=UUID_PROTO_SCHEMA"
+  tests:
+    -
+      # test select *


It would be good to have a test where more than one record is returned, as that's going to be more useful for testing continuations. The most interesting scenarios for that probably happen between this and a hypothetical future version that re-enables UUIDs, but if I'm right, the continuations for that one will be a bit weird in that they should alternate between structs and UUIDs based on the version chosen for each request, not just the starting version.

Yeah, thats right. The future version that brings back UUID will not be compatible with older version when that is executing first for the reason you mentioned.

yaml-tests/src/test/resources/uuid-proto.yamsql

alecgrieser · 2025-04-11T13:59:00Z

fdb-relational-core/src/main/java/com/apple/foundationdb/relational/api/RowStruct.java

                return getArray(oneBasedPosition);
            case Types.BINARY:
                return getBytes(oneBasedPosition);
+            case Types.OTHER:


Okay, I'm still trying to piece this together, but I think this works. If I'm understanding the intent of this function, it seems to be that it:

Given a field position, looks up the column type of the field

For a UUID, that meta-data may return that it's a STRUCT (in which case we get a nested message back) or an OTHER, in which case we look at whether the field is a TupleFieldsProto.UUID and then parse it as a UUID if so

The struct meta-data that this uses comes from the query plan's Type, which for plans coming from a continuation, will match the value coming from the serialized plan:

fdb-record-layer/fdb-relational-core/src/main/java/com/apple/foundationdb/relational/recordlayer/query/QueryPlan.java

Line 376 in ee8f327

return new RecordLayerResultSet(metaData, iterator, connection,

Which I think means that this should do what we expect, and should allow us to, for each execution of a run with a UUID type, either get UUIDs back or get structs back based on the type chosen by the plan's Type information.

It would be nice if we had some kind of test that that would work, but the only thing I can think of would be to take an existing plan with a UUID-as-struct, and then manually update the Type information to use the new UUID primitive type.

A different approach might be to re-enable 4.2.4.0 or 4.2.3.0 locally, and then run the uuidProto tests. They should fail, but they should fail in a way that let's us consistently return UUIDs if the initial version is below !current_version. We'd need to revert that change, though, before checking it in.

If this is wrong, I think this is something that we'd figure out in the future when we re-enable UUIDs (say, in 4.3). So, we could just say that we'll proceed for now under the assumption that this works, and then make adjustments if that ends up not being the case.

Yes. That's my thinking.

I actually tested locally, with 4.2.3.0 both ways, 4.2.3.0 -> current & the other way round, and it seems to be working as expected for 4.2.3.0 -> current, but not for current -> 4.2.3.0. The reason for failure of the later is again related to the fact that in 4.2.3.0 the conversion from TupleFieldsProto.UUID message -> JAVA UUID happens without checking the expected type. This is something that we should fix in future version to maintain backward compatibility with !current_version.

As you mentioned, I cannot includ these tests in PR since it doesn't entirely pass, but have tested locally satisfactorily and with additional test suites.

alecgrieser · 2025-04-11T14:20:41Z

...-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/typing/Type.java

-            } else if (TupleFieldsProto.UUID.getDescriptor().equals(messageDescriptor)) {
-                return Type.uuidType(isNullable);


That's not a bad test, but I was actually thinking more of a test that would show that the ResultSet returns a UUID or a struct, as appropriate. I have some thoughts in the other comment I left here, but the kind of test I was thinking of may be a bit more work than we'd like.

This PR brings back the complete support for UUID as a primitive data type. The support was first brought in with PR:#3198, but was later partially rolled back in #3293. It also brings in UUID-relevant parts from patch: #3395, that was based on top 4.2.4.0, onto the main.

g31pranjal added 2 commits April 3, 2025 17:36

revert UUID in SQL

12ba5ef

drop 4.2.3.0 and 4.2.4.0 from yaml-testing

1c13e3b

g31pranjal requested a review from alecgrieser April 3, 2025 17:03

g31pranjal added the breaking change Changes that are not backwards compatible label Apr 3, 2025

update

b5767c4

g31pranjal added the bug fix Change that fixes a bug label Apr 3, 2025

g31pranjal added 4 commits April 3, 2025 18:40

adds test for selecting fields other than UUID

840373d

removing test to check if UUID is parsable

33f80d4

commenting out test, with ref to issue

dfe746e

style

b69e51e

alecgrieser requested changes Apr 8, 2025

View reviewed changes

bring back relational support to get/set UUID

cbf5815

g31pranjal force-pushed the revert_UUID branch from 2adcfc4 to cbf5815 Compare April 10, 2025 22:42

g31pranjal requested a review from alecgrieser April 11, 2025 12:54

add test

035bc08

alecgrieser approved these changes Apr 11, 2025

View reviewed changes

g31pranjal force-pushed the revert_UUID branch from a73d5d5 to f949d5d Compare April 11, 2025 19:31

g31pranjal added 2 commits April 11, 2025 20:31

Merge remote-tracking branch 'upstream/main' into revert_UUID

555e74e

fix bug

4d8cede

g31pranjal force-pushed the revert_UUID branch from f949d5d to 4d8cede Compare April 11, 2025 19:32

g31pranjal merged commit d218cbb into FoundationDB:main Apr 11, 2025
5 checks passed

g31pranjal mentioned this pull request Jul 29, 2025

Bring back full support for UUID #3510

Merged

		} else if (TupleFieldsProto.UUID.getDescriptor().equals(messageDescriptor)) {
		return Type.uuidType(isNullable);

Rollback UUID support in SQL Layer #3293

Rollback UUID support in SQL Layer #3293

Uh oh!

Conversation

g31pranjal commented Apr 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alecgrieser left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

g31pranjal commented Apr 3, 2025 •

edited

Loading