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 restricting arguments as orderable or comparable in the function signature #6906

Closed
wants to merge 2 commits into from
Closed

Allow restricting arguments as orderable or comparable in the function signature #6906

wants to merge 2 commits into from

Conversation

duanmeng
Copy link
Collaborator

@duanmeng duanmeng commented Oct 5, 2023 •
edited
Loading

In Presto, input to min/max is restricted to orderable types.
For example, MAP type is not orderable. Velox currently allows MAP inputs.

This PR adds two APIs in FunctionSignatureBuilder: orderableTypeVariable
and comparableTypeVariable. Use type's orderable and comparable flag
added in #6770 to do the check
during type binding in SignatureBinder.tryBind.

Part of #6718

@netlify
Copy link

netlify bot commented Oct 5, 2023 •
edited
Loading

✅ Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit 4ee377b
🔍 Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/651ea50226da070008945326

@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 Oct 5, 2023
@duanmeng duanmeng force-pushed the orderCompara branch 2 times, most recently from 53be93d to 905275c Compare October 5, 2023 08:49
mbasmanova
mbasmanova reviewed Oct 5, 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 Nice change. Looks good % some minor comments.

velox/expression/FunctionSignature.h Outdated
@@ -58,6 +60,27 @@ class SignatureVariable {
return knownTypesOnly_;
}

void setKnownTypesOnly(bool knownTypesOnly) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's keep this class immutable and remove these setters.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Got it, fixed.

velox/expression/FunctionSignature.h Outdated
addVariable(
variables_,
SignatureVariable(name, "", ParameterType::kTypeParameter, true));
if (variables_.find(name) == variables_.end()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this logic is to allow calling both orderableTypeVariable and knownTypeVariable in the same signature. Do we need this? How about we don't allow if and if necessary add knownOrderableTypeVariable later?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It seems we do not need it for now. It is better to make them mutually exclusive and keep SignatureVarible immutable. I will update this PR according to these principles.

velox/expression/FunctionSignature.cpp
: name_{std::move(name)},
constraint_(constraint.has_value() ? std::move(constraint.value()) : ""),
type_{type},
knownTypesOnly_(knownTypesOnly) {
knownTypesOnly_(knownTypesOnly),
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a check that the new properties apply only when isTypeParameter() true?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added.

velox/expression/tests/SignatureBinderTest.cpp
@@ -378,6 +378,53 @@ TEST(SignatureBinderTest, knownOnly) {
}
}

TEST(SignatureBinderTest, orderableComparable) {
auto signature = exec::FunctionSignatureBuilder()
.orderableTypeVariable("T")
Copy link
Contributor

Choose a reason for hiding this comment

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

orderable implies comparable, hence, no point specifying both

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

mbasmanova
mbasmanova approved these changes Oct 5, 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.

Thanks.

velox/expression/FunctionSignature.h
@@ -79,6 +93,8 @@ class SignatureVariable {
// This property only applies to type variables and indicates if the type
// can bind to unknown or not.
bool knownTypesOnly_ = false;
bool orderableTypesOnly_ = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps, update the comment above:

// The following properties apply only to type variables and indicate whether the type can bind only to known, orderable or comparable types.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated

velox/expression/FunctionSignature.h Outdated
FunctionSignatureBuilder& knownTypeVariable(const std::string& name);

/// Orderable implies comparable, this method would enable
/// comaprableTypesOnly_ too.
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: comaprableTypesOnly_ -> comparableTypesOnly_

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

@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.

@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.

@duanmeng duanmeng mentioned this pull request Oct 5, 2023
Closed
@facebook-github-bot
Copy link
Contributor

@mbasmanova merged this pull request in 96f8ef5.

@conbench-facebook
Copy link

Conbench analyzed the 1 benchmark run on commit 96f8ef5f.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

@duanmeng duanmeng mentioned this pull request Oct 7, 2023
Closed
facebook-github-bot pushed a commit that referenced this pull request Oct 12, 2023
@duanmeng @facebook-github-bot
Respect orderable constraint in ArgumentTypeFuzzer (#6950)
30e1854 
Summary:
PR #6906 added support to restricting arguments as orderable.

This PR changes ArgumentTypeFuzzer to respect this constraint.

Part of #6718

Pull Request resolved: #6950

Reviewed By: Yuhta

Differential Revision: D50212902

Pulled By: mbasmanova

fbshipit-source-id: 318b5e443baba7b6800539fafcf34946e6f0e91d
ericyuliu pushed a commit to ericyuliu/velox that referenced this pull request Oct 12, 2023
@duanmeng @ericyuliu
Allow restricting arguments as orderable or comparable in the functio…
f8d13f3 
…n signature (facebookincubator#6906)

Summary:
In Presto, input to min/max is restricted to orderable types.
For example, MAP type is not orderable. Velox currently allows MAP inputs.

This PR adds two APIs in `FunctionSignatureBuilder`: `orderableTypeVariable`
and `comparableTypeVariable`. Use type's `orderable` and `comparable` flag
added in facebookincubator#6770 to do the check
during type binding in `SignatureBinder.tryBind`.

Part of facebookincubator#6718

Pull Request resolved: facebookincubator#6906

Reviewed By: xiaoxmeng

Differential Revision: D49951511

Pulled By: mbasmanova

fbshipit-source-id: 17ee5fd75b25a7df636279d07e99773105349220
ericyuliu pushed a commit to ericyuliu/velox that referenced this pull request Oct 12, 2023
@duanmeng @ericyuliu
Respect orderable constraint in ArgumentTypeFuzzer (facebookincubator…
a51ac3f 
…#6950)

Summary:
PR facebookincubator#6906 added support to restricting arguments as orderable.

This PR changes ArgumentTypeFuzzer to respect this constraint.

Part of facebookincubator#6718

Pull Request resolved: facebookincubator#6950

Reviewed By: Yuhta

Differential Revision: D50212902

Pulled By: mbasmanova

fbshipit-source-id: 318b5e443baba7b6800539fafcf34946e6f0e91d
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

@aditi-pandit aditi-pandit Awaiting requested review from aditi-pandit

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.
3 participants
@duanmeng @facebook-github-bot @mbasmanova