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

Add invocation property to AggregateFunction message for specifying distinct vs all #191

Merged
merged 1 commit into from
May 12, 2022

Conversation

JamesRTaylor
Copy link
Contributor

No description provided.

@JamesRTaylor JamesRTaylor force-pushed the aggregate_func_invocation branch 2 times, most recently from 8586aed to b65deab Compare May 11, 2022 00:02
@JamesRTaylor JamesRTaylor requested a review from jacques-n May 11, 2022 00:32
@JamesRTaylor JamesRTaylor force-pushed the aggregate_func_invocation branch from b65deab to 4b0879f Compare May 12, 2022 01:08
…ying distinct vs all

* feat: add new invocation property to AggregateFunction message
* docs: add AggregateFunction invocation docs
@JamesRTaylor JamesRTaylor force-pushed the aggregate_func_invocation branch from bfdf260 to f27e8c0 Compare May 12, 2022 01:45
@JamesRTaylor JamesRTaylor changed the title Add invocation property to aggregate message for specifying distinct vs all Add invocation property to AggregateFunction message for specifying distinct vs all May 12, 2022
@JamesRTaylor
Copy link
Contributor Author

I've updated the comment, @jacques-n. I also moved the new invocation property to the right proto message.

@jacques-n
Copy link
Contributor

@cpcloud and @jvanstraten , you good with this change? It's not breaking but it is an introduction of a new feature in the spec.

Copy link
Contributor

@cpcloud cpcloud left a comment

Choose a reason for hiding this comment

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

LGTM, this will get picked up in the next release on Sunday!

@jacques-n jacques-n merged commit 373b33f into substrait-io:main May 12, 2022
@jacques-n
Copy link
Contributor

Thanks @JamesRTaylor !

@jvanstraten
Copy link
Contributor

Looks like I'm too late, but looks good to me for what it's worth :)

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.

4 participants