westonpace commented on code in PR #13218:
URL: https://github.com/apache/arrow/pull/13218#discussion_r888571708
##########
cpp/src/arrow/compute/api_aggregate.h:
##########
@@ -482,6 +404,21 @@ struct ARROW_EXPORT Aggregate {
const FunctionOptions* options;
};
+Result<std::vector<const HashAggregateKernel*>> GetKernels(
Review Comment:
Yes but `api_..._internal` feels a bit awkward. I created
`arrow/compute/exec/aggregate.h`. This follows the same convention as things
like `arrow/compute/exec/hash_join.h` which contains logic specific to the
operators but unaware of the fact its being used in an exec plan. I think it
makes sense for the aggregate tests to use this type. It's still using the
internal namespace but that's because we need it in the hash kernels tests and
at least this keeps the `kernels` folder cleaner.
Maybe a longer term fix would be to modify the hash aggregate tests to use
the exec plan and an aggregate node?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
