refactor: Implement one step type registration and serialization/deserialization for OpaqueCustomType #15843

AshishCbMisra · 2025-12-22T21:57:59Z

Summary:

Background.

Today, a CustomType (inner type) may be wrapped by an OpaqueType (outer type), forming a OpaqueCustomType.
When an OpaqueCustomType is defined, it needs to be registered in two place.
- Custom Type Registry - a mapping from type name to an instance of CustomTypeFactory that provides an instance of the type
- OpaqueSerdeRegistry - a mapping from type name to functions that serialize and deserialize opaque type values
Today, this requires two separate calls to the API. These separate calls are inconvenient and error prone.

Implementation

OpaqueCustomTypes.h
- This PR changes the OpaqueCustomTypeRegister::registerType() function to accept optional serialization/deserialization functions, thus eliminating the need for a separate API call.
- By keep the function arguments optional, it provides backward compatibility with existing code.

Testing Changes

OpaqueCustomTypesTest.cpp
- Implements unit-tests that ensure that serialization/deserialization functions can be provided in the registerType function optionally.
- Unit-tests ensure that serialization/deserialization functions can be provided utmost once.

Differential Revision: D89686612

netlify · 2025-12-22T21:58:04Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`1d8ef69`
🔍 Latest deploy log	https://app.netlify.com/projects/meta-velox/deploys/694b23f3613c8d00083fb60f

meta-codesync · 2025-12-22T21:58:06Z

@AshishCbMisra has exported this pull request. If you are a Meta employee, you can view the originating Diff in D89686612.

…rialization for OpaqueCustomType (facebookincubator#15843) Summary: ## Background. - Today, a CustomType (inner type) may be wrapped by an OpaqueType (outer type), forming a OpaqueCustomType. - When an OpaqueCustomType is defined, it needs to be registered in two place. - Custom Type Registry - a mapping from type name to an instance of CustomTypeFactory that provides an instance of the type - OpaqueSerdeRegistry - a mapping from type name to functions that serialize and deserialize opaque type values - Today, this requires two separate calls to the API. These separate calls are inconvenient and error prone. ## Implementation - OpaqueCustomTypes.h - This PR changes the OpaqueCustomTypeRegister::registerType() function to accept optional serialization/deserialization functions, thus eliminating the need for a separate API call. - By keep the function arguments optional, it provides backward compatibility with existing code. ## Testing Changes - OpaqueCustomTypesTest.cpp - Implements unit-tests that ensure that serialization/deserialization functions can be provided in the registerType function optionally. - Unit-tests ensure that serialization/deserialization functions can be provided utmost once. Differential Revision: D89686612

velox/type/OpaqueCustomTypes.h

velox/type/tests/OpaqueCustomTypesTest.cpp

velox/type/OpaqueCustomTypes.h

mbasmanova · 2025-12-23T21:34:40Z

velox/type/OpaqueCustomTypes.h

+  }
+  return false;
+}
+template <typename T, const char* customTypeName>


add empty line between methods

mbasmanova · 2025-12-23T21:35:41Z

velox/type/OpaqueCustomTypes.h

+      obj["name"] = "Type";
+      obj["type"] = TypeTraits<TypeKind::OPAQUE>::name;
+      obj["opaque"] = customTypeName;


Why not just

obj["name"] = "Type"; obj["type"] = customTypeName;

?

Couple of reasons:

It is the OpaqueCustom type. The representation should be as such: Opaque outer type, embedding the custom inner type.

It breaks backward compatibility. A server running one velox version would break when sending serialized type to another running a previous version.

It is the OpaqueCustom type. The representation should be as such: Opaque outer type, embedding the custom inner type.

Not really. OpaqueType here is just an implementation detail of the custom type. It doesn't / shouldn't place a restriction on how custom type chooses to serialize itself.

It breaks backward compatibility. A server running one velox version would break when sending serialized type to another running a previous version.

There is no such guarantee in Velox to begin with. Let's discuss offline if this needs to be added as it would be non-trivial undertaking.

velox/type/OpaqueCustomTypes.h

velox/type/tests/OpaqueCustomTypesTest.cpp

mbasmanova · 2025-12-23T21:38:00Z

velox/type/tests/OpaqueCustomTypesTest.cpp

+    auto deserializedType = Type::create(serializedCustomType);
+
+    // TODO: Ideally this should be EXPECT_EQ() but it doesn't work
+    // because Type::create() currently does not return the singleton instance.


This will work if you just fix 'serialize' method like I suggested.

velox/type/tests/OpaqueCustomTypesTest.cpp

…rialization for OpaqueCustomType (facebookincubator#15843) Summary: ## Background. - Today, a CustomType (inner type) may be wrapped by an OpaqueType (outer type), forming a OpaqueCustomType. - When an OpaqueCustomType is defined, it needs to be registered in two place. - Custom Type Registry - a mapping from type name to an instance of CustomTypeFactory that provides an instance of the type - OpaqueSerdeRegistry - a mapping from type name to functions that serialize and deserialize opaque type values - Today, this requires two separate calls to the API. These separate calls are inconvenient and error prone. ## Implementation - OpaqueCustomTypes.h - This PR changes the OpaqueCustomTypeRegister::registerType() function to accept optional serialization/deserialization functions, thus eliminating the need for a separate API call. - By keep the function arguments optional, it provides backward compatibility with existing code. - Implemented the VeloxType::serialize() function - Cleared SerializationRegistry of the serialization/deserialization functions provided in the registerType() function. - Types.cpp - Implemented OpaqueType::clearSerializationRegistry() method. ## Testing Changes - OpaqueCustomTypesTest.cpp - Implements unit-tests that ensure that value serialization/deserialization functions can be provided in the registerType function optionally. - Implements checking for type-serialization/deserialization. - TypesTest.cpp Differential Revision: D89686612

mbasmanova · 2025-12-24T10:14:51Z

velox/type/Type.cpp

+  auto& registry = OpaqueSerdeRegistry::get();
+  auto it = registry.mapping.find(opaqueType->typeIndex_);
+  if (it != registry.mapping.end()) {
+    registry.mapping.erase(opaqueType->typeIndex_);


perhaps, use the 'it' to erase (no need to lookup the value again)

AshishCbMisra requested a review from majetideepak as a code owner December 22, 2025 21:57

meta-cla 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 Dec 22, 2025

meta-codesync bot added fb-exported meta-exported labels Dec 22, 2025

AshishCbMisra mentioned this pull request Dec 22, 2025

Extend OpaqueCustomTypeRegister to include calls to OpaqueType::registerSerialization #15836

Open

AshishCbMisra force-pushed the export-D89686612 branch from c60f365 to aeb814f Compare December 22, 2025 22:02

AshishCbMisra force-pushed the export-D89686612 branch from aeb814f to 1011baf Compare December 22, 2025 23:03

AshishCbMisra force-pushed the export-D89686612 branch 2 times, most recently from a0cacd4 to 14f2c87 Compare December 22, 2025 23:05

mbasmanova requested a review from juwentus1234 December 23, 2025 00:28