add opaque type support to hive type serde #11253

kunigami · 2024-10-14T22:14:26Z

Summary:
My understanding of opaque types in Velox is that Velox doesn't know about the underlying type of it, and treats them as a shared_ptr<void>. For serializing data across processes, we need to somewhat break that assumption, because when we need to know how to deserialize this opaque data.

One option is to have the underlying type as part of the serialized type signature, the other is to store this information with the serialized data itself. I'm adopting the first option here.

We also need to introduce a layer of abstraction for opaque type index, by allowing aliasing opaque types.

The reason we can't use opaque type index is the assumption that they're not stable across processes. So if you serialize a opaque type as string in process A and then deserialize in process B, even if running the same revision there's no guarantee the type ID is the same.

With this change, callers are required to register an alias for opaque types before serializing/deserializing it via HiveTypeSerializer and HiveTypeParser. I put this registry in Type.h but if we want to keep this specific to HiveTypeSerializer/HiveTypeParser we could move it elsewhere.

Differential Revision: D64358220

facebook-github-bot · 2024-10-14T22:14:34Z

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

netlify · 2024-10-14T22:14:43Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`7adb14c`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/6722b7129bdd330008f4de8d

Summary: My understanding of opaque types in Velox is that Velox doesn't know about the underlying type of it, and treats them as a `shared_ptr<void>`. For serializing data across processes, we need to somewhat break that assumption, because when we need to know how to deserialize this opaque data. One option is to have the underlying type as part of the serialized type signature, the other is to store this information with the serialized data itself. I'm adopting the first option here. We also need to introduce a layer of abstraction for opaque type index, by allowing aliasing opaque types. The reason we can't use opaque type index is the assumption that they're not stable across processes. So if you serialize a opaque type as string in process A and then deserialize in process B, even if running the same revision there's no guarantee the type ID is the same. With this change, callers are required to register an alias for opaque types before serializing/deserializing it via `HiveTypeSerializer` and `HiveTypeParser`. I put this registry in `Type.h` but if we want to keep this specific to `HiveTypeSerializer/HiveTypeParser` we could move it elsewhere. Differential Revision: D64358220

pedroerp

Really cool! Made a few comments, but could you also split the type and fuzzer stuff since they are quite independent?

Thanks!

pedroerp · 2024-10-15T13:06:37Z

velox/type/Type.cpp

