Skip to content

Add any_value alias for arbitrary aggregate.#7689

Closed
amitkdutta wants to merge 1 commit intofacebookincubator:mainfrom
amitkdutta:export-D51521293
Closed

Add any_value alias for arbitrary aggregate.#7689
amitkdutta wants to merge 1 commit intofacebookincubator:mainfrom
amitkdutta:export-D51521293

Conversation

@amitkdutta
Copy link
Copy Markdown
Contributor

Summary:
Presto added any_value alias for arbitrary aggregate in
prestodb/presto#21389

Differential Revision: D51521293

@netlify
Copy link
Copy Markdown

netlify bot commented Nov 22, 2023

Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit 106fb45
🔍 Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/655e2ce85679130008802dea

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. fb-exported labels Nov 22, 2023
@facebook-github-bot
Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D51521293

@amitkdutta amitkdutta requested a review from kagamiori November 22, 2023 06:56
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This name is used for error message. I passed the first name, also open to pass a concatenated string like "arbitrary/any_value".

Copy link
Copy Markdown
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.

@amitkdutta Thanks. Would you update documentation?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

return value doesn't seem to be used; maybe change to void

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@mbasmanova @kagamiori I think we can make all the register functions to return void. The return type is not used. In a separate change. Let me know what you think.

Summary:

Presto added `any_value` alias for arbitrary aggregate in
prestodb/presto#21389

Differential Revision: D51521293
@facebook-github-bot
Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D51521293

Copy link
Copy Markdown
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.

@facebook-github-bot
Copy link
Copy Markdown
Contributor

This pull request has been merged in c1824e6.

@conbench-facebook
Copy link
Copy Markdown

Conbench analyzed the 1 benchmark run on commit c1824e67.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

amitkdutta pushed a commit to amitkdutta/velox that referenced this pull request Dec 4, 2023
…or#7864)

Summary:

Aggregate function registration does not utilize return type. Some registrations are done with void and some are done with return type - where return type is unused. Making all of them void, similar to what is done in  facebookincubator#7689

Reviewed By: mbasmanova

Differential Revision: D51593723
facebook-github-bot pushed a commit that referenced this pull request Dec 4, 2023
Summary:
Pull Request resolved: #7864

Aggregate function registration does not utilize return type. Some registrations are done with void and some are done with return type - where return type is unused. Making all of them void, similar to what is done in  #7689

Reviewed By: mbasmanova

Differential Revision: D51593723

fbshipit-source-id: 088e25e9c8547cd4bf6260621ced4e07d5dfb7aa
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. fb-exported Merged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants