Skip to content
Merged
Show file tree
Hide file tree
Changes from 34 commits
Commits
Show all changes
38 commits
Select commit Hold shift + click to select a range
b8f1d4b
Increase the precision of decimals during compute
khwilson Sep 17, 2024
89f743d
migrate utility function to codegen
khwilson Sep 22, 2024
2564dcc
fix else condition
khwilson Sep 22, 2024
63340d7
improve casts
khwilson Sep 22, 2024
8a6959b
linting
khwilson Sep 23, 2024
d98ad4a
fix _pointer_ in casts
khwilson Sep 24, 2024
8c92b74
extract common code
khwilson Sep 30, 2024
cb6e0ed
continue simplifying code
khwilson Oct 1, 2024
f59a421
add doc and export
khwilson Oct 10, 2024
a426640
fix c++ tests
khwilson Oct 13, 2024
094e7e4
linting and doc fixes
khwilson Oct 13, 2024
1b45583
include hash aggregate sums
khwilson Oct 13, 2024
e87902e
fix more formatting
khwilson Oct 13, 2024
7c0d8d0
fix hash aggregate test
khwilson Oct 13, 2024
7c69607
update c++ docs on decimal aggregates
khwilson Oct 13, 2024
8dd4c49
move change to hash_aggregate_numeric
khwilson May 6, 2025
1b2eb85
Move type definition to the init and only keep for Sum
khwilson May 8, 2025
c4aa1c5
optionally promote a decimal in the hash aggregates
khwilson May 9, 2025
42addcd
fix and remove decimal32/64
khwilson May 11, 2025
4f50e27
lint
khwilson May 11, 2025
a27628a
fixes
khwilson May 14, 2025
8f2b752
Small simplification wrt WidenDecimalToMaxPrecision
zanmato1984 Jun 1, 2025
41fafbf
add tests and fix docs
khwilson Jun 8, 2025
08ac345
lint
khwilson Jun 9, 2025
ace7973
Make SFINAE check a virtual function
khwilson Jun 10, 2025
6e0551e
Format
zanmato1984 Jun 10, 2025
8e3da1f
Update cpp/src/arrow/compute/kernels/aggregate_test.cc
khwilson Jun 10, 2025
bdc4dbf
Update docs/source/cpp/compute.rst
khwilson Jun 10, 2025
55a2f46
remove extraneous template argument
khwilson Jun 10, 2025
432c57d
format
khwilson Jun 10, 2025
be4a043
add overflow test
khwilson Jun 15, 2025
7502951
fix merge conflict with main
khwilson Jun 15, 2025
540fb19
update submodule
khwilson Jun 15, 2025
5b666f8
slight doc modification
khwilson Jun 15, 2025
cc20e96
Update docs/source/cpp/compute.rst
zanmato1984 Jul 3, 2025
8a0acde
Use // to replace ///
zanmato1984 Jul 3, 2025
8d7e1b3
Refine bullet points for decimal promotion for sum in doc
zanmato1984 Jul 3, 2025
d97e8b2
Use // to replace ///
zanmato1984 Jul 3, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions cpp/src/arrow/acero/hash_aggregate_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -937,8 +937,8 @@ TEST_P(GroupBy, SumMeanProductDecimal) {

AssertDatumsEqual(ArrayFromJSON(struct_({
field("key_0", int64()),
field("hash_sum", decimal128(3, 2)),
field("hash_sum", decimal256(3, 2)),
field("hash_sum", decimal128(38, 2)),
field("hash_sum", decimal256(76, 2)),
field("hash_mean", decimal128(3, 2)),
field("hash_mean", decimal256(3, 2)),
field("hash_product", decimal128(3, 2)),
Expand Down
12 changes: 6 additions & 6 deletions cpp/src/arrow/compute/kernels/aggregate_basic.cc
Original file line number Diff line number Diff line change
Expand Up @@ -368,9 +368,9 @@ struct ProductImpl : public ScalarAggregator {
Status Finalize(KernelContext*, Datum* out) override {
if ((!options.skip_nulls && this->nulls_observed) ||
(this->count < options.min_count)) {
out->value = std::make_shared<OutputType>(out_type);
out->value = std::make_shared<OutputType>(this->out_type);
} else {
out->value = std::make_shared<OutputType>(this->product, out_type);
out->value = std::make_shared<OutputType>(this->product, this->out_type);
}
return Status::OK();
}
Expand Down Expand Up @@ -1049,10 +1049,10 @@ void RegisterScalarAggregateBasic(FunctionRegistry* registry) {
func = std::make_shared<ScalarAggregateFunction>("sum", Arity::Unary(), sum_doc,
&default_scalar_aggregate_options);
AddArrayScalarAggKernels(SumInit, {boolean()}, uint64(), func.get());
AddAggKernel(KernelSignature::Make({Type::DECIMAL128}, FirstType), SumInit, func.get(),
SimdLevel::NONE);
AddAggKernel(KernelSignature::Make({Type::DECIMAL256}, FirstType), SumInit, func.get(),
SimdLevel::NONE);
AddAggKernel(KernelSignature::Make({Type::DECIMAL128}, MaxPrecisionDecimalType),
SumInit, func.get(), SimdLevel::NONE);
AddAggKernel(KernelSignature::Make({Type::DECIMAL256}, MaxPrecisionDecimalType),
SumInit, func.get(), SimdLevel::NONE);
AddArrayScalarAggKernels(SumInit, SignedIntTypes(), int64(), func.get());
AddArrayScalarAggKernels(SumInit, UnsignedIntTypes(), uint64(), func.get());
AddArrayScalarAggKernels(SumInit, FloatingPointTypes(), float64(), func.get());
Expand Down
26 changes: 22 additions & 4 deletions cpp/src/arrow/compute/kernels/aggregate_basic.inc.cc
Original file line number Diff line number Diff line change
Expand Up @@ -95,9 +95,9 @@ struct SumImpl : public ScalarAggregator {
Status Finalize(KernelContext*, Datum* out) override {
if ((!options.skip_nulls && this->nulls_observed) ||
(this->count < options.min_count)) {
out->value = std::make_shared<OutputType>(out_type);
out->value = std::make_shared<OutputType>(this->out_type);
} else {
out->value = std::make_shared<OutputType>(this->sum, out_type);
out->value = std::make_shared<OutputType>(this->sum, this->out_type);
}
return Status::OK();
}
Expand Down Expand Up @@ -168,6 +168,14 @@ struct SumLikeInit {
const ScalarAggregateOptions& options)
: ctx(ctx), type(type), options(options) {}

/// If this returns true, then the aggregator will promote a decimal
/// to the maximum precision for that type. For instance, a decimal128(3, 2)
/// will be promoted to a decimal128(38, 2)
///
/// TODO: Ideally this should be configurable via the function options with an
/// enum PrecisionPolicy { PROMOTE_TO_MAX, DEMOTE_TO_DOUBLE, NO_PROMOTION }
virtual bool PromoteDecimal() const { return true; }

Status Visit(const DataType&) { return Status::NotImplemented("No sum implemented"); }

Status Visit(const HalfFloatType&) {
Expand All @@ -187,10 +195,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)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These comments might be unnecessary now?

template <typename Type>
enable_if_decimal<Type, Status> Visit(const Type&) {
state.reset(new KernelClass<Type>(type, options));
return Status::OK();
if (PromoteDecimal()) {
ARROW_ASSIGN_OR_RAISE(auto ty, WidenDecimalToMaxPrecision(type));
state.reset(new KernelClass<Type>(ty, options));
return Status::OK();
} else {
state.reset(new KernelClass<Type>(type, options));
return Status::OK();
}
}

virtual Status Visit(const NullType&) {
Expand Down Expand Up @@ -275,6 +291,8 @@ struct MeanKernelInit : public SumLikeInit<KernelClass> {
const ScalarAggregateOptions& options)
: SumLikeInit<KernelClass>(ctx, type, options) {}

bool PromoteDecimal() const override { return false; }

Status Visit(const NullType&) override {
this->state.reset(new NullSumImpl<DoubleType>(this->options));
return Status::OK();
Expand Down
Loading
Loading