+  auto it = getTypeIndexByOpaqueAlias().find(name);
+  VELOX_CHECK(
+      it != getTypeIndexByOpaqueAlias().end(),
+      "Could not find type {}. Did you call registerOpaqueType?",


nit: please surround the type name by quotes '{}'

pedroerp · 2024-10-15T13:08:16Z

velox/type/Type.h

@@ -2008,6 +2008,22 @@ bool registerCustomType(
    const std::string& name,
    std::unique_ptr<const CustomTypeFactories> factories);

+std::unordered_map<std::string, std::type_index>& getTypeIndexByOpaqueAlias();


can you add a bit more documentation here about what this does, when it should be used, and some of the restrictions/considerations you added to the PR summary?

pedroerp · 2024-10-15T13:10:19Z

velox/type/fbhive/HiveTypeParser.cpp

-  if (nt.isValidType() && nt.isPrimitiveType()) {
+
+  if (!nt.isValidType()) {
+    VELOX_FAIL(fmt::format(


I know you're only copying it, but you can omit the fmt::format as Velox exception macros can do the formatting automatically for you.

pedroerp · 2024-10-15T13:11:24Z

velox/type/fbhive/HiveTypeParser.cpp

@@ -118,7 +128,16 @@ Result HiveTypeParser::parseType() {
      eatToken(TokenType::RightRoundBracket);
    }
    return Result{scalarType};
-  } else if (nt.isValidType()) {
+  } else if (nt.metadata->tokenString[0] == "opaque") {


should we have a nt.isOpaqueType()?

pedroerp · 2024-10-15T13:24:12Z

velox/type/fbhive/tests/HiveTypeParserTests.cpp

+  registerOpaqueType<Foo>("bar");
+  HiveTypeParser parser;
+  auto t = parser.parse("opaque<bar>");
+  ASSERT_EQ(t->toString(), "OPAQUE<facebook::velox::type::fbhive::Foo>");


very cool :)

pedroerp · 2024-10-15T13:24:45Z

velox/type/fbhive/tests/HiveTypeParserTests.cpp

+  HiveTypeParser parser;
+  auto t = parser.parse("opaque<bar>");
+  ASSERT_EQ(t->toString(), "OPAQUE<facebook::velox::type::fbhive::Foo>");
+}


should we also test an invalid opaque type deserialization?

is it on purpose that we still don't have deserialization tests?

not sure I understand, what deserialization tests? we have parseOpaque()

pedroerp · 2024-10-15T13:25:35Z

velox/type/fbhive/tests/HiveTypeSerializerTests.cpp

+  // Use a custom name to highlight this is just an alias.
+  registerOpaqueType<Foo>("bar");
+
+  std::shared_ptr<const velox::Type> type = velox::OPAQUE<Foo>();


you can omit the velox:: namespace prefix throughout. You can also use the TypePtr alias or just auto

pedroerp · 2024-10-15T13:26:45Z

velox/type/fbhive/tests/HiveTypeSerializerTests.cpp

+  EXPECT_EQ(result, "opaque<bar>");
+}
+
+TEST(HiveTypeSerializer, unsupported) {


can we also test some unregistered type inside the opaque?

pedroerp · 2024-10-15T13:29:13Z

velox/vector/fuzzer/VectorFuzzer.cpp

@@ -558,6 +558,8 @@ VectorPtr VectorFuzzer::fuzzFlat(const TypePtr& type, vector_size_t size) {
    }

    return fuzzRow(std::move(childrenVectors), rowType.names(), size);
+  } else if (type->isOpaque()) {


Cc: @kagamiori @bikramSingh91 I remember some discussion about adding opaque support in Fuzzer; here it is :)

@kunigami could you just split the fuzzer support stuff to a different PR?

I actually had a PR for adding it to fuzzer but I missed some codepaths that this test triggered. I'll move it to
#11189

nvm, I just messed the export of the diff stack.

pedroerp · 2024-10-15T13:30:34Z

velox/vector/fuzzer/VectorFuzzer.cpp

+    const TypePtr& type,
+    vector_size_t size) {
+  auto vector = BaseVector::create(type, size, pool_);
+  using TFlat = typename KindToFlatVector<TypeKind::OPAQUE>::type;


nit: maybe TFlatOpaque ?

Summary: My understanding of opaque types in Velox is that Velox doesn't know about the underlying type of it, and treats them as a `shared_ptr<void>`. For serializing data across processes, we need to somewhat break that assumption, because when we need to know how to deserialize this opaque data. One option is to have the underlying type as part of the serialized type signature, the other is to store this information with the serialized data itself. I'm adopting the first option here. We also need to introduce a layer of abstraction for opaque type index, by allowing aliasing opaque types. The reason we can't use opaque type index is the assumption that they're not stable across processes. So if you serialize a opaque type as string in process A and then deserialize in process B, even if running the same revision there's no guarantee the type ID is the same. With this change, callers are required to register an alias for opaque types before serializing/deserializing it via `HiveTypeSerializer` and `HiveTypeParser`. I put this registry in `Type.h` but if we want to keep this specific to `HiveTypeSerializer/HiveTypeParser` we could move it elsewhere. Differential Revision: D64358220

facebook-github-bot · 2024-10-29T00:55:58Z

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

Summary: My understanding of opaque types in Velox is that Velox doesn't know about the underlying type of it, and treats them as a `shared_ptr<void>`. For serializing data across processes, we need to somewhat break that assumption, because when we need to know how to deserialize this opaque data. One option is to have the underlying type as part of the serialized type signature, the other is to store this information with the serialized data itself. I'm adopting the first option here. We also need to introduce a layer of abstraction for opaque type index, by allowing aliasing opaque types. The reason we can't use opaque type index is the assumption that they're not stable across processes. So if you serialize a opaque type as string in process A and then deserialize in process B, even if running the same revision there's no guarantee the type ID is the same. With this change, callers are required to register an alias for opaque types before serializing/deserializing it via `HiveTypeSerializer` and `HiveTypeParser`. I put this registry in `Type.h` but if we want to keep this specific to `HiveTypeSerializer/HiveTypeParser` we could move it elsewhere. Differential Revision: D64358220

facebook-github-bot · 2024-10-29T00:59:54Z

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

Summary: My understanding of opaque types in Velox is that Velox doesn't know about the underlying type of it, and treats them as a `shared_ptr<void>`. For serializing data across processes, we need to somewhat break that assumption, because when we need to know how to deserialize this opaque data. One option is to have the underlying type as part of the serialized type signature, the other is to store this information with the serialized data itself. I'm adopting the first option here. We also need to introduce a layer of abstraction for opaque type index, by allowing aliasing opaque types. The reason we can't use opaque type index is the assumption that they're not stable across processes. So if you serialize a opaque type as string in process A and then deserialize in process B, even if running the same revision there's no guarantee the type ID is the same. With this change, callers are required to register an alias for opaque types before serializing/deserializing it via `HiveTypeSerializer` and `HiveTypeParser`. I put this registry in `Type.h` but if we want to keep this specific to `HiveTypeSerializer/HiveTypeParser` we could move it elsewhere. Differential Revision: D64358220

pedroerp

Thanks @kunigami , looks pretty good!

pedroerp · 2024-10-30T19:15:15Z

velox/type/Type.h

+
+std::string getOpaqueAliasForTypeId(std::type_index typeIndex);
+
+// OpaqueType represents a type that is transparent to the Velox type system.


nit: I think the point is that opaque types are NOT transparent to the type system. :) (they are "opaque" to the type system) :)

