feat: Add opaque type support to PrestoSerializer #11256

kunigami · 2024-10-15T01:49:40Z

Summary:
The idea of opaque types in Presto serializer is that we'll serialize the underlying opaque type to StringView. This requires a serde callbacks to be registered via

OpaqueType::registerSerialization();

then inside PrestoSerializer.cpp we use these to write and read values of type opaque.

Differential Revision: D64362525

facebook-github-bot · 2024-10-15T01:49:52Z

This pull request was exported from Phabricator. Differential Revision: D64362525

netlify · 2024-10-15T01:49:59Z

✅ Deploy Preview for meta-velox canceled.

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

Summary: The idea of opaque types in Presto serializer is that we'll serialize the underlying opaque type to StringView. This requires a serde callbacks to be registered via OpaqueType::registerSerialization(); then inside `PrestoSerializer.cpp` we use these to write and read values of type opaque. Differential Revision: D64362525

facebook-github-bot · 2024-10-29T03:57:48Z

This pull request was exported from Phabricator. Differential Revision: D64362525

Summary: The idea of opaque types in Presto serializer is that we'll serialize the underlying opaque type to StringView. This requires a serde callbacks to be registered via OpaqueType::registerSerialization(); then inside `PrestoSerializer.cpp` we use these to write and read values of type opaque. Differential Revision: D64362525

facebook-github-bot · 2024-10-31T23:48:57Z

This pull request was exported from Phabricator. Differential Revision: D64362525

pedroerp · 2024-11-07T20:45:52Z

velox/serializers/tests/PrestoSerializerTest.cpp

+
+  static std::shared_ptr<Foo> create(int64_t id) {
+    // Return the same instance if the id is already in the map, to make
+    // the operator== work with the instance before and after serde.


not sure I followed. Why do you need to compare that they are the same instances, and not just different instances which are equal?

I think it's because the comparison of shared_ptr will compare the raw pointer address, so it needs to be the same instance. we could change the test utilities to de-reference before comparisons if desired.

bikramSingh91 · 2024-11-09T00:10:25Z

velox/serializers/PrestoSerializer.cpp

+        continue;
+      }
+      stream->appendNonNull();
+      std::shared_ptr<void> ptr = rawValues[offset];


nit: unused variable

bikramSingh91 · 2024-11-09T00:24:10Z

velox/serializers/PrestoSerializer.cpp

@@ -1935,6 +2001,39 @@ void serializeFlatVector<TypeKind::BOOLEAN>(
  }
 }

