refactor

rui-mo · Apr 10, 2024 · 89e649d · 89e649d
1 parent 89de5d3
commit 89e649d
Show file tree

Hide file tree

Showing 5 changed files with 80 additions and 53 deletions.
diff --git 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
@@ -1140,18 +1140,32 @@ bool hasType(const std::string& name) {
   return false;
 }
 
-TypePtr getType(
+Status getType(
     const std::string& name,
-    const std::vector<TypeParameter>& parameters) {
+    const std::vector<TypeParameter>& 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
@@ -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<const Type> 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<TypeParameter>& parameters);
+    const std::vector<TypeParameter>& parameters,
+    TypePtr& type);
 
 template <TypeKind KIND>
 std::shared_ptr<const Type> createType(

diff --git 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
@@ -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);
 }