Skip to content

Commit 24a3f15

Browse files
Ashish Misrafacebook-github-bot
authored andcommitted
fix: Find VeloxType singletons for CustomType when embedded in OpaqueType objects (facebookincubator#15829)
Summary: ## Background - This diff is to fix a bug in the Types::create() method for CustomType. - The bug manifests in the SignatureBinder.bind() operation failing to match for a CustomType argument. - The CustomType is basically an OpaqueType object. - The root cause is that two different registries store OpaqueType objects. - Custom Type Registry → VeloxType singleton (correct name()) - OpaqueSerdeRegistry → Base OpaqueType (name() = "OPAQUE") - When deserializing the custom type in Type::create() method, the code does not find VeloxType singleton, hence created an OpaqueType. - The CustomType returns its name in its custom name in the name() and toString() methods, whereas the OpaqueType returns "OPAQUE<demangled-name>" in its name/toString() methods. ## In this Diff - The primary problem is that the CustomType has a Velox singleton for its type. However, type is serialized and deserialized, the type produced is an Opaque type, with custom type inside. - In this diff, the velox deserialization correctly finds and produces the VeloxType singleton. ## Implementation Changes - Types.cpp - Implemented searching and deserializing CustomType within OpaqueTypes in Type::create() method. ## Testing Changes - OpaqueCustomTypesTest.cpp - Added test to serialize/deserialize a CustomType and confirm that VeloxType singleton is returned. Differential Revision: D89564769
1 parent d6b6de2 commit 24a3f15

File tree

2 files changed

+36
-2
lines changed

2 files changed

+36
-2
lines changed

velox/expression/tests/CustomTypeTest.cpp

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
#include "velox/functions/prestosql/types/SfmSketchRegistration.h"
2424
#include "velox/functions/prestosql/types/TimeWithTimezoneType.h"
2525
#include "velox/type/OpaqueCustomTypes.h"
26+
#include "velox/type/Type.h"
2627

2728
namespace facebook::velox::test {
2829

@@ -462,4 +463,29 @@ TEST_F(CustomTypeTest, timeWithTimezoneTypeTest) {
462463
assertEqualVectors(expected, result);
463464
}
464465
}
466+
467+
TEST_F(CustomTypeTest, SerdeTest) {
468+
// This is the C++ type registered within the opaque capsule. This could be
469+
// anything.
470+
struct TestCustomType {};
471+
// Custom type name.
472+
static constexpr char testCustomTypeName[] = "my_test_custom_type";
473+
using TestCustomTypeRegister =
474+
OpaqueCustomTypeRegister<TestCustomType, testCustomTypeName>;
475+
476+
// Register type and build a TypePtr.
477+
TestCustomTypeRegister::registerType();
478+
OpaqueType::registerSerialization<TestCustomType>(
479+
testCustomTypeName,
480+
[](const std::shared_ptr<TestCustomType>&) { return ""; },
481+
[](const std::string&) { return std::make_shared<TestCustomType>(); });
482+
auto testCustomType = TestCustomTypeRegister::VeloxType::get();
483+
484+
// Serialize and deserialize.
485+
auto serializedCustomType = testCustomType->serialize();
486+
EXPECT_EQ(serializedCustomType["opaque"], testCustomTypeName);
487+
488+
auto deserializedType = Type::create(serializedCustomType);
489+
EXPECT_EQ(testCustomType, deserializedType);
490+
}
465491
} // namespace facebook::velox::test

velox/type/Type.cpp

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -203,8 +203,8 @@ TypePtr Type::create(const folly::dynamic& obj) {
203203
if (isDecimalName(typeName)) {
204204
return DECIMAL(obj["precision"].asInt(), obj["scale"].asInt());
205205
}
206-
// Checks if 'typeName' specifies a custom type.
207-
if (customTypeExists(typeName)) {
206+
207+
auto customTypeSingleton = [&](const std::string& typeName) {
208208
std::vector<TypeParameter> params;
209209
if (obj.find("cTypes") != obj.items().end()) {
210210
params.reserve(childTypes.size());
@@ -220,6 +220,11 @@ TypePtr Type::create(const folly::dynamic& obj) {
220220
deserializeEnumParam<std::string>(obj["kVarcharEnumParam"]));
221221
}
222222
return getCustomType(typeName, params);
223+
};
224+
225+
// Checks if 'typeName' specifies a custom type.
226+
if (customTypeExists(typeName)) {
227+
return customTypeSingleton(typeName);
223228
}
224229

225230
// 'typeName' must be a built-in type.
@@ -237,6 +242,9 @@ TypePtr Type::create(const folly::dynamic& obj) {
237242

238243
case TypeKind::OPAQUE: {
239244
const auto& persistentName = obj["opaque"].asString();
245+
if (customTypeExists(persistentName)) {
246+
return customTypeSingleton(persistentName);
247+
}
240248
const auto& registry = OpaqueSerdeRegistry::get();
241249
auto it = registry.reverse.find(persistentName);
242250
VELOX_USER_CHECK(

0 commit comments

Comments
 (0)