fix: Fix Presto Java UUID serialization #11197

BryanCutler · 2024-10-08T20:42:50Z

This fixes the PrestoSerializer to put UUID values in the correct format that is expected by Presto Java so that the values will match those from a Java worker. First, when converting UUID to/from string, the values are no longer in big endian format (as taken from boost::uuid) and are instead stored as a little endian in an int128_t. Secondly, Presto Java will read UUID values from an Int128ArrayBlock with the first value as the most significant bits. To correct this, the upper/lower parts of the int128_t are swapped during serialization/deserialization.

A unit test for checking roundtrip UUID serializaiton was added and manual testing of Presto with a native worker to verify the problem from the issue description is fixed.

From prestodb/presto#23311

netlify · 2024-10-08T20:43:08Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`bf000b0`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/673d76747b8ff600082c98c2

BryanCutler · 2024-10-08T20:44:29Z

please review @aditi-pandit @Yuhta @mbasmanova

velox/functions/prestosql/tests/UuidFunctionsTest.cpp

velox/functions/prestosql/types/UuidType.cpp

BryanCutler · 2024-10-09T22:33:10Z

Thanks @Yuhta , but folly::Endian::big is not defined for int128 types. It's based off of compiler built-in bytes swapping functions, so it looks like it could be added for GCC variants, but there is no built-in function for MSVC. I could add a check to these changes to only swap if the host system is LE, wdyt?

Yuhta · 2024-10-16T17:40:10Z

@BryanCutler You can add a utility to DecimalUtil to do that. We can do it with 2 folly::Endian::bigs if needed.

BryanCutler · 2024-11-04T19:04:05Z

Status: I've added DecimalUtil::big() to swap bytes for int128_t, but there is an active PR at prestodb/presto#23847 which changes UUID format on the Presto Java side. There would need to be an additional adjustment to the changes here to compensate if the Java UUID PR is merged.

BryanCutler · 2024-11-06T22:30:54Z

I have updated to work with the latest changes from prestodb/presto#23847. The serialization makes sure Presto Java receives values according to the format described in prestodb/presto#23961 (comment)

I have manually tested these changes with Presto Java following the error description from prestodb/presto#23311 and confirmed the values are now displayed correctly.

@Yuhta please review again, thanks.

aditi-pandit

Thanks @BryanCutler

