Add support for UUID comparison functions #10791

BryanCutler · 2024-08-20T23:07:23Z

This adds binary comparison functions <, >, <=, >= to the UUID custom data type. Equality functions are already present. Added unit tests for testing a query with comparisons between UUID values.

Also fixes the binary representation of UuidType to be a LE int128_t and adjusted Presto serde to put UUID values in the format that Presto Java expects, as 2 long values with msb first.

From #10584, prestodb/presto#23311

netlify · 2024-08-20T23:07:42Z

✅ Deploy Preview for meta-velox ready!

Name	Link
🔨 Latest commit	`5aa91d4`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/67042c058c13de00080b4110
😎 Deploy Preview	https://deploy-preview-10791--meta-velox.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

aditi-pandit

Thanks @BryanCutler. One small comment.

Did you try running the queries in prestodb/presto#23301 with this logic ?

aditi-pandit · 2024-08-21T10:09:10Z

velox/functions/prestosql/UuidFunctions.h

+  }
+};
+
+#define VELOX_GEN_BINARY_EXPR_UUID(Name, uuidCompExpr, TResult)               \


TResult parameter is not used.

Thanks, good catch! It's actually the same in VELOX_GEN_BINARY_EXPR_TIMESTAMP_WITH_TIME_ZONE too

BryanCutler · 2024-08-22T04:42:03Z

Did you try running the queries in prestodb/presto#23301 with this logic ?

Yes, and the queries for <,>,<=,>= now pass. But it is a little strange, they only pass for me in debug mode, when I run in release mode I get this type of error:

not equal
Actual rows (1 of 1 extra rows shown, 6 rows in total):
    [f768f36d-4f09-4da7-a298-3564d8f3c986, 1fdff429-6f65-4370-bb2a-7ab660dbec92]
Expected rows (1 of 1 missing rows shown, 6 rows in total):
    [1fdff429-6f65-4370-bb2a-7ab660dbec92, f768f36d-4f09-4da7-a298-3564d8f3c986]

where it looks like one UUID is randomly generated, and I'm not sure where it's coming from. I'll look into that a bit more.

BryanCutler · 2024-08-22T17:43:33Z

@aditi-pandit I didn't realize the temp table from the test in prestodb/presto#23301 included a randomly generated UUID with (cast(uuid() AS VARCHAR)). That was the reason for failing in release mode, without this random value all the comparison tests pass.

aditi-pandit

Thanks @BryanCutler. Had some minor comments.

velox/functions/prestosql/UuidFunctions.h

aditi-pandit · 2024-08-26T08:13:43Z

velox/functions/prestosql/UuidFunctions.h

+  template <typename T>                                                       \
+  struct Name##Uuid : public UuidCompareBase {                                \
+    VELOX_DEFINE_FUNCTION_TYPES(T);                                           \
+    FOLLY_ALWAYS_INLINE void call(                                            \


Nit : Add newline in between.

@aditi-pandit I don't see a consistent format for this, most of the time it looks how it is currently (when the arguments don't fit within a single line) but I do see some cases where the format is:

template <typename T> struct MultiplyVoidOutputFunction { template <typename TInput> FOLLY_ALWAYS_INLINE void call(TInput& result, const TInput& a, const TInput& b) { result = functions::multiply(a, b); } };

Do you mean the latter, with a newline after void?

@BryanCutler : I meant newline in between

VELOX_DEFINE_FUNCTION_TYPES(T); FOLLY_ALWAYS_INLINE void call(

You can stick with the formatting from make format-fix otherwise.

aditi-pandit · 2024-08-26T08:17:50Z

velox/functions/prestosql/tests/UuidFunctionsTest.cpp

@@ -108,5 +108,45 @@ TEST_F(UuidFunctionsTest, unsupportedCast) {
      evaluate("cast(123 as uuid())", input), "Cannot cast BIGINT to UUID.");
 }