+template <>
+void serializeFlatVector<TypeKind::OPAQUE>(


This implementation is used by the PrestoBatchVectorSerializer only. You also need to add an implementation for PrestoIterativeVectorSerializer. I am not sure which one you use for your use-case. To give you more context the PrestoBatchVectorSerializer is optimized for serializing full vectors whereas PrestoIterativeVectorSerializer is used for serializing subsets of rows scattered within a single vector(which is usually used for a partitioning exchange).

the serializeFlatVector implementation for PrestoIterativeVectorSerializer is here:

velox/velox/serializers/PrestoSerializer.cpp

Line 2737 in a33e8d7

void serializeFlatVector<TypeKind::OPAQUE>(

Finally, for a complete implementation, you also need to implement estimateSerializedSize for both types of serializers. This is used by the PartitionedOutput node to figure out when it needs to flush data to the wire.

bikramSingh91 · 2024-11-09T00:39:50Z

velox/serializers/PrestoSerializer.cpp

+  }
+
+  const int64_t rawStringSizeBytes = dataSize * sizeof(char);
+  char* rawString = (char*)pool->allocate(rawStringSizeBytes);


nit: instead of directly using the pool's allocate API that returns a raw pointer, I would suggest create a buffer and using a BufferPtr instead since its much more safer as the space will be freed when BufferPtr is destroyed, otherwise if any of the steps throw after this then it can introduce a memory leak.

Its like using a shared_ptr vs a raw ptr and has similar benefits.

You can use:

BufferPtr newBuffer = AlignedBuffer::allocate<char>(dataSize, pool());

bikramSingh91 · 2024-11-09T00:58:35Z

velox/serializers/tests/PrestoSerializerTest.cpp

@@ -1555,6 +1589,7 @@ TEST_F(PrestoSerializerTest, deserializeSingleColumn) {
      DOUBLE(),
      VARCHAR(),
      TIMESTAMP(),
+      OPAQUE<Foo>(),


This is just adding a type to deserializeSingleColumn which is not used for serialization across the wire (See method comment of deserializeSingleColumn for more info). Therefore, you would also want to add test coverage for the serialize (both batch and iterative) and deserialize methods.

Also would have to add tests for estimateSerializedSize when you implement it

Summary: The idea of opaque types in Presto serializer is that we'll serialize the underlying opaque type to StringView. This requires a serde callbacks to be registered via OpaqueType::registerSerialization(); then inside `PrestoSerializer.cpp` we use these to write and read values of type opaque. Differential Revision: D64362525

facebook-github-bot · 2024-11-12T02:58:06Z

This pull request was exported from Phabricator. Differential Revision: D64362525

Summary: The idea of opaque types in Presto serializer is that we'll serialize the underlying opaque type to StringView. This requires a serde callbacks to be registered via OpaqueType::registerSerialization(); then inside `PrestoSerializer.cpp` we use these to write and read values of type opaque. Differential Revision: D64362525

facebook-github-bot · 2024-11-12T18:24:23Z

This pull request was exported from Phabricator. Differential Revision: D64362525

kunigami · 2024-11-12T18:31:38Z

addressing comments

use AlignedBuffer::allocate()
discussed w/ bikramSingh91 that we don't need PrestoIterativeVectorSerializer and estimateSerializedSize
add test case for opaque type serialization/deserialization

bikramSingh91

Looks good. Just a few nits. The build is failing, can you please take a look into it

bikramSingh91 · 2024-11-12T20:27:13Z

velox/serializers/PrestoSerializer.cpp

+    const SerdeOpts&,
+    VectorPtr& result) {
+  // Opaque values are serialized by first converting them to string
+  // them serializing them as if they were string. The deserializable


nit: "then" serializing

bikramSingh91 · 2024-11-12T20:38:04Z

velox/serializers/tests/PrestoSerializerTest.cpp

+  auto inputVector = makeFlatVector<std::shared_ptr<void>>(
+      2,
+      [](vector_size_t row) { return Foo::create(row + 10); },
+      /*isNullAt=*/nullptr,


can you also add a test with nulls for completeness

bikramSingh91 · 2024-11-12T20:39:51Z

velox/serializers/tests/PrestoSerializerTest.cpp

@@ -1339,6 +1373,25 @@ TEST_P(PrestoSerializerTest, encodedRoundtrip) {
  }
 }

+TEST_P(PrestoSerializerTest, opaqueBatchVectorSerializer) {


can you also add a test to ensure that opaque type will throw an error if used with Iterative serializer and if estimateSerializedSize is used

Summary: The idea of opaque types in Presto serializer is that we'll serialize the underlying opaque type to StringView. This requires a serde callbacks to be registered via OpaqueType::registerSerialization(); then inside `PrestoSerializer.cpp` we use these to write and read values of type opaque. Differential Revision: D64362525

facebook-github-bot · 2024-11-12T21:19:44Z

This pull request was exported from Phabricator. Differential Revision: D64362525

kunigami · 2024-11-12T23:45:37Z

address comments. use find() instead of contains() so it doesn't rely on newer c++ features

Summary: The idea of opaque types in Presto serializer is that we'll serialize the underlying opaque type to StringView. This requires a serde callbacks to be registered via OpaqueType::registerSerialization(); then inside `PrestoSerializer.cpp` we use these to write and read values of type opaque. Differential Revision: D64362525

facebook-github-bot · 2024-11-13T01:48:28Z

This pull request was exported from Phabricator. Differential Revision: D64362525

Summary: The idea of opaque types in Presto serializer is that we'll serialize the underlying opaque type to StringView. This requires a serde callbacks to be registered via OpaqueType::registerSerialization(); then inside `PrestoSerializer.cpp` we use these to write and read values of type opaque. Reviewed By: bikramSingh91 Differential Revision: D64362525

facebook-github-bot · 2024-11-18T20:33:15Z

This pull request was exported from Phabricator. Differential Revision: D64362525

…11256) Summary: The idea of opaque types in Presto serializer is that we'll serialize the underlying opaque type to StringView. This requires a serde callbacks to be registered via OpaqueType::registerSerialization(); then inside `PrestoSerializer.cpp` we use these to write and read values of type opaque. Reviewed By: bikramSingh91 Differential Revision: D64362525

facebook-github-bot · 2024-11-18T21:11:42Z

This pull request was exported from Phabricator. Differential Revision: D64362525

…11256) Summary: The idea of opaque types in Presto serializer is that we'll serialize the underlying opaque type to StringView. This requires a serde callbacks to be registered via OpaqueType::registerSerialization(); then inside `PrestoSerializer.cpp` we use these to write and read values of type opaque. Reviewed By: bikramSingh91 Differential Revision: D64362525

facebook-github-bot · 2024-11-18T22:58:34Z

This pull request was exported from Phabricator. Differential Revision: D64362525

…11256) Summary: The idea of opaque types in Presto serializer is that we'll serialize the underlying opaque type to StringView. This requires a serde callbacks to be registered via OpaqueType::registerSerialization(); then inside `PrestoSerializer.cpp` we use these to write and read values of type opaque. Reviewed By: bikramSingh91 Differential Revision: D64362525