Would be great to enhance the VectorFuzzer (https://github.com/facebookincubator/velox/blob/main/velox/vector/fuzzer/VectorFuzzer.h) to build vectors of random UUID values and use it in the fuzzers to incorporate this type and functions in different queries

https://facebookincubator.github.io/velox/develop/testing.html

Some of these fuzzers have also been enhanced to do validation of queries with Presto. That would be a good exercise to ensure the serialization works e2e.

Please consider adding that support as well.

@kagamiori @kgpai

Wei, Krishna : To give some background, we found these correctness issues and gaps when running some e2e queries using UUID type prestodb/presto#23311. Would be great to enhance the fuzzers to catch the issues with this type.

aditi-pandit · 2024-11-08T06:08:57Z

velox/serializers/tests/PrestoSerializerTest.cpp

+  for (int row = 0; row < uuidValues.size(); row++) {
+    uuidValues[row] = (int128_t) 0xD1 << row % 120;
+  }
+  auto vector = makeFlatVector<int128_t>(uuidValues, UUID());


Use variation https://github.com/facebookincubator/velox/blob/main/velox/vector/tests/utils/VectorTestBase.h#L183 of makeFlatVector to avoid the for loops above.

velox/type/DecimalUtil.cpp

aditi-pandit · 2024-11-08T06:15:10Z

velox/functions/prestosql/types/UuidType.cpp

@@ -127,6 +128,9 @@ class UuidCastOperator : public exec::CastOperator {
      int128_t u;
      memcpy(&u, &uuid, 16);

+      // Value is big endian from boost, store as native byte-order


Nit : Comments should be complete sentences and end in a full-stop.

aditi-pandit · 2024-11-08T06:15:25Z

velox/functions/prestosql/types/UuidType.cpp

@@ -75,7 +75,8 @@ class UuidCastOperator : public exec::CastOperator {
    const auto* uuids = input.as<SimpleVector<int128_t>>();

    context.applyToSelectedNoThrow(rows, [&](auto row) {
-      const auto uuid = uuids->valueAt(row);
+      // Make sure UUID bytes are big endian when building string


Nit : Comments should be complete sentences and end in a full-stop.

aditi-pandit · 2024-11-08T06:16:18Z

velox/serializers/PrestoSerializer.cpp

@@ -443,6 +445,42 @@ void readDecimalValues(
  }
 }

+int128_t readUuidValue(ByteInputStream* source) {
+  // ByteInputStream does not support reading int128_t values.
+  // UUIDs are serialized as 2 int64 values with msb int64 value first.


Nit : uint64 (not int64)

velox/serializers/PrestoSerializer.cpp

BryanCutler · 2024-11-13T01:24:26Z

Thanks for reviewing @aditi-pandit . What do you think about me working on additional fuzzer tests as a followup if the rest of this PR is ready? I did add a serialization round trip test that goes through a full range of values, but I do think improving the fuzzer to use UUIDs sounds like a good addition.

aditi-pandit · 2024-11-13T07:00:38Z

Thanks for reviewing @aditi-pandit . What do you think about me working on additional fuzzer tests as a followup if the rest of this PR is ready? I did add a serialization round trip test that goes through a full range of values, but I do think improving the fuzzer to use UUIDs sounds like a good addition.

Thats fine @BryanCutler. Thanks !

aditi-pandit

Thanks @BryanCutler

BryanCutler · 2024-11-13T19:53:14Z

@Yuhta please review, thanks

velox/type/DecimalUtil.h

BryanCutler · 2024-11-19T17:46:39Z

@Yuhta please review, thanks!

velox/serializers/PrestoSerializer.cpp

BryanCutler · 2024-11-19T22:16:42Z

I missed a format fix, could you try again @Yuhta ?

facebook-github-bot · 2024-11-19T23:48:43Z

@kagamiori has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

kagamiori · 2024-11-20T05:08:19Z

velox/serializers/tests/PrestoSerializerTest.cpp

@@ -16,6 +16,7 @@
 #include "velox/serializers/PrestoSerializer.h"
 #include <boost/random/uniform_int_distribution.hpp>
 #include <folly/Random.h>
+#include <functions/prestosql/types/UuidType.h>


Hi @BryanCutler
nit: Is this header not needed? If so, could you remove it? Thanks!

Thanks, good catch. No it's not needed anymore, I've removed it.

Summary: This fixes the PrestoSerializer to put UUID values in the correct format that is expected by Presto Java so that the values will match those from a Java worker. First, when converting UUID to/from string, the values are no longer in big endian format (as taken from boost::uuid) and are instead stored as a little endian in an int128_t. Secondly, Presto Java will read UUID values from an Int128ArrayBlock with the first value as the most significant bits. To correct this, the upper/lower parts of the int128_t are swapped during serialization/deserialization. A unit test for checking roundtrip UUID serializaiton was added and manual testing of Presto with a native worker to verify the problem from the issue description is fixed. From prestodb/presto#23311

facebook-github-bot · 2024-11-20T18:46:47Z

@kagamiori has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2024-11-21T01:05:47Z

@kagamiori merged this pull request in fe4f5a7.

conbench-facebook · 2024-11-21T01:57:05Z

Conbench analyzed the 1 benchmark run on commit fe4f5a77.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

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 Oct 8, 2024

BryanCutler mentioned this pull request Oct 8, 2024

feat: Add support for UUID comparison functions #10791

Open

Yuhta reviewed Oct 8, 2024

View reviewed changes

velox/functions/prestosql/tests/UuidFunctionsTest.cpp Outdated Show resolved Hide resolved

velox/functions/prestosql/types/UuidType.cpp Outdated Show resolved Hide resolved

velox/functions/prestosql/types/UuidType.cpp Outdated Show resolved Hide resolved

BryanCutler force-pushed the fix-uuid-java-serde-23311 branch 2 times, most recently from beb2740 to d608cfe Compare October 24, 2024 01:27

BryanCutler force-pushed the fix-uuid-java-serde-23311 branch 2 times, most recently from adb9e4a to 2ff3b4f Compare November 1, 2024 19:05

BryanCutler mentioned this pull request Nov 4, 2024

Fix UUID comparisons to conform to IETF RFC 4122 prestodb/presto#23847

Merged

6 tasks

BryanCutler force-pushed the fix-uuid-java-serde-23311 branch from 2ff3b4f to 65e09f1 Compare November 6, 2024 22:25

BryanCutler requested a review from Yuhta November 6, 2024 22:31

aditi-pandit reviewed Nov 8, 2024

View reviewed changes

BryanCutler force-pushed the fix-uuid-java-serde-23311 branch from 65e09f1 to fad021d Compare November 13, 2024 01:21

aditi-pandit approved these changes Nov 13, 2024

View reviewed changes

BryanCutler force-pushed the fix-uuid-java-serde-23311 branch 2 times, most recently from a51aec9 to da0e6cb Compare November 13, 2024 20:03

Yuhta reviewed Nov 13, 2024

View reviewed changes

velox/type/DecimalUtil.h Outdated Show resolved Hide resolved

BryanCutler force-pushed the fix-uuid-java-serde-23311 branch from da0e6cb to d2ebf7f Compare November 14, 2024 05:55

Yuhta reviewed Nov 14, 2024

View reviewed changes

velox/type/DecimalUtil.h Outdated Show resolved Hide resolved

BryanCutler force-pushed the fix-uuid-java-serde-23311 branch from d2ebf7f to 958efe6 Compare November 14, 2024 18:17

BryanCutler requested review from Yuhta and aditi-pandit November 15, 2024 21:35

aditi-pandit approved these changes Nov 19, 2024

View reviewed changes

Yuhta reviewed Nov 19, 2024

View reviewed changes

velox/serializers/PrestoSerializer.cpp Outdated Show resolved Hide resolved

BryanCutler force-pushed the fix-uuid-java-serde-23311 branch 2 times, most recently from d23db6f to 3c8c85d Compare November 19, 2024 20:28

Yuhta approved these changes Nov 19, 2024

View reviewed changes

Yuhta added the ready-to-merge PR that have been reviewed and are ready for merging. PRs with this tag notify the Velox Meta oncall label Nov 19, 2024

BryanCutler force-pushed the fix-uuid-java-serde-23311 branch from 3c8c85d to 7b7123b Compare November 19, 2024 21:47

kagamiori reviewed Nov 20, 2024

View reviewed changes

BryanCutler force-pushed the fix-uuid-java-serde-23311 branch from 7b7123b to bf000b0 Compare November 20, 2024 05:41

kagamiori changed the title ~~Fix Presto Java UUID serialization~~ fix: Fix Presto Java UUID serialization Nov 20, 2024

facebook-github-bot closed this in fe4f5a7 Nov 21, 2024

facebook-github-bot added the Merged label Nov 21, 2024

BryanCutler deleted the fix-uuid-java-serde-23311 branch November 21, 2024 23:57

This was referenced Nov 22, 2024

[Native] Add UUID to fuzzer for e2e tests prestodb/presto#24114

Closed

Add UUID type to fuzzer for e2e tests #11622

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Fix Presto Java UUID serialization #11197

fix: Fix Presto Java UUID serialization #11197

BryanCutler commented Oct 8, 2024

netlify bot commented Oct 8, 2024 •

edited

Loading

BryanCutler commented Oct 8, 2024

BryanCutler commented Oct 9, 2024

Yuhta commented Oct 16, 2024 •

edited

Loading

BryanCutler commented Nov 4, 2024

BryanCutler commented Nov 6, 2024

aditi-pandit left a comment

aditi-pandit Nov 8, 2024

aditi-pandit Nov 8, 2024

aditi-pandit Nov 8, 2024

aditi-pandit Nov 8, 2024

BryanCutler commented Nov 13, 2024

aditi-pandit commented Nov 13, 2024

aditi-pandit left a comment

BryanCutler commented Nov 13, 2024

BryanCutler commented Nov 19, 2024

BryanCutler commented Nov 19, 2024

facebook-github-bot commented Nov 19, 2024

kagamiori Nov 20, 2024 •

edited

Loading

BryanCutler Nov 20, 2024

facebook-github-bot commented Nov 20, 2024

facebook-github-bot commented Nov 21, 2024

conbench-facebook bot commented Nov 21, 2024

fix: Fix Presto Java UUID serialization #11197

fix: Fix Presto Java UUID serialization #11197

Conversation

BryanCutler commented Oct 8, 2024

netlify bot commented Oct 8, 2024 • edited Loading

✅ Deploy Preview for meta-velox canceled.

BryanCutler commented Oct 8, 2024

BryanCutler commented Oct 9, 2024

Yuhta commented Oct 16, 2024 • edited Loading

BryanCutler commented Nov 4, 2024

BryanCutler commented Nov 6, 2024

aditi-pandit left a comment

Choose a reason for hiding this comment

aditi-pandit Nov 8, 2024

Choose a reason for hiding this comment

aditi-pandit Nov 8, 2024

Choose a reason for hiding this comment

aditi-pandit Nov 8, 2024

Choose a reason for hiding this comment

aditi-pandit Nov 8, 2024

Choose a reason for hiding this comment

BryanCutler commented Nov 13, 2024

aditi-pandit commented Nov 13, 2024

aditi-pandit left a comment

Choose a reason for hiding this comment

BryanCutler commented Nov 13, 2024

BryanCutler commented Nov 19, 2024

BryanCutler commented Nov 19, 2024

facebook-github-bot commented Nov 19, 2024

kagamiori Nov 20, 2024 • edited Loading

Choose a reason for hiding this comment

BryanCutler Nov 20, 2024

Choose a reason for hiding this comment

facebook-github-bot commented Nov 20, 2024

facebook-github-bot commented Nov 21, 2024

conbench-facebook bot commented Nov 21, 2024

netlify bot commented Oct 8, 2024 •

edited

Loading

Yuhta commented Oct 16, 2024 •

edited

Loading

kagamiori Nov 20, 2024 •

edited

Loading