Add custom argument generators for Presto decimal functions #9715

rui-mo · 2024-05-06T07:56:06Z

To generate argument types in the decimal fuzzer test correctly, customized
argument type generators are required. This PR adds arg generators for the
Presto functions plus/minus, multiply, divide, and floor/round.
Inspired by #9358.

netlify · 2024-05-06T07:56:22Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`cbdaa94`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/663ad1c60d33f70008277e56

mbasmanova

@rui-mo Looks great % small comments.

mbasmanova · 2024-05-06T08:45:34Z

velox/functions/prestosql/tests/ArgGeneratorTest.cpp

+class ArgGeneratorTest : public testing::Test {};
+
+TEST_F(ArgGeneratorTest, plusMinus) {
+  auto signature =


Instead of hard-coding function signature here, can we fetch it from the registry?

Added this function to fetch signature from the registry.

std::vector<const exec::FunctionSignature*> getSignatures(
const FunctionSignatureMap& map,
const std::string& functionName,
const std::string& returnType);

mbasmanova · 2024-05-06T08:47:12Z

velox/functions/prestosql/fuzzer/FloorAndRoundArgGenerator.h

+#include "velox/expression/fuzzer/ArgGenerator.h"
+
+namespace facebook::velox::exec::test {
+namespace {


no anonymous namespaces in header files, please

use 'detail' namespace or make this a private static method

Thank you for pointing out this. Made it a private static method.

mbasmanova · 2024-05-06T08:50:52Z

velox/functions/prestosql/tests/ArgGeneratorTest.cpp

+        DECIMAL(38, 20),
+        DECIMAL(38, 38),
+        DECIMAL(38, 0)}) {
+    fuzzer::test::assertReturnType(generator, *signature, returnType);


nit: it might be easier to read if this is written without a loop

fuzzer::test::assertReturnType(generator, *signature, DECIMAL(10, 2)); fuzzer::test::assertReturnType(generator, *signature, DECIMAL(18, 18)); ...

Updated. Thanks.

mbasmanova · 2024-05-06T08:51:56Z

velox/expression/fuzzer/tests/ArgGeneratorTestUtils.cpp

+  if (binder.tryBind()) {
+    actualType = binder.tryResolveReturnType();
+  } else {
+    VELOX_FAIL(


nit:

VELOX_CHECK(binder.tryBind(), ...) auto actualType = binder.tryResolveReturnType();

mbasmanova · 2024-05-07T12:28:12Z

velox/expression/fuzzer/tests/ArgGeneratorTestUtils.cpp

+  EXPECT_TRUE(argTypes.empty());
+}
+
+std::vector<const exec::FunctionSignature*> getSignatures(


There is FunctionBaseTest::getSignatureStrings method. Maybe add another one that returns signatures as is.

Thanks for your suggestion. Added getSignatures in FunctionBaseTest.

static std::unordered_set<const exec::FunctionSignature*> getSignatures(
const std::string& functionName,
const std::string& returnType);

mbasmanova · 2024-05-07T12:29:04Z

velox/functions/prestosql/tests/ArgGeneratorTest.cpp

+  const auto signatures = getSignatures(allSignatures_, "plus", "decimal");
+  VELOX_CHECK_EQ(signatures.size(), 1);
+  const auto& signature = *signatures[0];


This code repeats multiple time. Consider adding helper method:

const auto& signature = getOnlySignature("plus");

Updated. Thanks.

mbasmanova · 2024-05-07T14:51:25Z

velox/functions/prestosql/tests/utils/FunctionBaseTest.h

@@ -318,6 +318,11 @@ class FunctionBaseTest : public testing::Test,
    return result[0];
  }

+  // Returns a set of signatures for the given function name and return type.
+  static std::unordered_set<const exec::FunctionSignature*> getSignatures(


Why do we return a set? An std::vector should work just fine, no?

Let's document 'returnType' parameter. It may not be obvious what the value is.

I was using set to keep similar style with FunctionBaseTest::getSignatureStrings method. Changed to use vector.

Document 'returnType' parameter as below.

The name of expected return type defined in function signature.

Thanks.

mbasmanova

Thanks.

rui-mo · 2024-05-09T00:16:25Z

@mbasmanova Could we merge this PR? Thanks!

mbasmanova · 2024-05-09T12:41:31Z

@rui-mo Yes, we'll merge this soon.

CC: @kgpai

facebook-github-bot · 2024-05-09T19:05:28Z

@kgpai has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

rui-mo · 2024-05-15T00:33:12Z

Hi @kgpai, I notice there are some internal failures. If there is anything needs to be fixed, please kindly let me know. Thanks.

facebook-github-bot · 2024-05-15T23:38:48Z

@kgpai merged this pull request in 08ffe22.

conbench-facebook · 2024-05-16T00:00:40Z

Conbench analyzed the 1 benchmark run on commit 08ffe220.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

…incubator#9715) Summary: To generate argument types in the decimal fuzzer test correctly, customized argument type generators are required. This PR adds arg generators for the Presto functions plus/minus, multiply, divide, and floor/round. Inspired by facebookincubator#9358. Pull Request resolved: facebookincubator#9715 Reviewed By: Yuhta Differential Revision: D57173196 Pulled By: kgpai fbshipit-source-id: 0bb23aaa55be046b27e4edef2aef9ed4cdf0545e

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label May 6, 2024

rui-mo force-pushed the presto_arg branch 3 times, most recently from bcae811 to 8249229 Compare May 6, 2024 08:25

mbasmanova approved these changes May 6, 2024

View reviewed changes

rui-mo force-pushed the presto_arg branch 4 times, most recently from b73776a to 6eb2e13 Compare May 7, 2024 05:27

mbasmanova reviewed May 7, 2024

View reviewed changes

mbasmanova changed the title ~~Add Presto arg generators~~ Add custom argument generators for Presto decimal functions May 7, 2024

rui-mo force-pushed the presto_arg branch 2 times, most recently from 4863a71 to 2edc272 Compare May 7, 2024 14:47

mbasmanova reviewed May 7, 2024

View reviewed changes

Add Presto arg generators

cbdaa94

rui-mo force-pushed the presto_arg branch 3 times, most recently from aba3f02 to 2de6929 Compare May 8, 2024 01:10

mbasmanova approved these changes May 8, 2024

View reviewed changes

mbasmanova added the ready-to-merge PR that have been reviewed and are ready for merging. PRs with this tag notify the Velox Meta oncall label May 8, 2024

rui-mo force-pushed the presto_arg branch from 2de6929 to cbdaa94 Compare May 8, 2024 01:13

Yuhta approved these changes May 15, 2024

View reviewed changes

facebook-github-bot closed this in 08ffe22 May 15, 2024

facebook-github-bot added the Merged label May 15, 2024

rui-mo mentioned this pull request May 28, 2024

Extend expression fuzzer test to support decimal #9149

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add custom argument generators for Presto decimal functions #9715

Add custom argument generators for Presto decimal functions #9715

rui-mo commented May 6, 2024 •

edited

Loading

netlify bot commented May 6, 2024 •

edited

Loading

mbasmanova left a comment

mbasmanova May 6, 2024

rui-mo May 7, 2024 •

edited

Loading

mbasmanova May 6, 2024

rui-mo May 7, 2024

mbasmanova May 6, 2024

rui-mo May 7, 2024

mbasmanova May 6, 2024

mbasmanova May 7, 2024

rui-mo May 7, 2024

mbasmanova May 7, 2024

rui-mo May 7, 2024

mbasmanova May 7, 2024

rui-mo May 8, 2024

mbasmanova left a comment

rui-mo commented May 9, 2024

mbasmanova commented May 9, 2024

facebook-github-bot commented May 9, 2024

rui-mo commented May 15, 2024

facebook-github-bot commented May 15, 2024

conbench-facebook bot commented May 16, 2024

Add custom argument generators for Presto decimal functions #9715

Add custom argument generators for Presto decimal functions #9715

Conversation

rui-mo commented May 6, 2024 • edited Loading

netlify bot commented May 6, 2024 • edited Loading

✅ Deploy Preview for meta-velox canceled.

mbasmanova left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rui-mo May 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbasmanova left a comment

Choose a reason for hiding this comment

rui-mo commented May 9, 2024

mbasmanova commented May 9, 2024

facebook-github-bot commented May 9, 2024

rui-mo commented May 15, 2024

facebook-github-bot commented May 15, 2024

conbench-facebook bot commented May 16, 2024

rui-mo commented May 6, 2024 •

edited

Loading

netlify bot commented May 6, 2024 •

edited

Loading

rui-mo May 7, 2024 •

edited

Loading