facebook-github-bot · 2024-11-18T23:06:09Z

This pull request was exported from Phabricator. Differential Revision: D64362525

…or#11256) Summary: The idea of opaque types in Presto serializer is that we'll serialize the underlying opaque type to StringView. This requires a serde callbacks to be registered via OpaqueType::registerSerialization(); then inside `PrestoSerializer.cpp` we use these to write and read values of type opaque. Reviewed By: bikramSingh91 Differential Revision: D64362525

facebook-github-bot · 2024-11-19T01:04:57Z

This pull request was exported from Phabricator. Differential Revision: D64362525

…or#11256) Summary: The idea of opaque types in Presto serializer is that we'll serialize the underlying opaque type to StringView. This requires a serde callbacks to be registered via OpaqueType::registerSerialization(); then inside `PrestoSerializer.cpp` we use these to write and read values of type opaque. Reviewed By: bikramSingh91 Differential Revision: D64362525

facebook-github-bot · 2024-11-19T01:30:30Z

This pull request was exported from Phabricator. Differential Revision: D64362525

…11256) Summary: The idea of opaque types in Presto serializer is that we'll serialize the underlying opaque type to StringView. This requires a serde callbacks to be registered via OpaqueType::registerSerialization(); then inside `PrestoSerializer.cpp` we use these to write and read values of type opaque. Reviewed By: bikramSingh91 Differential Revision: D64362525

facebook-github-bot · 2024-11-19T01:34:22Z

This pull request was exported from Phabricator. Differential Revision: D64362525

facebook-github-bot · 2024-11-19T04:12:19Z

This pull request has been merged in ce67924.

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 15, 2024

facebook-github-bot added the fb-exported label Oct 15, 2024

kunigami force-pushed the export-D64362525 branch from f058cb1 to 152650b Compare October 29, 2024 03:57

kunigami force-pushed the export-D64362525 branch from 152650b to 59b424d Compare October 31, 2024 23:48

pedroerp reviewed Nov 7, 2024

View reviewed changes

bikramSingh91 reviewed Nov 9, 2024

View reviewed changes

kunigami force-pushed the export-D64362525 branch from 59b424d to 8f9036a Compare November 12, 2024 02:57

kunigami force-pushed the export-D64362525 branch from 8f9036a to e21d43b Compare November 12, 2024 18:23

bikramSingh91 reviewed Nov 12, 2024

View reviewed changes

kunigami force-pushed the export-D64362525 branch from e21d43b to a64a8f7 Compare November 12, 2024 21:19

kunigami force-pushed the export-D64362525 branch from a64a8f7 to 32aa0da Compare November 13, 2024 01:48

bikramSingh91 approved these changes Nov 18, 2024

