Add UUID ordering tests #12913
Add UUID ordering tests #12913
Conversation
UUID is stored as two big-endian numbers, so incrementing the value should take this into account.
findepi
added
test
no-release-notes
| java.util.UUID lower = java.util.UUID.fromString(lowerAsString); | ||
| java.util.UUID higher = java.util.UUID.fromString(higherAsString); | ||
| // Java UUID's comparison is not consitent with RFC 4122, see https://bugs.openjdk.org/browse/JDK-7025832 | ||
| assertThat(higher).isLessThan(lower); |
There was a problem hiding this comment.
What's the purpose of testing the JDK implementation of isLessThan? I don't see anything below that relies on this behavior.
There was a problem hiding this comment.
The purpose was to show and document acknowledgement to the fact that JDK UUID behaves differently than our implementation, and also to anchor the explanation, if someone would wonder in the future who's right.
There was a problem hiding this comment.
But we don't use Java UUIDs for ordering, so how is that behavior relevant? If anything, such documentation should be added to the UUID type class so that anyone using it in their connector is aware. This comment is going to be largely invisible for that purpose.
There was a problem hiding this comment.
But we don't use Java UUIDs for ordering
we use it in H2QueryRunner (implicitly)
and in Iceberg library (implicitly, apparently)
such documentation should be added to the UUID type class so that anyone using it in their connector is aware
This isn't supposed to be consumer facing documentation.
It's supposed to capture information for future Trino maintainers, including future myself
There was a problem hiding this comment.
Given that we have javaUuidToTrinoUuid/trinoUuidToJavaUuid public methods in UuidType, that information seems relevant for anyone manipulating UUIDs, no?
There was a problem hiding this comment.
I agree that there can be some other place where the information would be useful too.
it doesn't make it unnecessary in the uuid ordering test.
There was a problem hiding this comment.
As somebody who tends to look at unit tests quite often when getting an understanding of things, I do appreciate this form of documentation for something a bit... quirky / not intuitively obvious when looking at the UUID type class directly.
This could possibly be documented elsewhere as mentioned, but I agree with @findepi that it has utility as a unit test. Particularly, the rest of the test seems to testing that two Trino UUID's are comparable as well as how to do it. If I were working on a feature (say for the Iceberg connector), this is amongst the first places I would likely go for looking at using the code.
If this isn't going to be user facing documentation, then I think this comparison test for the JDK UUIDs is beneficial. Though a comment in UuidType would also suffice for me - but that's more up to you guys. I think the rest of the test is beneficial though.
No description provided.