zanmato1984 commented on code in PR #44184:
URL: https://github.com/apache/arrow/pull/44184#discussion_r2074337147
##########
cpp/src/arrow/compute/kernels/hash_aggregate.cc:
##########
@@ -564,7 +564,7 @@ struct GroupedReducingAggregator : public GroupedAggregator
{
template <typename T = Type>
static enable_if_decimal<T, std::shared_ptr<DataType>> GetOutType(
const std::shared_ptr<DataType>& in_type) {
- return in_type;
+ return WidenDecimalToMaxPrecision(in_type).ValueOrDie();
Review Comment:
I don't think we should widen the output type for all the subclasses of
`GroupedReducingAggregator`. Promoting to max precision is reasonable for `sum`
but is not necessarily the case for others.
##########
cpp/src/arrow/compute/kernels/aggregate_basic.cc:
##########
@@ -365,11 +365,16 @@ struct ProductImpl : public ScalarAggregator {
}
Status Finalize(KernelContext*, Datum* out) override {
+ std::shared_ptr<DataType> out_type_ = this->out_type;
+ if (is_decimal(this->out_type->id())) {
+ ARROW_ASSIGN_OR_RAISE(out_type_,
WidenDecimalToMaxPrecision(this->out_type));
+ }
+
Review Comment:
This is hijacking the output type at the execution time. However we should
be consistent between the function signature (the return type determined at the
function resolution) and the value actually being returned.
You should probably change the return type somewhere in
https://github.com/apache/arrow/blob/067fd2a2c6e54d33b9ae8a3324f59bebe960d485/cpp/src/arrow/compute/kernels/aggregate_basic.cc#L1051-L1054
.
(BTW, what you did for hash aggregates is correct - widening the precision
to the actual output type rather than hijacking the return value at the last
minute).
--
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: github-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
