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.

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

feat: add any_value aggregate function #321

Merged
merged 5 commits into from
Sep 19, 2022

Conversation

vibhatha
Copy link
Contributor

@vibhatha vibhatha commented Sep 8, 2022

This PR adds any_value aggregate function.

@jvanstraten jvanstraten changed the title feat: Adding any_value aggregate function WIP feat: add any_value aggregate function WIP Sep 8, 2022
extensions/functions_arithmetic.yaml Outdated Show resolved Hide resolved
extensions/functions_arithmetic.yaml Outdated Show resolved Hide resolved
@vibhatha vibhatha marked this pull request as ready for review September 8, 2022 15:58
@vibhatha
Copy link
Contributor Author

vibhatha commented Sep 8, 2022

@jvanstraten I updated the PR.

@vibhatha vibhatha changed the title feat: add any_value aggregate function WIP feat: add any_value aggregate function Sep 8, 2022
@jvanstraten
Copy link
Contributor

Sorry, I don't know why I didn't notice this before, but there's no reason for this to be limited to numeric types. It can just be any_value(T) -> T. It should also be in the file for generic aggregates.

@vibhatha
Copy link
Contributor Author

vibhatha commented Sep 9, 2022

@jvanstraten updated the PR.

Comment on lines 1371 to 1407

- name: "any_value"
description: >
Selects an arbitrary value from a group of values.

If the input is empty, the function returns null.
impls:
- args:
- name: x
value: i8
nullability: DECLARED_OUTPUT
return: i8?
- args:
- name: x
value: i16
nullability: DECLARED_OUTPUT
return: i16?
- args:
- name: x
value: i32
nullability: DECLARED_OUTPUT
return: i32?
- args:
- name: x
value: i64
nullability: DECLARED_OUTPUT
return: i64?
- args:
- name: x
value: fp32
nullability: DECLARED_OUTPUT
return: fp32?
- args:
- name: x
value: fp64
nullability: DECLARED_OUTPUT
return: fp64?
Copy link
Contributor

Choose a reason for hiding this comment

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

This is completely covered by the f(any) -> any? variant, and it isn't really an arithmetic function to begin with. So why is this still here?

@vibhatha
Copy link
Contributor Author

@jvanstraten updated the PR. Is it okay now?

Copy link
Contributor

@jvanstraten jvanstraten left a comment

Choose a reason for hiding this comment

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

Yeah, LGTM now, I think.

@jvanstraten jvanstraten merged commit 6f603d3 into substrait-io:main Sep 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants