Skip to content

Commit 208aa00

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 ec91776 commit 208aa00

File tree

2 files changed

+35
-2
lines changed

2 files changed

+35
-2
lines changed

velox/expression/tests/CustomTypeTest.cpp

Lines changed: 25 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,28 @@ 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+
static constexpr char testCustomTypeName[] = "my_test_custom_type";
472+
using TestCustomTypeRegister =
473+
OpaqueCustomTypeRegister<TestCustomType, testCustomTypeName>;
474+
475+
// Register type and build a TypePtr.
476+
TestCustomTypeRegister::registerType(
477+
/*serialize=*/[](const std::shared_ptr<TestCustomType>&) { return ""; },
478+
/*deserialize=*/
479+
[](const std::string&) { return std::make_shared<TestCustomType>(); });
480+
481+
auto testCustomType = TestCustomTypeRegister::VeloxType::get();
482+
483+
// Serialize and deserialize.
484+
auto serializedCustomType = testCustomType->serialize();
485+
EXPECT_EQ(testCustomTypeName, serializedCustomType["opaque"]);
486+
487+
auto deserializedType = Type::create(serializedCustomType);
488+
EXPECT_EQ(testCustomType, deserializedType);
489+
}
465490
} // 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)