zanmato1984 commented on code in PR #44184:
URL: https://github.com/apache/arrow/pull/44184#discussion_r2119695323
##########
cpp/src/arrow/compute/kernels/hash_aggregate_numeric.cc:
##########
@@ -42,7 +42,8 @@ namespace {
// Sum/Mean/Product implementation
template <typename Type, typename Impl,
- typename AccumulateType = typename FindAccumulatorType<Type>::Type>
+ typename AccumulateType = typename FindAccumulatorType<Type>::Type,
+ bool PromoteDecimal = false>
Review Comment:
Ditto.
##########
cpp/src/arrow/compute/kernels/aggregate_basic.inc.cc:
##########
@@ -187,10 +187,18 @@ struct SumLikeInit {
return Status::OK();
}
+ /// By default, we widen the decimal to max precision for SumLikes
+ /// However, this may not be the desired behaviour (see, e.g.,
MeanKernelInit)
Review Comment:
These comments might be unnecessary now?
##########
docs/source/cpp/compute.rst:
##########
@@ -264,7 +264,9 @@ the input to a single output value.
is always -1, regardless of whether there are nulls in the input.
* \(5) For decimal inputs, the resulting decimal will have the same
- precision and scale. The result is rounded away from zero.
+ scale and the maximum precision for its width. For instance, an array
Review Comment:
This is for `mean` right? And we didn't promote for `mean` in this PR.
##########
cpp/src/arrow/compute/kernels/aggregate_basic.inc.cc:
##########
@@ -157,7 +157,7 @@ struct NullSumImpl : public NullImpl<ArrowType> {
}
};
-template <template <typename> class KernelClass>
+template <template <typename> class KernelClass, bool PromoteDecimal = true>
Review Comment:
Does it have to be a template argument? If this argument is not performance
critical, we should make it a runtime one.
--
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]