pedroerp · 2024-10-30T19:16:04Z

velox/type/Type.h

+// So if we were to serialize an opaque type using its std::type_index, we
+// might not be able to deserialize it in another process. To solve this
+// problem, we require that both the serializing and deserializing processes
+// register the opaque type using registerOpaqueType() with the same alias.


thanks for the comment. We usually use "///" for documentation header comments such as this.

pedroerp · 2024-10-30T19:18:13Z

velox/type/fbhive/HiveTypeParser.cpp

@@ -140,11 +159,6 @@ Result HiveTypeParser::parseType() {
      default:


could we use toString(nt.typeKind()) instead?

pedroerp · 2024-10-30T19:20:42Z

velox/type/fbhive/tests/HiveTypeParserTests.cpp

+  HiveTypeParser parser;
+  auto t = parser.parse("opaque<bar>");
+  ASSERT_EQ(t->toString(), "OPAQUE<facebook::velox::type::fbhive::Foo>");
+}


is it on purpose that we still don't have deserialization tests?

pedroerp · 2024-10-30T19:25:39Z

(please rebase on main first) @kunigami

Summary: My understanding of opaque types in Velox is that Velox doesn't know about the underlying type of it, and treats them as a `shared_ptr<void>`. For serializing data across processes, we need to somewhat break that assumption, because when we need to know how to deserialize this opaque data. One option is to have the underlying type as part of the serialized type signature, the other is to store this information with the serialized data itself. I'm adopting the first option here. We also need to introduce a layer of abstraction for opaque type index, by allowing aliasing opaque types. The reason we can't use opaque type index is the assumption that they're not stable across processes. So if you serialize a opaque type as string in process A and then deserialize in process B, even if running the same revision there's no guarantee the type ID is the same. With this change, callers are required to register an alias for opaque types before serializing/deserializing it via `HiveTypeSerializer` and `HiveTypeParser`. I put this registry in `Type.h` but if we want to keep this specific to `HiveTypeSerializer/HiveTypeParser` we could move it elsewhere. Reviewed By: pedroerp Differential Revision: D64358220

facebook-github-bot · 2024-10-30T21:55:45Z

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

facebook-github-bot · 2024-10-30T21:55:52Z

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

kunigami · 2024-10-30T21:56:03Z

rebased

Summary: My understanding of opaque types in Velox is that Velox doesn't know about the underlying type of it, and treats them as a `shared_ptr<void>`. For serializing data across processes, we need to somewhat break that assumption, because when we need to know how to deserialize this opaque data. One option is to have the underlying type as part of the serialized type signature, the other is to store this information with the serialized data itself. I'm adopting the first option here. We also need to introduce a layer of abstraction for opaque type index, by allowing aliasing opaque types. The reason we can't use opaque type index is the assumption that they're not stable across processes. So if you serialize a opaque type as string in process A and then deserialize in process B, even if running the same revision there's no guarantee the type ID is the same. With this change, callers are required to register an alias for opaque types before serializing/deserializing it via `HiveTypeSerializer` and `HiveTypeParser`. I put this registry in `Type.h` but if we want to keep this specific to `HiveTypeSerializer/HiveTypeParser` we could move it elsewhere. Reviewed By: pedroerp Differential Revision: D64358220

facebook-github-bot · 2024-10-30T22:45:53Z

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

facebook-github-bot · 2024-10-31T00:29:51Z

This pull request has been merged in 5cbba09.

conbench-facebook · 2024-10-31T01:22:06Z

Conbench analyzed the 1 benchmark run on commit 5cbba097.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

