You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2021/07/15 17:00:18 UTC
[GitHub] [arrow] lidavidm opened a new pull request #10725: ARROW-9056: [C++] Support aggregations over scalars
lidavidm opened a new pull request #10725:
URL: https://github.com/apache/arrow/pull/10725
This adds various trivial implementations to support scalar aggregations over scalar inputs, for all kernels except `index`.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #10725: ARROW-9056: [C++] Support aggregations over scalars
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #10725:
URL: https://github.com/apache/arrow/pull/10725#issuecomment-880864307
https://issues.apache.org/jira/browse/ARROW-9056
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on a change in pull request #10725: ARROW-9056: [C++] Support aggregations over scalars
Posted by GitBox <gi...@apache.org>.
bkietz commented on a change in pull request #10725:
URL: https://github.com/apache/arrow/pull/10725#discussion_r670777575
##########
File path: cpp/src/arrow/compute/kernels/aggregate_basic_internal.h
##########
@@ -59,13 +59,22 @@ struct SumImpl : public ScalarAggregator {
using OutputType = typename TypeTraits<SumType>::ScalarType;
Status Consume(KernelContext*, const ExecBatch& batch) override {
- const auto& data = batch[0].array();
- this->count = data->length - data->GetNullCount();
- if (is_boolean_type<ArrowType>::value) {
- this->sum += static_cast<typename SumType::c_type>(BooleanArray(data).true_count());
+ if (batch[0].is_array()) {
+ const auto& data = batch[0].array();
+ this->count = data->length - data->GetNullCount();
+ if (is_boolean_type<ArrowType>::value) {
+ this->sum +=
+ static_cast<typename SumType::c_type>(BooleanArray(data).true_count());
+ } else {
+ this->sum +=
+ arrow::compute::detail::SumArray<CType, typename SumType::c_type>(*data);
+ }
} else {
- this->sum +=
- arrow::compute::detail::SumArray<CType, typename SumType::c_type>(*data);
+ const auto& data = *batch[0].scalar();
+ this->count = data.is_valid;
+ if (data.is_valid) {
+ this->sum += internal::UnboxScalar<ArrowType>::Unbox(data);
Review comment:
```suggestion
this->sum += internal::UnboxScalar<ArrowType>::Unbox(data) * batch.length;
```
##########
File path: cpp/src/arrow/compute/kernels/aggregate_basic_internal.h
##########
@@ -59,13 +59,22 @@ struct SumImpl : public ScalarAggregator {
using OutputType = typename TypeTraits<SumType>::ScalarType;
Status Consume(KernelContext*, const ExecBatch& batch) override {
- const auto& data = batch[0].array();
- this->count = data->length - data->GetNullCount();
- if (is_boolean_type<ArrowType>::value) {
- this->sum += static_cast<typename SumType::c_type>(BooleanArray(data).true_count());
+ if (batch[0].is_array()) {
+ const auto& data = batch[0].array();
+ this->count = data->length - data->GetNullCount();
+ if (is_boolean_type<ArrowType>::value) {
+ this->sum +=
+ static_cast<typename SumType::c_type>(BooleanArray(data).true_count());
+ } else {
+ this->sum +=
+ arrow::compute::detail::SumArray<CType, typename SumType::c_type>(*data);
+ }
} else {
- this->sum +=
- arrow::compute::detail::SumArray<CType, typename SumType::c_type>(*data);
+ const auto& data = *batch[0].scalar();
+ this->count = data.is_valid;
Review comment:
```suggestion
this->count += data.is_valid * batch.length;
```
##########
File path: cpp/src/arrow/compute/kernels/aggregate_basic_internal.h
##########
@@ -59,13 +59,22 @@ struct SumImpl : public ScalarAggregator {
using OutputType = typename TypeTraits<SumType>::ScalarType;
Status Consume(KernelContext*, const ExecBatch& batch) override {
- const auto& data = batch[0].array();
- this->count = data->length - data->GetNullCount();
- if (is_boolean_type<ArrowType>::value) {
- this->sum += static_cast<typename SumType::c_type>(BooleanArray(data).true_count());
+ if (batch[0].is_array()) {
+ const auto& data = batch[0].array();
+ this->count = data->length - data->GetNullCount();
Review comment:
```suggestion
this->count += data->length - data->GetNullCount();
```
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on a change in pull request #10725: ARROW-9056: [C++] Support aggregations over scalars
Posted by GitBox <gi...@apache.org>.
lidavidm commented on a change in pull request #10725:
URL: https://github.com/apache/arrow/pull/10725#discussion_r670726923
##########
File path: cpp/src/arrow/compute/kernels/aggregate_basic.cc
##########
@@ -59,10 +59,16 @@ struct CountImpl : public ScalarAggregator {
explicit CountImpl(ScalarAggregateOptions options) : options(std::move(options)) {}
Status Consume(KernelContext*, const ExecBatch& batch) override {
- const ArrayData& input = *batch[0].array();
- const int64_t nulls = input.GetNullCount();
- this->nulls += nulls;
- this->non_nulls += input.length - nulls;
+ if (batch[0].is_array()) {
+ const ArrayData& input = *batch[0].array();
+ const int64_t nulls = input.GetNullCount();
+ this->nulls += nulls;
+ this->non_nulls += input.length - nulls;
+ } else {
+ const Scalar& input = *batch[0].scalar();
+ this->nulls += !input.is_valid;
Review comment:
Looks like this only affects Count. I've added a test to exercise it as part of the exec plan tests, since it's hard to hit that particular case otherwise.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on a change in pull request #10725: ARROW-9056: [C++] Support aggregations over scalars
Posted by GitBox <gi...@apache.org>.
lidavidm commented on a change in pull request #10725:
URL: https://github.com/apache/arrow/pull/10725#discussion_r670784085
##########
File path: cpp/src/arrow/compute/kernels/aggregate_basic_internal.h
##########
@@ -59,13 +59,22 @@ struct SumImpl : public ScalarAggregator {
using OutputType = typename TypeTraits<SumType>::ScalarType;
Status Consume(KernelContext*, const ExecBatch& batch) override {
- const auto& data = batch[0].array();
- this->count = data->length - data->GetNullCount();
- if (is_boolean_type<ArrowType>::value) {
- this->sum += static_cast<typename SumType::c_type>(BooleanArray(data).true_count());
+ if (batch[0].is_array()) {
+ const auto& data = batch[0].array();
+ this->count = data->length - data->GetNullCount();
+ if (is_boolean_type<ArrowType>::value) {
+ this->sum +=
+ static_cast<typename SumType::c_type>(BooleanArray(data).true_count());
+ } else {
+ this->sum +=
+ arrow::compute::detail::SumArray<CType, typename SumType::c_type>(*data);
+ }
} else {
- this->sum +=
- arrow::compute::detail::SumArray<CType, typename SumType::c_type>(*data);
+ const auto& data = *batch[0].scalar();
+ this->count = data.is_valid;
+ if (data.is_valid) {
+ this->sum += internal::UnboxScalar<ArrowType>::Unbox(data);
Review comment:
Fixed (+extended the test), this is what I get for skimming over header files.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on pull request #10725: ARROW-9056: [C++] Support aggregations over scalars
Posted by GitBox <gi...@apache.org>.
lidavidm commented on pull request #10725:
URL: https://github.com/apache/arrow/pull/10725#issuecomment-881451121
Fixed the MSVC failure + rebased.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm closed pull request #10725: ARROW-9056: [C++] Support aggregations over scalars
Posted by GitBox <gi...@apache.org>.
lidavidm closed pull request #10725:
URL: https://github.com/apache/arrow/pull/10725
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on a change in pull request #10725: ARROW-9056: [C++] Support aggregations over scalars
Posted by GitBox <gi...@apache.org>.
bkietz commented on a change in pull request #10725:
URL: https://github.com/apache/arrow/pull/10725#discussion_r670689127
##########
File path: cpp/src/arrow/compute/kernels/aggregate_basic.cc
##########
@@ -59,10 +59,16 @@ struct CountImpl : public ScalarAggregator {
explicit CountImpl(ScalarAggregateOptions options) : options(std::move(options)) {}
Status Consume(KernelContext*, const ExecBatch& batch) override {
- const ArrayData& input = *batch[0].array();
- const int64_t nulls = input.GetNullCount();
- this->nulls += nulls;
- this->non_nulls += input.length - nulls;
+ if (batch[0].is_array()) {
+ const ArrayData& input = *batch[0].array();
+ const int64_t nulls = input.GetNullCount();
+ this->nulls += nulls;
+ this->non_nulls += input.length - nulls;
+ } else {
+ const Scalar& input = *batch[0].scalar();
+ this->nulls += !input.is_valid;
Review comment:
In dataset scans and elsewhere, a scalar can be used to encode a constant column. We should take the batch's length into account here
```suggestion
this->nulls += !input.is_valid * batch.length;
```
##########
File path: cpp/src/arrow/compute/kernels/aggregate_basic.cc
##########
@@ -395,6 +433,10 @@ void AddMinMaxKernels(KernelInit init,
auto out_ty = struct_({field("min", ty), field("max", ty)});
auto sig = KernelSignature::Make({InputType::Array(ty)}, ValueDescr::Scalar(out_ty));
AddAggKernel(std::move(sig), init, func, simd_level);
+
+ // scalar[InT] -> scalar[struct<min: T, max: T>]
+ sig = KernelSignature::Make({InputType::Scalar(ty)}, ValueDescr::Scalar(out_ty));
+ AddAggKernel(std::move(sig), init, func, SimdLevel::NONE);
Review comment:
Instead since the same init is being used for each kernel, I'd prefer we just make the signature inclusive of scalars as well as arrays:
```suggestion
auto sig = KernelSignature::Make({InputType(ty)}, ValueDescr::Scalar(out_ty));
AddAggKernel(std::move(sig), init, func, simd_level);
```
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org