Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Allow marking arguments as constant in the function signature #4204

Closed
wants to merge 4 commits into from
Closed

Allow marking arguments as constant in the function signature #4204

wants to merge 4 commits into from

Conversation

duanmeng
Copy link
Collaborator

@duanmeng duanmeng commented Mar 7, 2023

Details in #4140

@netlify
Copy link

netlify bot commented Mar 7, 2023 •
edited
Loading

✅ Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit 6ca68da
🔍 Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/640767b326de820009a36928

@facebook-github-bot 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 Mar 7, 2023
mbasmanova
mbasmanova reviewed Mar 7, 2023
View reviewed changes
velox/expression/FunctionSignature.h Outdated
return isConstant_;
}

void setIsConstant(bool isConstant) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to keep this class immutable. Since is-constant is not a property of the type, consider, adding a list of boolean is-constant flags to the FunctionSignature class.

  const TypeSignature returnType_;
  const std::vector<TypeSignature> argumentTypes_;
  const std::vector<bool> constantArguments_;
  const bool variableArity_;

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

mbasmanova
mbasmanova reviewed Mar 7, 2023
View reviewed changes
velox/expression/tests/ArgumentTypeFuzzerTest.cpp Outdated
.argumentType("array(boolean)")
.build();

testFuzzingSuccess(signature, BIGINT(), {VARCHAR(), ARRAY(BOOLEAN())});
testFuzzingSuccess(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change here should not affect ArgumentTypeFuzzer, hence, I'm surprised to see changes to ArgumentTypeFuzzerTest.cpp. Let's remove these. Let's add tests to velox/exec/tests/FunctionSignatureBuilderTest.cpp instead.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@mbasmanova
Copy link
Contributor

@duanmeng Should we also update FunctionSignature::toString() to include "const" qualified for the arguments?

@duanmeng
Copy link
Collaborator Author

duanmeng commented Mar 7, 2023

@duanmeng Should we also update FunctionSignature::toString() to include "const" qualified for the arguments?

@mbasmanova This may be confused with the const in C++, or should we use constant as close to the semantics of velox constant vector?

@mbasmanova
Copy link
Contributor

This may be confused with the const in C++, or should we use constant as close to the semantics of velox constant vector?

@duanmeng I don't have a strong preference here. Here are some options.

(bigint, constant double) -> double

(bigint, const double) -> double

(bigint, fixed double) -> double

@duanmeng
Copy link
Collaborator Author

duanmeng commented Mar 7, 2023

This may be confused with the const in C++, or should we use constant as close to the semantics of velox constant vector?

@duanmeng I don't have a strong preference here. Here are some options.

(bigint, constant double) -> double

(bigint, const double) -> double

(bigint, fixed double) -> double

@mbasmanova I've resolved all the comments and updated the toString method, could you help to take a look?

mbasmanova
mbasmanova reviewed Mar 7, 2023
View reviewed changes
Copy link
Contributor

@mbasmanova mbasmanova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@duanmeng Looks great. Let's add tests for variadic and generic arguments and then merge.

velox/expression/FunctionSignature.cpp
VELOX_USER_CHECK_EQ(
argumentTypes.size(),
constantArguments.size(),
"Argument types size is not equal to constant flags");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice check.

velox/exec/tests/FunctionSignatureBuilderTest.cpp Outdated
@@ -177,3 +177,30 @@ TEST_F(FunctionSignatureBuilderTest, anyInReturn) {
},
"Type 'Any' cannot appear in return type");
}

TEST_F(FunctionSignatureBuilderTest, constFalgs) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typos: constFalgs -> constantFlags

velox/exec/tests/FunctionSignatureBuilderTest.cpp Outdated
EXPECT_TRUE(signature->constantArguments().at(1));
EXPECT_FALSE(signature->constantArguments().at(2));
EXPECT_EQ(
"(double,constant boolean,bigint) -> bigint", signature->toString());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice test. Would you also add tests for constant variadic argument and constant generic argument?

velox/exec/tests/FunctionSignatureBuilderTest.cpp Outdated
EXPECT_EQ(
"(double,constant boolean,bigint) -> bigint", signature->toString());

auto aggSignature = AggregateFunctionSignatureBuilder()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider moving this into a separate test and renaming to 'signature'.

TEST_F(FunctionSignatureBuilderTest, scalarConstantFlags) {

TEST_F(FunctionSignatureBuilderTest, aggregateConstantFlags) {

@mbasmanova mbasmanova changed the title Marking an argument as constant in the function signature Allow marking arguments as constant in the function signature Mar 7, 2023
mbasmanova
mbasmanova reviewed Mar 7, 2023
View reviewed changes
velox/exec/tests/FunctionSignatureBuilderTest.cpp Outdated
EXPECT_FALSE(signature->constantArguments().at(0));
EXPECT_TRUE(signature->constantArguments().at(1));
EXPECT_FALSE(signature->constantArguments().at(2));
std::cout << signature->toString() << std::endl;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Accidental?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed, sorry not to check and remove it before the push.

mbasmanova
mbasmanova approved these changes Mar 7, 2023
View reviewed changes
Copy link
Contributor

@mbasmanova mbasmanova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@duanmeng Looks great. Thank you for implementing this enhancement. Looking forward to a follow-up PR that leverages this functionality in the fuzzer.

@facebook-github-bot
Copy link
Contributor

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

@laithsakka
Copy link
Contributor

If an argument is marked const in the signature, is the expression compiler allowed to compile such a function if the input is not literal? I do not see changes related to that ?

could you also add a summary explaining the functionalities.

@mbasmanova
Copy link
Contributor

If an argument is marked const in the signature, is the expression compiler allowed to compile such a function if the input is not literal? I do not see changes related to that ?

could you also add a summary explaining the functionalities.

@laithsakka It would nice to add a check for 'const' arguments to the ExprCompiler. Let's do that in a follow-up PR though to keep PRs small and easy to review.

@facebook-github-bot
Copy link
Contributor

@mbasmanova merged this pull request in 09bd177.

@duanmeng duanmeng mentioned this pull request Sep 30, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mbasmanova mbasmanova mbasmanova approved these changes

@kagamiori kagamiori Awaiting requested review from kagamiori

@bikramSingh91 bikramSingh91 Awaiting requested review from bikramSingh91

@laithsakka laithsakka Awaiting requested review from laithsakka

Assignees
No one assigned
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@duanmeng @mbasmanova @facebook-github-bot @laithsakka