+TEST_F(UuidFunctionsTest, comparisons) {
+  auto data0 = makeFlatVector<std::string>({
+      "33355449-2c7d-43d7-967a-f53cd23215ad",


Might be good to add more tests where the lower 8 bytes are the same for better coverage of the comparison logic

Sure, for the lower bits I have a check for <, = and I'll add one more for >

I added some additional cases, one with equal lower bytes

aditi-pandit

Thanks @BryanCutler.

aditi-pandit · 2024-08-29T09:16:23Z

@Yuhta @kagamiori @mbasmanova : Please can you help with the review.

BryanCutler · 2024-09-09T04:45:25Z

Gentle ping @Yuhta @kagamiori @mbasmanova to please review

mbasmanova

@BryanCutler Thank you for adding this functionality. I wonder where is the semantics of comparing UUIDs come from? Do you have a pointer? Would be nice to update PR description and documentation (https://facebookincubator.github.io/velox/develop/types.html) to clarify.

mbasmanova · 2024-09-09T09:52:40Z

velox/functions/prestosql/UuidFunctions.h

 #include "velox/functions/prestosql/types/UuidType.h"

 namespace facebook::velox::functions {

+struct UuidCompareBase {
+  FOLLY_ALWAYS_INLINE int compare64(uint64_t x, uint64_t y)


These can be static functions or free functions.

Let's also put this code into 'details' namespace.

mbasmanova · 2024-09-09T09:53:07Z

velox/functions/prestosql/UuidFunctions.h

+  }
+
+  int compare128(const int128_t& lhs, const int128_t& rhs) {
+    int comp = compare64(HugeInt::upper(lhs), HugeInt::upper(rhs));


comp

Do not abbreviate: comp -> result or similar.

mbasmanova · 2024-09-09T09:54:29Z

velox/functions/prestosql/tests/UuidFunctionsTest.cpp

+      "33355449-2c7d-43d7-967a-f53cd23215ad",
+  });
+
+  auto actual = evaluate("cast(c0 as uuid) < cast(c1 as uuid)", makeRowVector({data0, data1}));


Any particular reason not to test one value at a time using evaluateOnce? This makes tests easier to read and debug.

No, just not familiar with the test environment. I will try with evaluateOnce.

BryanCutler · 2024-09-10T19:01:55Z

Thanks for reviewing @mbasmanova

I wonder where is the semantics of comparing UUIDs come from? Do you have a pointer? Would be nice to update PR description and documentation

I basically followed the comparison done in in Presto Java https://github.com/prestodb/presto/blob/97d88cfdebb553b5c7fb0e217b0211573df3d190/presto-common/src/main/java/com/facebook/presto/common/type/UuidType.java#L97

Since int128_t doesn't have support for atomic operations, we first check the higher bytes, then lower. Returning an int from the comparison allows it to be streamlined to do minimal checks. I will update the PR and documentation.

mbasmanova · 2024-09-10T19:04:08Z

@BryanCutler I wonder whether it makes sense to compare UUIDs at all? What does it mean that one UUID is greater than the other? Perhaps, this type should be non-orderable.

BryanCutler · 2024-09-10T19:39:01Z

@BryanCutler I wonder whether it makes sense to compare UUIDs at all? What does it mean that one UUID is greater than the other? Perhaps, this type should be non-orderable.

@mbasmanova I think it makes sense, just as you would compare values for std::byte, etc. Most UUID implementations support such operations, see https://www.boost.org/doc/libs/1_85_0/libs/uuid/doc/uuid.html#Operators

aditi-pandit · 2024-09-12T11:02:52Z

@BryanCutler I wonder whether it makes sense to compare UUIDs at all? What does it mean that one UUID is greater than the other? Perhaps, this type should be non-orderable.

@mbasmanova I think it makes sense, just as you would compare values for std::byte, etc. Most UUID implementations support such operations, see https://www.boost.org/doc/libs/1_85_0/libs/uuid/doc/uuid.html#Operators

@mbasmanova : I found UUID comparisons in the Java spec also https://docs.oracle.com/javase/8/docs/api/java/util/UUID.html

My understanding is that a source generating UUIDs uses a combination of timestamp with the DNS address of the generating device. So there is some notion of ordering of UUIDs as a result.

BryanCutler · 2024-09-18T20:05:39Z

@mbasmanova just wanted to confirm if you are ok with adding this functionality? I still need to update the tests per your request and will get to that soon, thanks!

mbasmanova · 2024-09-18T20:10:17Z

@BryanCutler Yes, please, move ahead with addressing the comments. Thank you for clarifying the use case.

BryanCutler · 2024-09-23T18:32:56Z

@mbasmanova @aditi-pandit after looking into prestodb/presto#23311 there are a couple issues with UUIDs in Velox:

UUID data is being stored as big endian in the int128_t value, which comes from using boost:uuid during casting to/from strings. This causes comparisons like 11000000-0000-0022-0000-000000000000 < 22000000-0000-0011-0000-000000000000 to fail.
Serialization of the int128_t value to an int128 Array Block results in [LSW, MSW], however the Presto Java UUIDType expects long values from the int128 Array Block to be [MSW, LSW].

For (1), since Velox stores UUID values with an int128_t, unlike boost::uuid which is a byte array, I think the value should be in little endian. This means instead of directly using memcpy bytes from the boost::uuid during casting, the bytes would need to swapped to get int128 in LE order. wdyt?

To fix (2), the word order would need to be switched during serialization. Would you prefer I fix that in this PR or open another?

aditi-pandit · 2024-09-25T17:14:07Z

I feel the first fix should be to address is the issue of the incorrect results in prestodb/presto#23311. Post that we can do this comparisons PR.

From your comments seems that requires 2 changes:
i) Fixing the seriailization order between the native and java sides (2) above
ii) Changing the uuid int128_t to be little endian in the copy from boost::uuid.

@mbasmanova : Using boost::uuid (instead of int128_t) to represent UUID in Velox could be cleaner given these issues. We could make UUID TypeKind::VARBINARY and use the boost::uuid bytes to represent it. wdyt ?

@BryanCutler : If we did that, would we need the serialization changes from Native to Java side as well ? My intuition is yes, but I could be wrong.

@Yuhta : Jimmy, would be great to hear your thoughts as well.

BryanCutler · 2024-09-25T18:28:01Z

Thanks @aditi-pandit , if we did change the storage from int128_t to a byte array in boost::uuid, then we would still need to adjust the serialization to Java, since Presto expects an Int128ArrayBlock.

Yuhta · 2024-10-01T21:04:47Z

@aditi-pandit I don't think VARBINARY is the right physical type for UUID, it is meant for variable length data, while UUID is fixed length. I would say just keep using int128 and fix the endianness so the comparison between UUID is the same as comparison between int128.

aditi-pandit · 2024-10-01T21:16:09Z

Thanks @Yuhta for your response. I do see a pattern of using VARBINARY for fixed lengths (in IPPREFIX type) but that has length 17 bytes which can't be captured with any integral type. For UUID we do have the option of using int128_t.

@BryanCutler : Let's go with Jimmy's suggestion. Will make our implementation simpler as well.

BryanCutler · 2024-10-07T18:57:51Z

Thanks @Yuhta and @aditi-pandit , I have fixed the serialization issue here and added a unit test, I can put that in a separate PR if preferred. I will also test these changes again with prestodb/presto#23311

BryanCutler · 2024-10-08T20:05:15Z

After testing the comparisons with Presto Java in prestodb/presto#23311 there are some different results because Presto Java uses Slice.compareTo() which compares values by swapping bytes of each long value, see https://github.com/airlift/slice/blob/0.38/src/main/java/io/airlift/slice/Slice.java#L1336. So the values are ordered according to the LSB of each long value, which seems a little strange and I'm not sure if that should be changed.

** moved the serializer fix to #11197, will update this pr soon
** updated - the underlying issue in Presto Java is an additional byte swap, not an overflow from https://bugs.openjdk.org/browse/JDK-7025832

Yuhta · 2024-10-16T17:17:37Z

velox/functions/prestosql/UuidFunctions.h

+  return x < y ? -1 : (x == y ? 0 : 1);
+}
+
+static int compare128(const int128_t& lhs, const int128_t& rhs) {


Just compare (uint128_t)lhs and (uint128_t)rhs

BryanCutler · 2024-10-16T18:06:38Z

Just compare (uint128_t)lhs and (uint128_t)rhs

Thanks @Yuhta , I can look into that but there is a bigger concern that this type of ordering will not match Presto Java, see above #10791 (comment).

Presto Java UuidOperations use Slice.compareTo() which follows a strange ordering based on swapping bytes. As far as I can tell this ordering was changed in the Slice library 2.0+, which uses Arrays.compareUnsigned and I don't believe it would have the same problem, but not sure upgrading that is planned in the near-term.

I'm not sure if we should try to follow Presto Java with the comparisons or use the spec at RFC-4122 to order lexicographically, e.g. comparing uint128_t in little endian, and differ from Presto Java. Please share recommendations on this @Yuhta @aditi-pandit

BryanCutler · 2024-10-16T18:16:37Z

There is a recent related PR cherry-picked from trindodb, this might address some part of the Presto Java issue prestodb/presto#17939.

Yuhta · 2024-10-16T18:29:45Z

@BryanCutler We should ask Presto Java community to decide the ordering of UUID and document them officially first.

BryanCutler · 2024-10-21T23:23:16Z

@BryanCutler We should ask Presto Java community to decide the ordering of UUID and document them officially first.

There is a PR up at prestodb/presto#23847 that corrects Presto Java to conform to the IETF RFC 4122 definition of ordering UUIDs. When that is resolved, this PR can be updated to match.

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 20, 2024

aditi-pandit reviewed Aug 21, 2024

View reviewed changes

BryanCutler requested a review from aditi-pandit August 23, 2024 17:15

aditi-pandit reviewed Aug 26, 2024

View reviewed changes

aditi-pandit changed the title ~~Add Support for UUID Comparison Functions~~ Add support for UUID Comparison Functions Aug 26, 2024

aditi-pandit changed the title ~~Add support for UUID Comparison Functions~~ Add support for UUID Comparison functions Aug 26, 2024

aditi-pandit approved these changes Aug 29, 2024

View reviewed changes

aditi-pandit requested review from mbasmanova, kagamiori and Yuhta August 29, 2024 09:15

mbasmanova reviewed Sep 9, 2024

View reviewed changes

mbasmanova changed the title ~~Add support for UUID Comparison functions~~ Add support for UUID comparison functions Sep 9, 2024

BryanCutler added 2 commits October 7, 2024 11:44

Add UUID comparison functions and comparison unit test

f8ee222

Fixed UUID serde to match Presto Java

5aa91d4

BryanCutler force-pushed the add-uuid-comp-functions-10584 branch from e4f984a to 5aa91d4 Compare October 7, 2024 18:44

BryanCutler requested a review from aditi-pandit October 7, 2024 19:00

Yuhta reviewed Oct 16, 2024

View reviewed changes

Add support for UUID comparison functions #10791

Are you sure you want to change the base?

Add support for UUID comparison functions #10791

Conversation

BryanCutler commented Aug 20, 2024 • edited Loading

netlify bot commented Aug 20, 2024 • edited Loading

✅ Deploy Preview for meta-velox ready!

aditi-pandit left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BryanCutler commented Aug 22, 2024

BryanCutler commented Aug 22, 2024

aditi-pandit left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BryanCutler Aug 28, 2024 • edited Loading

Choose a reason for hiding this comment

aditi-pandit Aug 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BryanCutler Aug 29, 2024 • edited Loading

Choose a reason for hiding this comment

aditi-pandit left a comment

Choose a reason for hiding this comment

aditi-pandit commented Aug 29, 2024

BryanCutler commented Sep 9, 2024

mbasmanova left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BryanCutler commented Sep 10, 2024

mbasmanova commented Sep 10, 2024

BryanCutler commented Sep 10, 2024

aditi-pandit commented Sep 12, 2024

BryanCutler commented Sep 18, 2024

mbasmanova commented Sep 18, 2024

BryanCutler commented Sep 23, 2024

aditi-pandit commented Sep 25, 2024 • edited Loading

BryanCutler commented Sep 25, 2024

Yuhta commented Oct 1, 2024

aditi-pandit commented Oct 1, 2024

BryanCutler commented Oct 7, 2024

BryanCutler commented Oct 8, 2024 • edited Loading

Choose a reason for hiding this comment

BryanCutler commented Oct 16, 2024

BryanCutler commented Oct 16, 2024

Yuhta commented Oct 16, 2024

BryanCutler commented Oct 21, 2024

BryanCutler commented Aug 20, 2024 •

edited

Loading

netlify bot commented Aug 20, 2024 •

edited

Loading

BryanCutler Aug 28, 2024 •

edited

Loading

aditi-pandit Aug 29, 2024 •

edited

Loading

BryanCutler Aug 29, 2024 •

edited

Loading

aditi-pandit commented Sep 25, 2024 •

edited

Loading

BryanCutler commented Oct 8, 2024 •

edited

Loading