Bring back full support for UUID #3510
Conversation
g31pranjal
added
enhancement
|
We probably want to cut a release with the intermediate support for UUID in JDBC server, before bringing this PR in. |
This PR (1) adds the missing support in `DataType` and gRPC-related classes for the future UUID type. This is needed to maintain compatibility in mixed-mode testing with the future full-support version. It is relatively difficult to test this on the `main` as we cannot bring in the support in the current PR. However, here is the PR for re-introducing the full support: #3510 (2). I have tested that (2) is backward compatible with fixes in (1). Hence, if (1) releases first, and then we bring in (2) we will maintain compatibility to "older" version JDBC servers.
g31pranjal
changed the title
|
This is good to review now! |
| --- | ||
| test_block: | ||
| name: uuid-as-a-field-tests | ||
| preset: single_repetition_ordered |
There was a problem hiding this comment.
is there a reason for adding an explicit preset? if not, can we remove it?
There was a problem hiding this comment.
Yeah, that was stray code. Removed!
| {4, !uuid '5394a912-aa8e-40fc-a4bb-ddf3f89ac45b'}] | ||
| - | ||
| - query: select * from ta where b >= !! !uuid '99e8e8b1-ac34-4f4d-9f01-1f4a7debf4d6' !! | ||
| - unorderedResult: [{3, !uuid 'c35ba01f-f8fc-47d7-bb00-f077e8a75682'}, |
There was a problem hiding this comment.
can you please verify the plan as well?
| final var elementField = messageDescriptor.findFieldByName(NullableArrayTypeUtils.getRepeatedFieldName()); | ||
| final var elementTypeCode = TypeCode.fromProtobufType(elementField.getType()); | ||
| return fromProtoTypeToArray(descriptor, protoType, elementTypeCode, true); | ||
| } else if (TupleFieldsProto.UUID.getDescriptor().equals(messageDescriptor)) { |
There was a problem hiding this comment.
I think we may have discussed this before, but can you please remind me again why are handling the conversion of UUID PB message within a Struct but not within and Array?
There was a problem hiding this comment.
We are adding an extra check here for the case of RECORD, since the descriptor could be that representing a UUID. If so, we do not consider it a "Record" type, but a "UUID" type. This is only needed for this particular case, and not also in Arrays as that dispatches the element processing to this function only.
g31pranjal
force-pushed
the
full_support_for_uuid
branch
from
80e60b7 to
2d20510
Compare
g31pranjal
added
DO NOT MERGE
This PR is a follow-up PR in the series for deprecating using sql_type_codes in JDBC, and instead using "richer" Relational types. The support for relational types were introduced some time bakc in #3510. This PR shifts to relational type usage, away from sql_type usage, to make way for retiring the latter field completely from the protobuf. After this, we could remove the field in 2 steps: 1. stop setting jdbcSqlType fields in JDBC proto and mark them deprecated. 2. remove from protobuf
2 participants
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.