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

Support TypeSignature::Nullary #13354

Merged
merged 4 commits into from
Nov 12, 2024
Merged

Support TypeSignature::Nullary #13354

merged 4 commits into from
Nov 12, 2024

Conversation

jayzhan211
Copy link
Contributor

@jayzhan211 jayzhan211 commented Nov 11, 2024 •
edited
Loading

Which issue does this PR close?

Closes #.

Rationale for this change

Currently there are uniform(0), any(0), exact(0) 3 ways to describe zero argument signature. To avoid confusion, add TypeSignature::ZeroArg and disable 0 argument for those 3 signatures, they now require at least one argument.

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

@jayzhan211
support zero arg
d0f27c6 
Signed-off-by: jayzhan211 <[email protected]>
@github-actions github-actions bot added logical-expr Logical plan and expressions functions labels Nov 11, 2024
@jayzhan211 jayzhan211 marked this pull request as ready for review November 11, 2024 10:35
comphead
comphead reviewed Nov 11, 2024
View reviewed changes
datafusion/expr-common/src/signature.rs Outdated
@@ -135,6 +134,8 @@ pub enum TypeSignature {
/// Null is considerd as `Utf8` by default
/// Dictionary with string value type is also handled.
String(usize),
/// Zero argument
ZeroArg,
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe NoArgs would be more explanatory? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't see the difference 👀

Copy link
Contributor

Choose a reason for hiding this comment

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

Its choice of wording yes. Another option we can take from https://en.wikipedia.org/wiki/Arity and name it Nullary to be in sync with unary, binary functions we already use.

@findepi
Copy link
Member

findepi commented Nov 11, 2024

I think it would be nice to support a signature defined by something like Vec<LogicalType>
then ZeroArg / NoArgs would be just represented by an empty vec.
What do you think?

@jayzhan211
Copy link
Contributor Author

I think it would be nice to support a signature defined by something like Vec<LogicalType> then ZeroArg / NoArgs would be just represented by an empty vec. What do you think?

Signature::coercible(vec![], Volatility::Immutable) can represent this already. And you can also do Any(0), Uniform(0), so I want a standard representation for zero arg

@jayzhan211
rename to nullary
825e714 
Signed-off-by: jayzhan211 <[email protected]>
@jayzhan211 jayzhan211 changed the title Support TypeSignature::ZeroArg Support TypeSignature::Nullary Nov 12, 2024
comphead
comphead approved these changes Nov 12, 2024
View reviewed changes
Copy link
Contributor

@comphead comphead left a comment

Choose a reason for hiding this comment

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

lgtm thanks @jayzhan211 👍

@jayzhan211
rename
26abab9 
Signed-off-by: jayzhan211 <[email protected]>
@jayzhan211
tostring
d02297c 
Signed-off-by: jayzhan211 <[email protected]>
2010YOUY01
2010YOUY01 approved these changes Nov 12, 2024
View reviewed changes
Copy link
Contributor

@2010YOUY01 2010YOUY01 left a comment

Choose a reason for hiding this comment

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

It's great to have only one case for defining a signature with zero arguments. This will make it easier to use and less error-prone.
(Though I think "ZeroArg" or "NoArg" would be slightly more friendly for non-native English speakers)

Weijun-H
Weijun-H approved these changes Nov 12, 2024
View reviewed changes
Copy link
Member

@Weijun-H Weijun-H left a comment

Choose a reason for hiding this comment

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

Ship it! Thanks @jayzhan211

@Weijun-H Weijun-H merged commit 8d6899e into apache:main Nov 12, 2024
25 checks passed
@Weijun-H
Copy link
Member

Thanks @2010YOUY01 and @comphead for review.

@jayzhan211 jayzhan211 deleted the zero-arg-sig branch November 13, 2024 00:46
alamb pushed a commit to alamb/datafusion that referenced this pull request Nov 13, 2024
@jayzhan211 @alamb
Support TypeSignature::Nullary (apache#13354)
de28a30 
* support zero arg

Signed-off-by: jayzhan211 <[email protected]>

* rename to nullary

Signed-off-by: jayzhan211 <[email protected]>

* rename

Signed-off-by: jayzhan211 <[email protected]>

* tostring

Signed-off-by: jayzhan211 <[email protected]>

---------

Signed-off-by: jayzhan211 <[email protected]>
@alamb alamb mentioned this pull request Nov 20, 2024
Closed
3 tasks
@alamb alamb mentioned this pull request Dec 13, 2024
Open
@alamb
Copy link
Contributor

alamb commented Dec 13, 2024 •
edited
Loading

BTW I hit a usability rough edge due to this change I think:

@alamb alamb mentioned this pull request Dec 13, 2024
Open
10 tasks
kylebarron
kylebarron reviewed Dec 17, 2024
View reviewed changes
datafusion/expr-common/src/signature.rs
@@ -135,6 +134,8 @@ pub enum TypeSignature {
/// Null is considerd as `Utf8` by default
/// Dictionary with string value type is also handled.
String(usize),
/// Zero argument
NullAry,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the A here intended to be upper-case? I'd have expected Nullary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nullary looks good to me too

Copy link
Contributor

Choose a reason for hiding this comment

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

@alamb alamb mentioned this pull request Dec 17, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kylebarron kylebarron kylebarron left review comments

@alamb alamb alamb left review comments

@2010YOUY01 2010YOUY01 2010YOUY01 approved these changes

@Weijun-H Weijun-H Weijun-H approved these changes

@comphead comphead comphead approved these changes

Assignees
No one assigned
Labels
functions logical-expr Logical plan and expressions
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@jayzhan211 @findepi @Weijun-H @alamb @comphead @kylebarron @2010YOUY01