Make return type void during aggregate registration.#7864
Make return type void during aggregate registration.#7864amitkdutta wants to merge 1 commit intofacebookincubator:mainfrom
Conversation
✅ Deploy Preview for meta-velox canceled.
|
facebook-github-bot
added
the
CLA Signed
|
This pull request was exported from Phabricator. Differential Revision: D51593723 |
mbasmanova
left a comment
mbasmanova
left a comment
There was a problem hiding this comment.
@amitkdutta Looks good to me, but would be nice to ask @kagamiori to take a look as well.
|
Hi @amitkdutta, I think a previous PR intentionally make all aggregation registration functions return exec::AggregateRegistrationResult (#7133). Is there a reason why you'd like them to all return void instead? |
…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
amitkdutta
force-pushed
the
export-D51593723
branch
from
5896e50 to
7aa87d7
Compare
|
This pull request was exported from Phabricator. Differential Revision: D51593723 |
@kagamiori The return type is unused - hence making it void. Also a suggestion in #7689 (comment) |
|
This pull request has been merged in c0fadb9. |
|
Conbench analyzed the 1 benchmark run on commit There were no benchmark performance regressions. 🎉 The full Conbench report has more details. |
4 participants
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 #7689
Differential Revision: D51593723