Update variableArity function signature (facebookincubator#10719)

Summary: Pull Request resolved: facebookincubator#10719 This simplifies the call and makes it more readable. The number of methods should now be equal to the number of argument types. Differential Revision: D61144588
DanielHunte · Aug 26, 2024 · e101f91 · e101f91
1 parent d33cdb2
commit e101f91
Show file tree

Hide file tree

Showing 12 changed files with 64 additions and 52 deletions.
diff --git a/velox/exec/tests/FunctionSignatureBuilderTest.cpp b/velox/exec/tests/FunctionSignatureBuilderTest.cpp
@@ -177,8 +177,7 @@ TEST_F(FunctionSignatureBuilderTest, scalarConstantFlags) {
                          .argumentType("double")
                          .constantArgumentType("T")
                          .argumentType("bigint")
-                         .constantArgumentType("boolean")
-                         .variableArity()
+                         .constantVariableArity("boolean")
                          .build();
     EXPECT_FALSE(signature->constantArguments().at(0));
     EXPECT_TRUE(signature->constantArguments().at(1));
@@ -214,8 +213,7 @@ TEST_F(FunctionSignatureBuilderTest, aggregateConstantFlags) {
                             .argumentType("bigint")
                             .constantArgumentType("T")
                             .argumentType("T")
-                            .constantArgumentType("double")
-                            .variableArity()
+                            .constantVariableArity("double")
                             .build();
     EXPECT_FALSE(aggSignature->constantArguments().at(0));
     EXPECT_TRUE(aggSignature->constantArguments().at(1));

diff --git a/velox/expression/FunctionSignature.h b/velox/expression/FunctionSignature.h
@@ -314,6 +314,24 @@ class FunctionSignatureBuilder {
     return *this;
   }
 
+  /// Variable arity arguments can appear only at the end of the argument list
+  /// and can appear zero or more times.
+  FunctionSignatureBuilder& variableArity(const std::string& type) {
+    argumentTypes_.emplace_back(parseTypeSignature(type));
+    constantArguments_.push_back(false);
+    variableArity_ = true;
+    return *this;
+  }
+
+  /// Variable arity arguments can appear only at the end of the argument list
+  /// and can appear zero or more times.
+  FunctionSignatureBuilder& constantVariableArity(const std::string& type) {
+    argumentTypes_.emplace_back(parseTypeSignature(type));
+    constantArguments_.push_back(true);
+    variableArity_ = true;
+    return *this;
+  }
+
   FunctionSignaturePtr build();
 
  private:
@@ -391,6 +409,25 @@ class AggregateFunctionSignatureBuilder {
     return *this;
   }
 
+  /// Variable arity arguments can appear only at the end of the argument list
+  /// and can appear zero or more times.
+  AggregateFunctionSignatureBuilder& variableArity(const std::string& type) {
+    argumentTypes_.emplace_back(parseTypeSignature(type));
+    constantArguments_.push_back(false);
+    variableArity_ = true;
+    return *this;
+  }
+
+  /// Variable arity arguments can appear only at the end of the argument list
+  /// and can appear zero or more times.
+  AggregateFunctionSignatureBuilder& constantVariableArity(
+      const std::string& type) {
+    argumentTypes_.emplace_back(parseTypeSignature(type));
+    constantArguments_.push_back(true);
+    variableArity_ = true;
+    return *this;
+  }
+
   std::shared_ptr<AggregateFunctionSignature> build();
 
  private:

diff --git a/velox/expression/fuzzer/ExpressionFuzzer.cpp b/velox/expression/fuzzer/ExpressionFuzzer.cpp
@@ -185,8 +185,7 @@ static const std::unordered_map<
              // boolean, boolean,.. -> boolean
              facebook::velox::exec::FunctionSignatureBuilder()
                  .argumentType("boolean")
-                 .argumentType("boolean")
-                 .variableArity()
+                 .variableArity("boolean")
                  .returnType("boolean")
                  .build()}},
         {"or",
@@ -195,8 +194,7 @@ static const std::unordered_map<
              // boolean, boolean,.. -> boolean
              facebook::velox::exec::FunctionSignatureBuilder()
                  .argumentType("boolean")
-                 .argumentType("boolean")
-                 .variableArity()
+                 .variableArity("boolean")
                  .returnType("boolean")
                  .build()}},
         {"coalesce",
@@ -206,8 +204,7 @@ static const std::unordered_map<
              facebook::velox::exec::FunctionSignatureBuilder()
                  .typeVariable("T")
                  .argumentType("T")
-                 .argumentType("T")
-                 .variableArity()
+                 .variableArity("T")
                  .returnType("T")
                  .build()}},
         {

diff --git a/velox/expression/fuzzer/tests/ArgumentTypeFuzzerTest.cpp b/velox/expression/fuzzer/tests/ArgumentTypeFuzzerTest.cpp
@@ -182,8 +182,7 @@ TEST_F(ArgumentTypeFuzzerTest, variableArity) {
                          .typeVariable("X")
                          .returnType("X")
                          .argumentType("X")
-                         .argumentType("bigint")
-                         .variableArity()
+                         .variableArity("bigint")
                          .build();
 
     testFuzzingSuccess(signature, VARCHAR(), {VARCHAR(), BIGINT()});
@@ -193,8 +192,7 @@ TEST_F(ArgumentTypeFuzzerTest, variableArity) {
     auto signature = exec::FunctionSignatureBuilder()
                          .knownTypeVariable("K")
                          .returnType("bigint")
-                         .argumentType("K")
-                         .variableArity()
+                         .variableArity("K")
                          .build();
     std::mt19937 seed{0};
     ArgumentTypeFuzzer fuzzer{*signature, BIGINT(), seed};
@@ -434,8 +432,7 @@ TEST_F(ArgumentTypeFuzzerTest, fuzzDecimalArgumentTypes) {
                   .integerVariable("s")
                   .returnType("decimal(p,s)")
                   .argumentType("decimal(p,s)")
-                  .argumentType("decimal(p,s)")
-                  .variableArity()
+                  .variableArity("decimal(p,s)")
                   .build();
   argTypes = fuzzArgumentTypes(*signature, DECIMAL(10, 7));
   ASSERT_LE(1, argTypes.size());
@@ -582,8 +579,7 @@ TEST_F(ArgumentTypeFuzzerTest, fuzzDecimalReturnType) {
                   .integerVariable("s")
                   .returnType("decimal(p,s)")
                   .argumentType("decimal(p,s)")
-                  .argumentType("decimal(p,s)")
-                  .variableArity()
+                  .variableArity("decimal(p,s)")
                   .build();
 
   returnType = fuzzReturnType(*signature);

diff --git a/velox/expression/tests/ExprTest.cpp b/velox/expression/tests/ExprTest.cpp
@@ -1424,8 +1424,7 @@ class StatefulVectorFunction : public exec::VectorFunction {
     return {exec::FunctionSignatureBuilder()
                 .typeVariable("T")
                 .returnType("integer")
-                .argumentType("T")
-                .variableArity()
+                .variableArity("T")
                 .build()};
   }
 
@@ -2008,8 +2007,7 @@ class NullArrayFunction : public exec::VectorFunction {
     return {exec::FunctionSignatureBuilder()
                 .typeVariable("T")
                 .returnType("array(varchar)")
-                .argumentType("T")
-                .variableArity()
+                .variableArity("T")
                 .build()};
   }
 };

diff --git a/velox/expression/tests/SignatureBinderTest.cpp b/velox/expression/tests/SignatureBinderTest.cpp
@@ -586,8 +586,7 @@ TEST(SignatureBinderTest, variableArity) {
   {
     auto signature = exec::FunctionSignatureBuilder()
                          .returnType("varchar")
-                         .argumentType("varchar")
-                         .variableArity()
+                         .variableArity("varchar")
                          .build();
 
     testSignatureBinder(signature, {}, VARCHAR());
@@ -602,8 +601,7 @@ TEST(SignatureBinderTest, variableArity) {
     auto signature = exec::FunctionSignatureBuilder()
                          .returnType("varchar")
                          .argumentType("integer")
-                         .argumentType("double")
-                         .variableArity()
+                         .variableArity("double")
                          .build();
 
     testSignatureBinder(signature, {INTEGER()}, VARCHAR());
@@ -617,8 +615,7 @@ TEST(SignatureBinderTest, variableArity) {
   {
     auto signature = exec::FunctionSignatureBuilder()
                          .returnType("varchar")
-                         .argumentType("any")
-                         .variableArity()
+                         .variableArity("any")
                          .build();
 
     testSignatureBinder(signature, {}, VARCHAR());
@@ -633,8 +630,7 @@ TEST(SignatureBinderTest, variableArity) {
     auto signature = exec::FunctionSignatureBuilder()
                          .returnType("timestamp")
                          .argumentType("integer")
-                         .argumentType("any")
-                         .variableArity()
+                         .variableArity("any")
                          .build();
 
     testSignatureBinder(signature, {INTEGER()}, TIMESTAMP());
@@ -683,8 +679,7 @@ TEST(SignatureBinderTest, unresolvable) {
   {
     auto signature = exec::FunctionSignatureBuilder()
                          .returnType("varchar")
-                         .argumentType("integer")
-                         .variableArity()
+                         .variableArity("integer")
                          .build();
 
     // wrong type

diff --git a/velox/functions/lib/MapConcat.cpp b/velox/functions/lib/MapConcat.cpp
@@ -161,8 +161,7 @@ class MapConcatFunction : public exec::VectorFunction {
                 .typeVariable("V")
                 .returnType("map(K,V)")
                 .argumentType("map(K,V)")
-                .argumentType("map(K,V)")
-                .variableArity()
+                .variableArity("map(K,V)")
                 .build()};
   }
 };

diff --git a/velox/functions/prestosql/ArrayConstructor.cpp b/velox/functions/prestosql/ArrayConstructor.cpp
@@ -113,8 +113,7 @@ class ArrayConstructor : public exec::VectorFunction {
         exec::FunctionSignatureBuilder()
             .typeVariable("T")
             .returnType("array(T)")
-            .argumentType("T")
-            .variableArity()
+            .variableArity("T")
             .build(),
     };
   }

diff --git a/velox/functions/prestosql/StringFunctions.cpp b/velox/functions/prestosql/StringFunctions.cpp
@@ -258,14 +258,14 @@ class ConcatFunction : public exec::VectorFunction {
             .returnType("varchar")
             .argumentType("varchar")
             .argumentType("varchar")
-            .variableArity()
+            .variableArity("varchar")
             .build(),
         // varbinary, varbinary,.. -> varbinary
         exec::FunctionSignatureBuilder()
             .returnType("varbinary")
             .argumentType("varbinary")
             .argumentType("varbinary")
-            .variableArity()
+            .variableArity("varbinary")
             .build(),
     };
   }

diff --git a/velox/functions/sparksql/Hash.cpp b/velox/functions/sparksql/Hash.cpp
@@ -717,8 +717,7 @@ void checkArgTypes(const std::vector<exec::VectorFunctionArg>& args) {
 std::vector<std::shared_ptr<exec::FunctionSignature>> hashSignatures() {
   return {exec::FunctionSignatureBuilder()
               .returnType("integer")
-              .argumentType("any")
-              .variableArity()
+              .variableArity("any")
               .build()};
 }
 
@@ -748,16 +747,14 @@ std::vector<std::shared_ptr<exec::FunctionSignature>> hashWithSeedSignatures() {
   return {exec::FunctionSignatureBuilder()
               .returnType("integer")
               .constantArgumentType("integer")
-              .argumentType("any")
-              .variableArity()
+              .variableArity("any")
               .build()};
 }
 
 std::vector<std::shared_ptr<exec::FunctionSignature>> xxhash64Signatures() {
   return {exec::FunctionSignatureBuilder()
               .returnType("bigint")
-              .argumentType("any")
-              .variableArity()
+              .variableArity("any")
               .build()};
 }
 
@@ -766,8 +763,7 @@ xxhash64WithSeedSignatures() {
   return {exec::FunctionSignatureBuilder()
               .returnType("bigint")
               .constantArgumentType("bigint")
-              .argumentType("any")
-              .variableArity()
+              .variableArity("any")
               .build()};
 }
 

diff --git a/velox/functions/sparksql/LeastGreatest.cpp b/velox/functions/sparksql/LeastGreatest.cpp
@@ -152,17 +152,15 @@ std::vector<std::shared_ptr<exec::FunctionSignature>> leastSignatures() {
     signatures.emplace_back(exec::FunctionSignatureBuilder()
                                 .returnType(type)
                                 .argumentType(type)
-                                .argumentType(type)
-                                .variableArity()
+                                .variableArity(type)
                                 .build());
   }
   signatures.emplace_back(exec::FunctionSignatureBuilder()
                               .integerVariable("p")
                               .integerVariable("s")
                               .returnType("decimal(p,s)")
                               .argumentType("decimal(p,s)")
-                              .argumentType("decimal(p,s)")
-                              .variableArity()
+                              .variableArity("decimal(p,s)")
                               .build());
   return signatures;
 }

diff --git a/velox/functions/tests/FunctionRegistryTest.cpp b/velox/functions/tests/FunctionRegistryTest.cpp
@@ -351,8 +351,7 @@ TEST_F(FunctionRegistryTest, getFunctionSignatures) {
       functionSignatures["variadic_func"].at(0)->toString(),
       exec::FunctionSignatureBuilder()
           .returnType("varchar")
-          .argumentType("varchar")
-          .variableArity()
+          .variableArity("varchar")
           .build()
           ->toString());