felipecrv commented on code in PR #15083:
URL: https://github.com/apache/arrow/pull/15083#discussion_r1064883321
##########
cpp/src/arrow/compute/kernels/hash_aggregate.cc:
##########
@@ -108,19 +108,31 @@ Result<TypeHolder> ResolveGroupOutputType(KernelContext*
ctx,
return checked_cast<GroupedAggregator*>(ctx->state())->out_type();
}
-HashAggregateKernel MakeKernel(InputType argument_type, KernelInit init) {
+HashAggregateKernel MakeKernel(std::shared_ptr<KernelSignature> signature,
+ KernelInit init) {
HashAggregateKernel kernel;
kernel.init = std::move(init);
- kernel.signature =
- KernelSignature::Make({std::move(argument_type),
InputType(Type::UINT32)},
- OutputType(ResolveGroupOutputType));
+ kernel.signature = std::move(signature);
kernel.resize = HashAggregateResize;
kernel.consume = HashAggregateConsume;
kernel.merge = HashAggregateMerge;
kernel.finalize = HashAggregateFinalize;
return kernel;
}
+HashAggregateKernel MakeKernel(InputType argument_type, KernelInit init) {
+ return MakeKernel(
+ KernelSignature::Make({std::move(argument_type),
InputType(Type::UINT32)},
+ OutputType(ResolveGroupOutputType)),
+ std::move(init));
+}
+
+HashAggregateKernel MakeUnaryKernel(KernelInit init) {
+ return MakeKernel(KernelSignature::Make({InputType(Type::UINT32)},
+ OutputType(ResolveGroupOutputType)),
+ std::move(init));
+}
+
Review Comment:
Very confusing. I decided to mention the confusing arity because the `Arity`
is mentioned in the context where `Make*Kernel` is called:
```cpp
{
auto func = std::make_shared<HashAggregateFunction>(
"hash_count", Arity::Binary(), hash_count_doc,
&default_count_options);
DCHECK_OK(func->AddKernel(
MakeKernel(InputType::Any(), HashAggregateInit<GroupedCountImpl>)));
DCHECK_OK(registry->AddFunction(std::move(func)));
}
{
auto func = std::make_shared<HashAggregateFunction>(
"hash_count_all", Arity::Unary(), hash_count_all_doc,
&default_count_all_options);
DCHECK_OK(func->AddKernel(MakeUnaryKernel(HashAggregateInit<GroupedCountAllImpl>)));
auto status = registry->AddFunction(std::move(func));
DCHECK_OK(status);
}
```
As I understood it, there is the arity of the kernels and the arity of the
aggregation functions. The former needs to have arity+1 to support aggregations
combined with group-by.
--
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]
