address all the comments

facebookincubator · Oct 12, 2024 · f980aba · f980aba
1 parent b19d7fc
commit f980aba
Show file tree

Hide file tree

Showing 3 changed files with 57 additions and 48 deletions.
diff --git a/velox/core/SimpleFunctionMetadata.h b/velox/core/SimpleFunctionMetadata.h
@@ -394,7 +394,9 @@ class ISimpleFunctionMetadata {
   virtual bool physicalSignatureEquals(
       const ISimpleFunctionMetadata& other) const = 0;
   virtual std::string helpMessage(const std::string& name) const = 0;
-  virtual std::string toString(const std::string& name) const = 0;
+  /// Returns string of all the fields such as logical signature,
+  /// physical signature and priority.
+  virtual std::string toDebugString() const = 0;
 };
 
 template <typename T, typename = int32_t>
@@ -537,40 +539,19 @@ class SimpleFunctionMetadata : public ISimpleFunctionMetadata {
     return s;
   }
 
-  std::string toString(const std::string& name) const final {
-    std::stringstream sig;
-    bool first = true;
-    for (auto& arg : signature_->argumentTypes()) {
-      if (!first) {
-        sig << ", ";
-      }
-      first = false;
-      sig << boost::algorithm::to_upper_copy(arg.toString());
-    }
+  std::string toDebugString() const final {
+    auto logicalArguments =
+        argumentToString<exec::TypeSignature>(signature_->argumentTypes());
 
-    if (isVariadic()) {
-      sig << "...";
-    }
+    auto physicalArguments = argumentToString<TypePtr>(argPhysicalTypes_);
 
-    std::stringstream ss;
-    first = true;
-    for (const auto& arg : argPhysicalTypes_) {
-      if (!first) {
-        ss << ", ";
-      }
-      first = false;
-      ss << arg->toString();
-    }
-    if (isVariadic()) {
-      ss << "...";
-    }
     return fmt::format(
-        "FunctionName: {}\nSignature argument types:\n{}\nPhysical argument types:\n{}"
-        "\nPhysical result types:\n{}\nPriority:{}\nDefaultNullBehavior:{}",
-        name,
-        sig.str(),
-        ss.str(),
-        resultPhysicalType_,
+        "Logical signature: ({}) -> {}\nPhysical signature: ({}) -> {}\n"
+        "Priority: {}\nDefaultNullBehavior: {}",
+        logicalArguments,
+        signature_->returnType().toString(),
+        physicalArguments,
+        resultPhysicalType_->toString(),
         priority_,
         defaultNullBehavior_);
   }
@@ -648,6 +629,27 @@ class SimpleFunctionMetadata : public ISimpleFunctionMetadata {
     signature_ = builder.build();
   }
 
+  template <typename T>
+  static std::string argumentToString(const std::vector<T>& arguments) {
+    std::stringstream ss;
+    bool first = true;
+    for (const auto& arg : arguments) {
+      if (!first) {
+        ss << ", ";
+      }
+      first = false;
+      if constexpr (std::is_same_v<T, exec::TypeSignature>) {
+        ss << arg.toString();
+      } else {
+        ss << arg->toString();
+      }
+    }
+    if (isVariadic()) {
+      ss << "...";
+    }
+    return ss.str();
+  }
+
   const bool defaultNullBehavior_;
   exec::FunctionSignaturePtr signature_;
   uint32_t priority_;
@@ -878,9 +880,9 @@ class UDFHolder {
   // null, without calling the function implementation.
   static constexpr bool is_default_null_behavior = !udf_has_callNullable;
 
-  // If any of the the provided "call" flavors can produce null (in case any of
-  // them return bool). This is only false if all the call methods provided for
-  // a function return void.
+  // If any of the the provided "call" flavors can produce null (in case any
+  // of them return bool). This is only false if all the call methods provided
+  // for a function return void.
   static constexpr bool can_produce_null_output = udf_has_call_return_bool |
       udf_has_callNullable_return_bool | udf_has_callNullFree_return_bool |
       udf_has_callAscii_return_bool;

diff --git a/velox/expression/SimpleFunctionRegistry.h b/velox/expression/SimpleFunctionRegistry.h
@@ -132,8 +132,8 @@ class SimpleFunctionRegistry {
       return functionEntry_.getMetadata().helpMessage(name);
     }
 
-    std::string toString(const std::string& name) const {
-      return functionEntry_.getMetadata().toString(name);
+    std::string toDebugString() const {
+      return functionEntry_.getMetadata().toDebugString();
     }
 
     VectorFunctionMetadata metadata() const {

diff --git a/velox/expression/tests/SimpleFunctionTest.cpp b/velox/expression/tests/SimpleFunctionTest.cpp
@@ -1631,21 +1631,28 @@ struct DecimalPlusValueFunction {
   int8_t scale_;
 };
 
-TEST_F(SimpleFunctionTest, toString) {
-  const std::string functionName = "decimal_plus_value";
+TEST_F(SimpleFunctionTest, toDebugString) {
+  const std::string functionName = "decimal_plus_value_debug_string";
   registerFunction<
       DecimalPlusValueFunction,
+      LongDecimal<P2, S1>,
       LongDecimal<P1, S1>,
-      LongDecimal<P1, S1>,
-      int64_t>({functionName});
-  auto function = exec::simpleFunctions().resolveFunction(
-      functionName, {DECIMAL(20, 2), BIGINT()});
-  EXPECT_TRUE(function.has_value());
+      int32_t>(
+      {functionName},
+      {exec::SignatureVariable(
+          P2::name(),
+          fmt::format("{a_precision} + 1", fmt::arg("a_precision", P1::name())),
+          exec::ParameterType::kIntegerParameter)});
+
+  auto resolved = exec::simpleFunctions().resolveFunction(
+      functionName, {DECIMAL(20, 2), INTEGER()});
+
+  ASSERT_TRUE(resolved.has_value());
   EXPECT_EQ(
-      function.value().toString(functionName),
-      "FunctionName: decimal_plus_value\nSignature argument types:\nDECIMAL(I1,I5), BIGINT\n"
-      "Physical argument types:\nHUGEINT, BIGINT\nPhysical result types:\nHUGEINT\n"
-      "Priority:999997\nDefaultNullBehavior:true");
+      resolved.value().toDebugString(),
+      "Logical signature: (decimal(i1,i5), integer) -> decimal(i2,i5)\n"
+      "Physical signature: (HUGEINT, INTEGER) -> HUGEINT\n"
+      "Priority: 999997\nDefaultNullBehavior: true");
 }
 
 } // namespace