diff --git a/velox/expression/SignatureBinder.cpp b/velox/expression/SignatureBinder.cpp index 4ea670d4d1951..1c8bd43a9d291 100644 --- a/velox/expression/SignatureBinder.cpp +++ b/velox/expression/SignatureBinder.cpp @@ -270,12 +270,12 @@ TypePtr SignatureBinder::tryResolveType( typeParameters.push_back(TypeParameter(type)); } - try { - if (auto type = getType(typeName, typeParameters)) { - return type; - } - } catch (const std::exception&) { - // TODO Perhaps, modify getType to add suppress-errors flag. + TypePtr type; + auto status = getType(typeName, typeParameters, type); + if (type) { + return type; + } + if (!status.ok()) { return nullptr; } diff --git a/velox/type/Type.cpp b/velox/type/Type.cpp index 1d2ba1cb3ccca..4d335669e2e8d 100644 --- a/velox/type/Type.cpp +++ b/velox/type/Type.cpp @@ -1140,18 +1140,32 @@ bool hasType(const std::string& name) { return false; } -TypePtr getType( +Status getType( const std::string& name, - const std::vector& parameters) { + const std::vector& parameters, + TypePtr& type) { if (singletonBuiltInTypes().count(name)) { - return singletonBuiltInTypes().at(name); + type = singletonBuiltInTypes().at(name); + VELOX_DCHECK_NOT_NULL(type); + return Status::OK(); } if (parametricBuiltinTypes().count(name)) { - return parametricBuiltinTypes().at(name)(parameters); + try { + type = parametricBuiltinTypes().at(name)(parameters); + } catch (const std::exception& e) { + type = nullptr; + return Status::UserError(e.what()); + } + VELOX_DCHECK_NOT_NULL(type); + return Status::OK(); } - return getCustomType(name); + type = getCustomType(name); + if (type == nullptr) { + return Status::Invalid("Type {} not found." + name); + } + return Status::OK(); } } // namespace facebook::velox diff --git a/velox/type/Type.h b/velox/type/Type.h index 2753dcb5af527..522e0a5801fc0 100644 --- a/velox/type/Type.h +++ b/velox/type/Type.h @@ -37,6 +37,7 @@ #include "velox/type/StringView.h" #include "velox/type/Timestamp.h" #include "velox/type/Tree.h" +#include "velox/common/base/Status.h" namespace facebook::velox { @@ -1681,11 +1682,12 @@ std::shared_ptr createType( /// Returns true built-in or custom type with specified name exists. bool hasType(const std::string& name); -/// Returns built-in or custom type with specified name and child types. -/// Returns nullptr if type with specified name doesn't exist. -TypePtr getType( +/// Sets the type as built-in or custom type with specified name and child +/// types. Returns error status if type with specified name doesn't exist. +Status getType( const std::string& name, - const std::vector& parameters); + const std::vector& parameters, + TypePtr& type); template std::shared_ptr createType( diff --git a/velox/type/parser/ParserUtil.cpp b/velox/type/parser/ParserUtil.cpp index b7fa36ec27b9b..e2a95e6bc24f0 100644 --- a/velox/type/parser/ParserUtil.cpp +++ b/velox/type/parser/ParserUtil.cpp @@ -29,10 +29,14 @@ TypePtr typeFromString( } else if (upper == "DOUBLE PRECISION") { upper = "DOUBLE"; } - auto inferredType = getType(upper, {}); + TypePtr inferredType; + const auto status = getType(upper, {}, inferredType); if (failIfNotRegistered) { VELOX_CHECK( - inferredType, "Failed to parse type [{}]. Type not registered.", type); + inferredType && status.ok(), + "Failed to parse type [{}] due to {}. Type not registered.", + type, + status.message()); } return inferredType; } diff --git a/velox/type/tests/TypeTest.cpp b/velox/type/tests/TypeTest.cpp index f377ba3e5a72c..986d299dc4d1a 100644 --- a/velox/type/tests/TypeTest.cpp +++ b/velox/type/tests/TypeTest.cpp @@ -71,8 +71,9 @@ TEST(TypeTest, array) { EXPECT_TRUE(arrayType->parameters()[0].kind == TypeParameterKind::kType); EXPECT_EQ(*arrayType->parameters()[0].type, *arrayType->childAt(0)); - EXPECT_EQ( - *arrayType, *getType("ARRAY", {TypeParameter(ARRAY(ARRAY(INTEGER())))})); + TypePtr type; + getType("ARRAY", {TypeParameter(ARRAY(ARRAY(INTEGER())))}, type); + EXPECT_EQ(*arrayType, *type); testTypeSerde(arrayType); } @@ -90,7 +91,9 @@ TEST(TypeTest, integer) { } TEST(TypeTest, hugeint) { - EXPECT_EQ(getType("HUGEINT", {}), HUGEINT()); + TypePtr type; + getType("HUGEINT", {}, type); + EXPECT_EQ(type, HUGEINT()); } TEST(TypeTest, timestamp) { @@ -240,14 +243,15 @@ TEST(TypeTest, shortDecimal) { shortDecimal->parameters()[1].kind == TypeParameterKind::kLongLiteral); EXPECT_EQ(shortDecimal->parameters()[1].longLiteral.value(), 5); - EXPECT_EQ( - *shortDecimal, - *getType( - "DECIMAL", - { - TypeParameter(10), - TypeParameter(5), - })); + TypePtr type; + getType( + "DECIMAL", + { + TypeParameter(10), + TypeParameter(5), + }, + type); + EXPECT_EQ(*shortDecimal, *type); testTypeSerde(shortDecimal); } @@ -276,14 +280,15 @@ TEST(TypeTest, longDecimal) { longDecimal->parameters()[1].kind == TypeParameterKind::kLongLiteral); EXPECT_EQ(longDecimal->parameters()[1].longLiteral.value(), 5); - EXPECT_EQ( - *longDecimal, - *getType( - "DECIMAL", - { - TypeParameter(30), - TypeParameter(5), - })); + TypePtr type; + getType( + "DECIMAL", + { + TypeParameter(30), + TypeParameter(5), + }, + type); + EXPECT_EQ(*longDecimal, *type); testTypeSerde(longDecimal); } @@ -375,14 +380,15 @@ TEST(TypeTest, map) { EXPECT_EQ(*mapType->parameters()[i].type, *mapType->childAt(i)); } - EXPECT_EQ( - *mapType, - *getType( - "MAP", - { - TypeParameter(INTEGER()), - TypeParameter(ARRAY(BIGINT())), - })); + TypePtr type; + getType( + "MAP", + { + TypeParameter(INTEGER()), + TypeParameter(ARRAY(BIGINT())), + }, + type); + EXPECT_EQ(*mapType, *type); testTypeSerde(mapType); } @@ -756,15 +762,16 @@ TEST(TypeTest, function) { EXPECT_EQ(*type->parameters()[i].type, *type->childAt(i)); } - EXPECT_EQ( - *type, - *getType( - "FUNCTION", - { - TypeParameter(BIGINT()), - TypeParameter(VARCHAR()), - TypeParameter(BOOLEAN()), - })); + TypePtr inferredType; + getType( + "FUNCTION", + { + TypeParameter(BIGINT()), + TypeParameter(VARCHAR()), + TypeParameter(BOOLEAN()), + }, + inferredType); + EXPECT_EQ(*type, *inferredType); testTypeSerde(type); }