Summary: Pull Request resolved: facebookincubator#11253 My understanding of opaque types in Velox is that Velox doesn't know about the underlying type of it, and treats them as a `shared_ptr<void>`. For serializing data across processes, we need to somewhat break that assumption, because when we need to know how to deserialize this opaque data. One option is to have the underlying type as part of the serialized type signature, the other is to store this information with the serialized data itself. I'm adopting the first option here. We also need to introduce a layer of abstraction for opaque type index, by allowing aliasing opaque types. The reason we can't use opaque type index is the assumption that they're not stable across processes. So if you serialize a opaque type as string in process A and then deserialize in process B, even if running the same revision there's no guarantee the type ID is the same. With this change, callers are required to register an alias for opaque types before serializing/deserializing it via `HiveTypeSerializer` and `HiveTypeParser`. I put this registry in `Type.h` but if we want to keep this specific to `HiveTypeSerializer/HiveTypeParser` we could move it elsewhere. Reviewed By: pedroerp Differential Revision: D64358220 fbshipit-source-id: fb702e4366592c2f0e84c8ea11b7a2a9f5176854

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

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

pedroerp reviewed Oct 15, 2024

View reviewed changes

kunigami force-pushed the export-D64358220 branch from 58dfb67 to e037138 Compare October 29, 2024 00:55

kunigami force-pushed the export-D64358220 branch from e037138 to 709fd5c Compare October 29, 2024 00:59

pedroerp approved these changes Oct 30, 2024

View reviewed changes

pedroerp 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 Oct 30, 2024

kunigami force-pushed the export-D64358220 branch from 709fd5c to 4787813 Compare October 30, 2024 21:55

kunigami force-pushed the export-D64358220 branch from 4787813 to 8d0819c Compare October 30, 2024 21:55

kunigami force-pushed the export-D64358220 branch from 8d0819c to 7adb14c Compare October 30, 2024 22:45

facebook-github-bot closed this in 5cbba09 Oct 31, 2024

facebook-github-bot added the Merged label Oct 31, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add opaque type support to hive type serde #11253

add opaque type support to hive type serde #11253

kunigami commented Oct 14, 2024

facebook-github-bot commented Oct 14, 2024

netlify bot commented Oct 14, 2024 •

edited

Loading

pedroerp left a comment

pedroerp Oct 15, 2024

pedroerp Oct 15, 2024

pedroerp Oct 15, 2024

pedroerp Oct 15, 2024

pedroerp Oct 15, 2024

pedroerp Oct 15, 2024

pedroerp Oct 30, 2024

kunigami Oct 30, 2024 •

edited

Loading

pedroerp Oct 15, 2024

pedroerp Oct 15, 2024

pedroerp Oct 15, 2024

kunigami Oct 15, 2024

kunigami Oct 15, 2024

pedroerp Oct 15, 2024

facebook-github-bot commented Oct 29, 2024

facebook-github-bot commented Oct 29, 2024

pedroerp left a comment

pedroerp Oct 30, 2024

pedroerp Oct 30, 2024

pedroerp Oct 30, 2024

pedroerp Oct 30, 2024

pedroerp commented Oct 30, 2024

facebook-github-bot commented Oct 30, 2024

facebook-github-bot commented Oct 30, 2024

kunigami commented Oct 30, 2024

facebook-github-bot commented Oct 30, 2024

facebook-github-bot commented Oct 31, 2024

conbench-facebook bot commented Oct 31, 2024


		std::string getOpaqueAliasForTypeId(std::type_index typeIndex);

		// OpaqueType represents a type that is transparent to the Velox type system.

		@@ -140,11 +159,6 @@ Result HiveTypeParser::parseType() {
		default:

add opaque type support to hive type serde #11253

add opaque type support to hive type serde #11253

Conversation

kunigami commented Oct 14, 2024

facebook-github-bot commented Oct 14, 2024

netlify bot commented Oct 14, 2024 • edited Loading

✅ Deploy Preview for meta-velox canceled.

pedroerp 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kunigami Oct 30, 2024 • edited Loading

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

Choose a reason for hiding this comment

facebook-github-bot commented Oct 29, 2024

facebook-github-bot commented Oct 29, 2024

pedroerp 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

pedroerp commented Oct 30, 2024

facebook-github-bot commented Oct 30, 2024

facebook-github-bot commented Oct 30, 2024

kunigami commented Oct 30, 2024

facebook-github-bot commented Oct 30, 2024

facebook-github-bot commented Oct 31, 2024

conbench-facebook bot commented Oct 31, 2024

netlify bot commented Oct 14, 2024 •

edited

Loading

kunigami Oct 30, 2024 •

edited

Loading