ARROW-13737: [C++] Support for grouped aggregation over scalar columns #10994
Conversation
There was a problem hiding this comment.
It looks like most of this could be factored out to reuse in both GroupByUsingExecPlan overloads.
There was a problem hiding this comment.
Is there a reason we don't want to be explicit in our capture here?
There was a problem hiding this comment.
We may capture explicitly when lifetime is at stake, e.g. in async code where it's important to delineate what exactly needs to survive past the enclosing scope. Here, the callable is executed synchronously, listing every capture explicitly is more of a nuisance (both for typing and readability-wise).
There was a problem hiding this comment.
@felipeblazing Out of curiosity, why do you think capturing explicitly would be better here?
There was a problem hiding this comment.
can we also capture explicitly here?
pitrou
left a comment
There was a problem hiding this comment.
+1 from me. @felipeblazing Feel free to review again as well.
Also fixes a major bug in grouped var/std, where multiple batches fed to the same state instance would improperly update state.