Skip to content

Commit d155261

Browse files
Ashish Misrafacebook-github-bot
authored andcommitted
fix: Find VeloxType singletons for CustomType when embedded in OpaqueType objects (#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 b2125aa commit d155261

File tree

2 files changed

+24
-2
lines changed

2 files changed

+24
-2
lines changed

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(

velox/type/tests/OpaqueCustomTypesTest.cpp

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,13 @@ TEST(OpaqueCustomTypeTest, RegisterWithoutSerde) {
4343
/*deserialize=*/
4444
[](const std::string&) { return std::make_shared<TestCustomType>(); });
4545

46+
// Serialize and deserialize.
47+
auto serializedCustomType = testCustomType->serialize();
48+
EXPECT_EQ(testCustomTypeName, serializedCustomType["opaque"]);
49+
50+
auto deserializedType = Type::create(serializedCustomType);
51+
EXPECT_EQ(testCustomType, deserializedType);
52+
4653
// Trying to re-register serialization/deserialization should fail.
4754
VELOX_ASSERT_THROW(
4855
TestCustomTypeRegister::registerSerialization(
@@ -70,6 +77,13 @@ TEST(OpaqueCustomTypeTest, RegisterWithSerde) {
7077
[](const std::string&) { return std::make_shared<TestCustomType>(); });
7178
auto testCustomType = TestCustomTypeRegister::VeloxType::get();
7279

80+
// Serialize and deserialize.
81+
auto serializedCustomType = testCustomType->serialize();
82+
EXPECT_EQ(testCustomTypeName, serializedCustomType["opaque"]);
83+
84+
auto deserializedType = Type::create(serializedCustomType);
85+
EXPECT_EQ(testCustomType, deserializedType);
86+
7387
// Trying to re-register serialization/deserialization should fail since
7488
// serialization/deserialization was already registered during type
7589
// registration.

0 commit comments

Comments
 (0)