View reviewed changes

kunigami force-pushed the export-D64362525 branch from 32aa0da to 352d45e Compare November 18, 2024 20:32

kunigami force-pushed the export-D64362525 branch from 352d45e to cbc819c Compare November 18, 2024 21:10

kunigami changed the title ~~add opaque type support to PrestoSerializer~~ feat: add opaque type support to PrestoSerializer Nov 18, 2024

kunigami force-pushed the export-D64362525 branch from cbc819c to 9fae51d Compare November 18, 2024 22:58

kunigami force-pushed the export-D64362525 branch from 9fae51d to 955e1e9 Compare November 18, 2024 23:05

kunigami changed the title ~~feat: add opaque type support to PrestoSerializer~~ feature: add opaque type support to PrestoSerializer Nov 19, 2024

kunigami force-pushed the export-D64362525 branch from 955e1e9 to d5f0ad7 Compare November 19, 2024 01:04

kunigami force-pushed the export-D64362525 branch from d5f0ad7 to da43424 Compare November 19, 2024 01:28

kunigami changed the title ~~feature: add opaque type support to PrestoSerializer~~ feat: Add opaque type support to PrestoSerializer Nov 19, 2024

kunigami force-pushed the export-D64362525 branch from da43424 to a857cfb Compare November 19, 2024 01:32

facebook-github-bot closed this in ce67924 Nov 19, 2024

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add opaque type support to PrestoSerializer #11256

feat: Add opaque type support to PrestoSerializer #11256

kunigami commented Oct 15, 2024

facebook-github-bot commented Oct 15, 2024

netlify bot commented Oct 15, 2024 •

edited

Loading

facebook-github-bot commented Oct 29, 2024

facebook-github-bot commented Oct 31, 2024

pedroerp Nov 7, 2024

kunigami Nov 8, 2024

bikramSingh91 Nov 9, 2024

bikramSingh91 Nov 9, 2024

bikramSingh91 Nov 9, 2024

bikramSingh91 Nov 9, 2024

facebook-github-bot commented Nov 12, 2024

facebook-github-bot commented Nov 12, 2024

kunigami commented Nov 12, 2024

bikramSingh91 left a comment

bikramSingh91 Nov 12, 2024

bikramSingh91 Nov 12, 2024

bikramSingh91 Nov 12, 2024

facebook-github-bot commented Nov 12, 2024

kunigami commented Nov 12, 2024

facebook-github-bot commented Nov 13, 2024

facebook-github-bot commented Nov 18, 2024

facebook-github-bot commented Nov 18, 2024

facebook-github-bot commented Nov 18, 2024

facebook-github-bot commented Nov 18, 2024

facebook-github-bot commented Nov 19, 2024

facebook-github-bot commented Nov 19, 2024

facebook-github-bot commented Nov 19, 2024

facebook-github-bot commented Nov 19, 2024

feat: Add opaque type support to PrestoSerializer #11256

feat: Add opaque type support to PrestoSerializer #11256

Conversation

kunigami commented Oct 15, 2024

facebook-github-bot commented Oct 15, 2024

netlify bot commented Oct 15, 2024 • edited Loading

✅ Deploy Preview for meta-velox canceled.

facebook-github-bot commented Oct 29, 2024

facebook-github-bot commented Oct 31, 2024

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

Choose a reason for hiding this comment

facebook-github-bot commented Nov 12, 2024

facebook-github-bot commented Nov 12, 2024

kunigami commented Nov 12, 2024

bikramSingh91 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

facebook-github-bot commented Nov 12, 2024

kunigami commented Nov 12, 2024

facebook-github-bot commented Nov 13, 2024

facebook-github-bot commented Nov 18, 2024

facebook-github-bot commented Nov 18, 2024

facebook-github-bot commented Nov 18, 2024

facebook-github-bot commented Nov 18, 2024

facebook-github-bot commented Nov 19, 2024

facebook-github-bot commented Nov 19, 2024

facebook-github-bot commented Nov 19, 2024

facebook-github-bot commented Nov 19, 2024

netlify bot commented Oct 15, 2024 •

edited

Loading