You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "pitrou (via GitHub)" <gi...@apache.org> on 2025/03/26 17:00:34 UTC
[PR] GH-45732: [C++][Compute] Accept more pivot key types [arrow]
pitrou opened a new pull request, #45945:
URL: https://github.com/apache/arrow/pull/45945
Thanks for opening a pull request!
If this is your first pull request you can find detailed information on how to contribute here:
* [New Contributor's Guide](https://arrow.apache.org/docs/dev/developers/guide/step_by_step/pr_lifecycle.html#reviews-and-merge-of-the-pull-request)
* [Contributing Overview](https://arrow.apache.org/docs/dev/developers/overview.html)
Please remove this line and the above text before creating your pull request.
### Rationale for this change
### What changes are included in this PR?
### Are these changes tested?
### Are there any user-facing changes?
**This PR includes breaking changes to public APIs.** (If there are any breaking changes to public APIs, please explain which changes are breaking. If not, you can remove this.)
**This PR contains a "Critical Fix".** (If the changes fix either (a) a security vulnerability, (b) a bug that caused incorrect or invalid data to be produced, or (c) a bug that causes a crash (even when the API contract is upheld), please provide explanation. If not, you can remove this.)
--
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
Re: [PR] GH-45732: [C++][Compute] Accept more pivot key types [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #45945:
URL: https://github.com/apache/arrow/pull/45945#discussion_r2024290199
##########
docs/source/cpp/compute.rst:
##########
@@ -199,57 +199,57 @@ Aggregations
Scalar aggregations operate on a (chunked) array or scalar value and reduce
the input to a single output value.
-+--------------------+---------+------------------+------------------------+----------------------------------+-------+
-| Function name | Arity | Input types | Output type | Options class | Notes |
-+====================+=========+==================+========================+==================================+=======+
-| all | Unary | Boolean | Scalar Boolean | :struct:`ScalarAggregateOptions` | \(1) |
-+--------------------+---------+------------------+------------------------+----------------------------------+-------+
-| any | Unary | Boolean | Scalar Boolean | :struct:`ScalarAggregateOptions` | \(1) |
-+--------------------+---------+------------------+------------------------+----------------------------------+-------+
-| approximate_median | Unary | Numeric | Scalar Float64 | :struct:`ScalarAggregateOptions` | |
-+--------------------+---------+------------------+------------------------+----------------------------------+-------+
-| count | Unary | Any | Scalar Int64 | :struct:`CountOptions` | \(2) |
-+--------------------+---------+------------------+------------------------+----------------------------------+-------+
-| count_all | Nullary | | Scalar Int64 | | |
-+--------------------+---------+------------------+------------------------+----------------------------------+-------+
-| count_distinct | Unary | Non-nested types | Scalar Int64 | :struct:`CountOptions` | \(2) |
-+--------------------+---------+------------------+------------------------+----------------------------------+-------+
-| first | Unary | Numeric, Binary | Scalar Input type | :struct:`ScalarAggregateOptions` | \(3) |
-+--------------------+---------+------------------+------------------------+----------------------------------+-------+
-| first_last | Unary | Numeric, Binary | Scalar Struct | :struct:`ScalarAggregateOptions` | \(3) |
-+--------------------+---------+------------------+------------------------+----------------------------------+-------+
-| index | Unary | Any | Scalar Int64 | :struct:`IndexOptions` | \(4) |
-+--------------------+---------+------------------+------------------------+----------------------------------+-------+
-| kurtosis | Unary | Numeric | Scalar Float64 | :struct:`SkewOptions` | \(11) |
-+--------------------+---------+------------------+------------------------+----------------------------------+-------+
-| last | Unary | Numeric, Binary | Scalar Input type | :struct:`ScalarAggregateOptions` | \(3) |
-+--------------------+---------+------------------+------------------------+----------------------------------+-------+
-| max | Unary | Non-nested types | Scalar Input type | :struct:`ScalarAggregateOptions` | |
-+--------------------+---------+------------------+------------------------+----------------------------------+-------+
-| mean | Unary | Numeric | Scalar Decimal/Float64 | :struct:`ScalarAggregateOptions` | \(5) |
-+--------------------+---------+------------------+------------------------+----------------------------------+-------+
-| min | Unary | Non-nested types | Scalar Input type | :struct:`ScalarAggregateOptions` | |
-+--------------------+---------+------------------+------------------------+----------------------------------+-------+
-| min_max | Unary | Non-nested types | Scalar Struct | :struct:`ScalarAggregateOptions` | \(6) |
-+--------------------+---------+------------------+------------------------+----------------------------------+-------+
-| mode | Unary | Numeric | Struct | :struct:`ModeOptions` | \(7) |
-+--------------------+---------+------------------+------------------------+----------------------------------+-------+
-| pivot_wider | Binary | Binary, Any | Scalar Struct | :struct:`PivotWiderOptions` | \(8) |
-+--------------------+---------+------------------+------------------------+----------------------------------+-------+
-| product | Unary | Numeric | Scalar Numeric | :struct:`ScalarAggregateOptions` | \(9) |
-+--------------------+---------+------------------+------------------------+----------------------------------+-------+
-| quantile | Unary | Numeric | Scalar Numeric | :struct:`QuantileOptions` | \(10) |
-+--------------------+---------+------------------+------------------------+----------------------------------+-------+
-| skew | Unary | Numeric | Scalar Float64 | :struct:`SkewOptions` | \(11) |
-+--------------------+---------+------------------+------------------------+----------------------------------+-------+
-| stddev | Unary | Numeric | Scalar Float64 | :struct:`VarianceOptions` | \(11) |
-+--------------------+---------+------------------+------------------------+----------------------------------+-------+
-| sum | Unary | Numeric | Scalar Numeric | :struct:`ScalarAggregateOptions` | \(9) |
-+--------------------+---------+------------------+------------------------+----------------------------------+-------+
-| tdigest | Unary | Numeric | Float64 | :struct:`TDigestOptions` | \(12) |
-+--------------------+---------+------------------+------------------------+----------------------------------+-------+
-| variance | Unary | Numeric | Scalar Float64 | :struct:`VarianceOptions` | \(11) |
-+--------------------+---------+------------------+------------------------+----------------------------------+-------+
++--------------------+---------+-------------------------+------------------------+----------------------------------+-------+
+| Function name | Arity | Input types | Output type | Options class | Notes |
++====================+=========+=========================+========================+==================================+=======+
+| all | Unary | Boolean | Scalar Boolean | :struct:`ScalarAggregateOptions` | \(1) |
++--------------------+---------+-------------------------+------------------------+----------------------------------+-------+
+| any | Unary | Boolean | Scalar Boolean | :struct:`ScalarAggregateOptions` | \(1) |
++--------------------+---------+-------------------------+------------------------+----------------------------------+-------+
+| approximate_median | Unary | Numeric | Scalar Float64 | :struct:`ScalarAggregateOptions` | |
++--------------------+---------+-------------------------+------------------------+----------------------------------+-------+
+| count | Unary | Any | Scalar Int64 | :struct:`CountOptions` | \(2) |
++--------------------+---------+-------------------------+------------------------+----------------------------------+-------+
+| count_all | Nullary | | Scalar Int64 | | |
++--------------------+---------+-------------------------+------------------------+----------------------------------+-------+
+| count_distinct | Unary | Non-nested types | Scalar Int64 | :struct:`CountOptions` | \(2) |
++--------------------+---------+-------------------------+------------------------+----------------------------------+-------+
+| first | Unary | Numeric, Binary | Scalar Input type | :struct:`ScalarAggregateOptions` | \(3) |
++--------------------+---------+-------------------------+------------------------+----------------------------------+-------+
+| first_last | Unary | Numeric, Binary | Scalar Struct | :struct:`ScalarAggregateOptions` | \(3) |
++--------------------+---------+-------------------------+------------------------+----------------------------------+-------+
+| index | Unary | Any | Scalar Int64 | :struct:`IndexOptions` | \(4) |
++--------------------+---------+-------------------------+------------------------+----------------------------------+-------+
+| kurtosis | Unary | Numeric | Scalar Float64 | :struct:`SkewOptions` | \(11) |
++--------------------+---------+-------------------------+------------------------+----------------------------------+-------+
+| last | Unary | Numeric, Binary | Scalar Input type | :struct:`ScalarAggregateOptions` | \(3) |
++--------------------+---------+-------------------------+------------------------+----------------------------------+-------+
+| max | Unary | Non-nested types | Scalar Input type | :struct:`ScalarAggregateOptions` | |
++--------------------+---------+-------------------------+------------------------+----------------------------------+-------+
+| mean | Unary | Numeric | Scalar Decimal/Float64 | :struct:`ScalarAggregateOptions` | \(5) |
++--------------------+---------+-------------------------+------------------------+----------------------------------+-------+
+| min | Unary | Non-nested types | Scalar Input type | :struct:`ScalarAggregateOptions` | |
++--------------------+---------+-------------------------+------------------------+----------------------------------+-------+
+| min_max | Unary | Non-nested types | Scalar Struct | :struct:`ScalarAggregateOptions` | \(6) |
++--------------------+---------+-------------------------+------------------------+----------------------------------+-------+
+| mode | Unary | Numeric | Struct | :struct:`ModeOptions` | \(7) |
++--------------------+---------+-------------------------+------------------------+----------------------------------+-------+
+| pivot_wider | Binary | Binary or Integer, Any | Scalar Struct | :struct:`PivotWiderOptions` | \(8) |
Review Comment:
Yes, that sounds better indeed.
##########
cpp/src/arrow/compute/kernels/pivot_internal.cc:
##########
@@ -18,110 +18,139 @@
#include "arrow/compute/kernels/pivot_internal.h"
#include <cstdint>
+#include <string_view>
+#include <unordered_set>
+#include "arrow/array/array_primitive.h"
+#include "arrow/array/builder_binary.h"
+#include "arrow/compute/cast.h"
#include "arrow/compute/exec.h"
#include "arrow/compute/kernels/codegen_internal.h"
+#include "arrow/compute/row/grouper.h"
#include "arrow/scalar.h"
#include "arrow/type_traits.h"
+#include "arrow/util/bit_run_reader.h"
#include "arrow/util/checked_cast.h"
+#include "arrow/util/logging.h"
+#include "arrow/util/unreachable.h"
#include "arrow/visit_type_inline.h"
namespace arrow::compute::internal {
using ::arrow::util::span;
-struct BasePivotKeyMapper : public PivotWiderKeyMapper {
- Status Init(const PivotWiderOptions* options) override {
- if (options->key_names.size() > static_cast<size_t>(kMaxPivotKey) + 1) {
+struct ConcretePivotWiderKeyMapper : public PivotWiderKeyMapper {
+ Status Init(const DataType& key_type, const PivotWiderOptions* options,
+ ExecContext* ctx) {
+ if (options->key_names.size() > static_cast<size_t>(kMaxPivotKey)) {
return Status::NotImplemented("Pivoting to more than ",
- static_cast<size_t>(kMaxPivotKey) + 1,
- " columns: got ", options->key_names.size());
+ static_cast<size_t>(kMaxPivotKey), " columns: got ",
+ options->key_names.size());
}
- key_name_map_.reserve(options->key_names.size());
- PivotWiderKeyIndex index = 0;
- for (const auto& key_name : options->key_names) {
- bool inserted =
- key_name_map_.try_emplace(std::string_view(key_name), index++).second;
- if (!inserted) {
- return Status::KeyError("Duplicate key name '", key_name,
- "' in PivotWiderOptions");
+ unexpected_key_behavior_ = options->unexpected_key_behavior;
+ ARROW_ASSIGN_OR_RAISE(grouper_, Grouper::Make({&key_type}, ctx));
+ // Build a binary array of the pivot key values, and cast it to the desired key type
+ BinaryBuilder builder(ctx->memory_pool());
+ RETURN_NOT_OK(builder.Reserve(options->key_names.size()));
+ int64_t total_length = 0;
+ for (const auto& key : options->key_names) {
+ total_length += static_cast<int64_t>(key.length());
+ }
+ RETURN_NOT_OK(builder.ReserveData(total_length));
+ for (const auto& key : options->key_names) {
+ builder.UnsafeAppend(key);
+ }
+ ARROW_ASSIGN_OR_RAISE(auto binary_key_array, builder.Finish());
+ ARROW_ASSIGN_OR_RAISE(auto key_array,
+ Cast(*binary_key_array, &key_type, CastOptions::Safe(), ctx));
+ // Populate the grouper with the keys from the array
+ ExecSpan batch({ExecValue(*key_array->data())}, key_array->length());
+ RETURN_NOT_OK(grouper_->Populate(batch));
Review Comment:
It's there for a reason :)
--
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
Re: [PR] GH-45732: [C++][Compute] Accept more pivot key types [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #45945:
URL: https://github.com/apache/arrow/pull/45945#issuecomment-2755328405
Revision: af5faf7ae410c92c820b0b58a3bcbccf155228db
Submitted crossbow builds: [ursacomputing/crossbow @ actions-4a7680cc83](https://github.com/ursacomputing/crossbow/branches/all?query=actions-4a7680cc83)
|Task|Status|
|----|------|
|example-cpp-minimal-build-static|[](https://github.com/ursacomputing/crossbow/actions/runs/14090192624/job/39464660994)|
|example-cpp-minimal-build-static-system-dependency|[](https://github.com/ursacomputing/crossbow/actions/runs/14090193309/job/39464664102)|
|example-cpp-tutorial|[](https://github.com/ursacomputing/crossbow/actions/runs/14090193168/job/39464663118)|
|test-alpine-linux-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/14090192944/job/39464661836)|
|test-build-cpp-fuzz|[](https://github.com/ursacomputing/crossbow/actions/runs/14090193239/job/39464663683)|
|test-conda-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/14090193337/job/39464664343)|
|test-conda-cpp-meson|[](https://github.com/ursacomputing/crossbow/actions/runs/14090193250/job/39464663670)|
|test-conda-cpp-valgrind|[](https://github.com/ursacomputing/crossbow/actions/runs/14090192640/job/39464660963)|
|test-cuda-cpp-ubuntu-22.04-cuda-11.7.1|[](https://github.com/ursacomputing/crossbow/actions/runs/14090193284/job/39464663684)|
|test-debian-12-cpp-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/14090193630/job/39464665178)|
|test-debian-12-cpp-i386|[](https://github.com/ursacomputing/crossbow/actions/runs/14090193640/job/39464665253)|
|test-fedora-39-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/14090193035/job/39464662835)|
|test-ubuntu-22.04-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/14090193663/job/39464665399)|
|test-ubuntu-22.04-cpp-20|[](https://github.com/ursacomputing/crossbow/actions/runs/14090193153/job/39464663165)|
|test-ubuntu-22.04-cpp-bundled|[](https://github.com/ursacomputing/crossbow/actions/runs/14090192832/job/39464661808)|
|test-ubuntu-22.04-cpp-emscripten|[](https://github.com/ursacomputing/crossbow/actions/runs/14090192940/job/39464661838)|
|test-ubuntu-22.04-cpp-no-threading|[](https://github.com/ursacomputing/crossbow/actions/runs/14090193231/job/39464663285)|
|test-ubuntu-24.04-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/14090193381/job/39464664859)|
|test-ubuntu-24.04-cpp-bundled-offline|[](https://github.com/ursacomputing/crossbow/actions/runs/14090193476/job/39464664914)|
|test-ubuntu-24.04-cpp-gcc-13-bundled|[](https://github.com/ursacomputing/crossbow/actions/runs/14090193364/job/39464664455)|
|test-ubuntu-24.04-cpp-gcc-14|[](https://github.com/ursacomputing/crossbow/actions/runs/14090192960/job/39464662459)|
|test-ubuntu-24.04-cpp-minimal-with-formats|[](https://github.com/ursacomputing/crossbow/actions/runs/14090192959/job/39464661987)|
|test-ubuntu-24.04-cpp-thread-sanitizer|[](https://github.com/ursacomputing/crossbow/actions/runs/14090193241/job/39464663951)|
--
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
Re: [PR] GH-45732: [C++][Compute] Accept more pivot key types [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #45945:
URL: https://github.com/apache/arrow/pull/45945#discussion_r2021004395
##########
cpp/src/arrow/acero/hash_aggregate_test.cc:
##########
@@ -4749,6 +4773,21 @@ TEST_P(GroupBy, PivotDuplicateKeys) {
RunPivot(key_type, value_type, options, table_json));
}
+TEST_P(GroupBy, PivotInvalidKeys) {
+ // Integer key type, but key names cannot be converted to int
+ auto key_type = int32();
+ auto value_type = float32();
+ std::vector<std::string> table_json = {R"([])"};
+ PivotWiderOptions options(/*key_names=*/{"height", "width"});
+ EXPECT_RAISES_WITH_MESSAGE_THAT(
+ Invalid, HasSubstr("Failed to parse string: 'width' as a scalar of type int32"),
Review Comment:
Hmm, you're right, I should make this test less implementation-dependent.
--
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
Re: [PR] GH-45732: [C++][Compute] Accept more pivot key types [arrow]
Posted by "kszucs (via GitHub)" <gi...@apache.org>.
kszucs commented on PR #45945:
URL: https://github.com/apache/arrow/pull/45945#issuecomment-2765664670
> By the way, this PR (the move to a `Grouper`-based pivot implementation) results in a ~15% decrease in `hash_pivot_wider` performance in the simplistic benchmarks I posted in #45741 .
I think consolidating the implementations is more important, especially if we can optimize the reused components later on.
--
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
Re: [PR] GH-45732: [C++][Compute] Accept more pivot key types [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #45945:
URL: https://github.com/apache/arrow/pull/45945#issuecomment-2755174360
Revision: 1b737964a6d00817370e389064cbfb70b42c5280
Submitted crossbow builds: [ursacomputing/crossbow @ actions-5ef3d1dda9](https://github.com/ursacomputing/crossbow/branches/all?query=actions-5ef3d1dda9)
|Task|Status|
|----|------|
|example-cpp-minimal-build-static|[](https://github.com/ursacomputing/crossbow/actions/runs/14089582195/job/39462601343)|
|example-cpp-minimal-build-static-system-dependency|[](https://github.com/ursacomputing/crossbow/actions/runs/14089581991/job/39462600711)|
|example-cpp-tutorial|[](https://github.com/ursacomputing/crossbow/actions/runs/14089581938/job/39462600146)|
|test-alpine-linux-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/14089582014/job/39462600674)|
|test-build-cpp-fuzz|[](https://github.com/ursacomputing/crossbow/actions/runs/14089581287/job/39462595998)|
|test-conda-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/14089581736/job/39462598887)|
|test-conda-cpp-meson|[](https://github.com/ursacomputing/crossbow/actions/runs/14089581464/job/39462597080)|
|test-conda-cpp-valgrind|[](https://github.com/ursacomputing/crossbow/actions/runs/14089582016/job/39462600635)|
|test-cuda-cpp-ubuntu-22.04-cuda-11.7.1|[](https://github.com/ursacomputing/crossbow/actions/runs/14089581885/job/39462599145)|
|test-debian-12-cpp-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/14089581313/job/39462596060)|
|test-debian-12-cpp-i386|[](https://github.com/ursacomputing/crossbow/actions/runs/14089581285/job/39462596351)|
|test-fedora-39-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/14089581589/job/39462598004)|
|test-ubuntu-22.04-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/14089581937/job/39462600091)|
|test-ubuntu-22.04-cpp-20|[](https://github.com/ursacomputing/crossbow/actions/runs/14089581448/job/39462597113)|
|test-ubuntu-22.04-cpp-bundled|[](https://github.com/ursacomputing/crossbow/actions/runs/14089582217/job/39462601372)|
|test-ubuntu-22.04-cpp-emscripten|[](https://github.com/ursacomputing/crossbow/actions/runs/14089581827/job/39462598901)|
|test-ubuntu-22.04-cpp-no-threading|[](https://github.com/ursacomputing/crossbow/actions/runs/14089581358/job/39462596169)|
|test-ubuntu-24.04-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/14089581495/job/39462597633)|
|test-ubuntu-24.04-cpp-bundled-offline|[](https://github.com/ursacomputing/crossbow/actions/runs/14089581645/job/39462598043)|
|test-ubuntu-24.04-cpp-gcc-13-bundled|[](https://github.com/ursacomputing/crossbow/actions/runs/14089581408/job/39462596088)|
|test-ubuntu-24.04-cpp-gcc-14|[](https://github.com/ursacomputing/crossbow/actions/runs/14089581576/job/39462598793)|
|test-ubuntu-24.04-cpp-minimal-with-formats|[](https://github.com/ursacomputing/crossbow/actions/runs/14089581560/job/39462598101)|
|test-ubuntu-24.04-cpp-thread-sanitizer|[](https://github.com/ursacomputing/crossbow/actions/runs/14089581739/job/39462598909)|
--
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
Re: [PR] GH-45732: [C++][Compute] Accept more pivot key types [arrow]
Posted by "zanmato1984 (via GitHub)" <gi...@apache.org>.
zanmato1984 commented on PR #45945:
URL: https://github.com/apache/arrow/pull/45945#issuecomment-2756661985
Sorry I have to defer the reviewing until later next week. @pitrou would you wait for a while? Thank you.
--
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
Re: [PR] GH-45732: [C++][Compute] Accept more pivot key types [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #45945:
URL: https://github.com/apache/arrow/pull/45945#issuecomment-2755168432
@github-actions crossbow submit -g cpp
--
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
Re: [PR] GH-45732: [C++][Compute] Accept more pivot key types [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #45945:
URL: https://github.com/apache/arrow/pull/45945#issuecomment-2755321735
@github-actions crossbow submit -g cpp
--
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
Re: [PR] GH-45732: [C++][Compute] Accept more pivot key types [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #45945:
URL: https://github.com/apache/arrow/pull/45945#issuecomment-2757282929
Oh, yes, definitely.
--
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
Re: [PR] GH-45732: [C++][Compute] Accept more pivot key types [arrow]
Posted by "kszucs (via GitHub)" <gi...@apache.org>.
kszucs commented on code in PR #45945:
URL: https://github.com/apache/arrow/pull/45945#discussion_r2020672383
##########
cpp/src/arrow/acero/hash_aggregate_test.cc:
##########
@@ -4749,6 +4773,21 @@ TEST_P(GroupBy, PivotDuplicateKeys) {
RunPivot(key_type, value_type, options, table_json));
}
+TEST_P(GroupBy, PivotInvalidKeys) {
+ // Integer key type, but key names cannot be converted to int
+ auto key_type = int32();
+ auto value_type = float32();
+ std::vector<std::string> table_json = {R"([])"};
+ PivotWiderOptions options(/*key_names=*/{"height", "width"});
+ EXPECT_RAISES_WITH_MESSAGE_THAT(
+ Invalid, HasSubstr("Failed to parse string: 'width' as a scalar of type int32"),
Review Comment:
Picking the last value for the error message is specific to the `Cast` implementation?
--
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
Re: [PR] GH-45732: [C++][Compute] Accept more pivot key types [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou merged PR #45945:
URL: https://github.com/apache/arrow/pull/45945
--
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
Re: [PR] GH-45732: [C++][Compute] Accept more pivot key types [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #45945:
URL: https://github.com/apache/arrow/pull/45945#issuecomment-2758516264
Revision: af5faf7ae410c92c820b0b58a3bcbccf155228db
Submitted crossbow builds: [ursacomputing/crossbow @ actions-93306ba1c2](https://github.com/ursacomputing/crossbow/branches/all?query=actions-93306ba1c2)
|Task|Status|
|----|------|
|example-cpp-minimal-build-static|[](https://github.com/ursacomputing/crossbow/actions/runs/14110727206/job/39528433418)|
|example-cpp-minimal-build-static-system-dependency|[](https://github.com/ursacomputing/crossbow/actions/runs/14110727802/job/39528436735)|
|example-cpp-tutorial|[](https://github.com/ursacomputing/crossbow/actions/runs/14110727471/job/39528434025)|
|test-alpine-linux-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/14110727528/job/39528435511)|
|test-build-cpp-fuzz|[](https://github.com/ursacomputing/crossbow/actions/runs/14110727371/job/39528434148)|
|test-conda-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/14110727208/job/39528433436)|
|test-conda-cpp-meson|[](https://github.com/ursacomputing/crossbow/actions/runs/14110728102/job/39528437356)|
|test-conda-cpp-valgrind|[](https://github.com/ursacomputing/crossbow/actions/runs/14110727918/job/39528436589)|
|test-cuda-cpp-ubuntu-22.04-cuda-11.7.1|[](https://github.com/ursacomputing/crossbow/actions/runs/14110727279/job/39528434169)|
|test-debian-12-cpp-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/14110727478/job/39528434514)|
|test-debian-12-cpp-i386|[](https://github.com/ursacomputing/crossbow/actions/runs/14110728258/job/39528437607)|
|test-fedora-39-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/14110727013/job/39528432420)|
|test-ubuntu-22.04-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/14110728207/job/39528437470)|
|test-ubuntu-22.04-cpp-20|[](https://github.com/ursacomputing/crossbow/actions/runs/14110727064/job/39528432383)|
|test-ubuntu-22.04-cpp-bundled|[](https://github.com/ursacomputing/crossbow/actions/runs/14110727599/job/39528436029)|
|test-ubuntu-22.04-cpp-emscripten|[](https://github.com/ursacomputing/crossbow/actions/runs/14110727373/job/39528434988)|
|test-ubuntu-22.04-cpp-no-threading|[](https://github.com/ursacomputing/crossbow/actions/runs/14110727568/job/39528435573)|
|test-ubuntu-24.04-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/14110727512/job/39528434120)|
|test-ubuntu-24.04-cpp-bundled-offline|[](https://github.com/ursacomputing/crossbow/actions/runs/14110727809/job/39528436531)|
|test-ubuntu-24.04-cpp-gcc-13-bundled|[](https://github.com/ursacomputing/crossbow/actions/runs/14110727125/job/39528432453)|
|test-ubuntu-24.04-cpp-gcc-14|[](https://github.com/ursacomputing/crossbow/actions/runs/14110727417/job/39528435068)|
|test-ubuntu-24.04-cpp-minimal-with-formats|[](https://github.com/ursacomputing/crossbow/actions/runs/14110727763/job/39528436429)|
|test-ubuntu-24.04-cpp-thread-sanitizer|[](https://github.com/ursacomputing/crossbow/actions/runs/14110727028/job/39528432477)|
--
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
Re: [PR] GH-45732: [C++][Compute] Accept more pivot key types [arrow]
Posted by "kszucs (via GitHub)" <gi...@apache.org>.
kszucs commented on code in PR #45945:
URL: https://github.com/apache/arrow/pull/45945#discussion_r2020612366
##########
cpp/src/arrow/compute/kernels/pivot_internal.cc:
##########
@@ -18,110 +18,137 @@
#include "arrow/compute/kernels/pivot_internal.h"
#include <cstdint>
+#include <string_view>
+#include <unordered_set>
+#include "arrow/array/array_primitive.h"
+#include "arrow/array/builder_binary.h"
+#include "arrow/compute/cast.h"
#include "arrow/compute/exec.h"
#include "arrow/compute/kernels/codegen_internal.h"
+#include "arrow/compute/row/grouper.h"
#include "arrow/scalar.h"
#include "arrow/type_traits.h"
+#include "arrow/util/bit_run_reader.h"
#include "arrow/util/checked_cast.h"
+#include "arrow/util/logging.h"
+#include "arrow/util/unreachable.h"
#include "arrow/visit_type_inline.h"
namespace arrow::compute::internal {
using ::arrow::util::span;
-struct BasePivotKeyMapper : public PivotWiderKeyMapper {
- Status Init(const PivotWiderOptions* options) override {
- if (options->key_names.size() > static_cast<size_t>(kMaxPivotKey) + 1) {
+struct ConcretePivotWiderKeyMapper : public PivotWiderKeyMapper {
+ Status Init(const DataType& key_type, const PivotWiderOptions* options,
+ ExecContext* ctx) {
+ if (options->key_names.size() > static_cast<size_t>(kMaxPivotKey)) {
return Status::NotImplemented("Pivoting to more than ",
- static_cast<size_t>(kMaxPivotKey) + 1,
- " columns: got ", options->key_names.size());
+ static_cast<size_t>(kMaxPivotKey), " columns: got ",
+ options->key_names.size());
}
- key_name_map_.reserve(options->key_names.size());
- PivotWiderKeyIndex index = 0;
- for (const auto& key_name : options->key_names) {
- bool inserted =
- key_name_map_.try_emplace(std::string_view(key_name), index++).second;
- if (!inserted) {
- return Status::KeyError("Duplicate key name '", key_name,
- "' in PivotWiderOptions");
+ unexpected_key_behavior_ = options->unexpected_key_behavior;
+ ARROW_ASSIGN_OR_RAISE(grouper_, Grouper::Make({&key_type}, ctx));
+ // Build a binary array of the pivot key values, and cast it to the desired key type
+ BinaryBuilder builder(ctx->memory_pool());
+ RETURN_NOT_OK(builder.Reserve(options->key_names.size()));
+ int64_t total_length = 0;
+ for (const auto& key : options->key_names) {
+ total_length += static_cast<int64_t>(key.length());
+ }
+ RETURN_NOT_OK(builder.ReserveData(total_length));
+ for (const auto& key : options->key_names) {
+ builder.UnsafeAppend(key);
+ }
+ ARROW_ASSIGN_OR_RAISE(auto binary_key_array, builder.Finish());
+ ARROW_ASSIGN_OR_RAISE(auto key_array,
+ Cast(*binary_key_array, &key_type, CastOptions::Safe(), ctx));
+ // Populate the grouper with the keys from the array
+ ExecSpan batch({ExecValue(*key_array->data())}, key_array->length());
+ RETURN_NOT_OK(grouper_->Populate(batch));
+ if (grouper_->num_groups() != options->key_names.size()) {
+ // There's a duplicate key, find it to emit a nicer error message
+ std::unordered_set<std::string_view> seen;
+ for (const auto& key : options->key_names) {
+ auto [_, inserted] = seen.emplace(key);
+ if (!inserted) {
+ return Status::KeyError("Duplicate key name '", key, "' in PivotWiderOptions");
+ }
}
+ Unreachable("Grouper doesn't agree with std::unordered_set");
}
- unexpected_key_behavior_ = options->unexpected_key_behavior;
return Status::OK();
}
- protected:
- Result<PivotWiderKeyIndex> KeyNotFound(std::string_view key_name) {
- if (unexpected_key_behavior_ == PivotWiderOptions::kIgnore) {
- return kNullPivotKey;
+ Result<std::shared_ptr<ArrayData>> MapKeys(const ArraySpan& array) override {
+ if (array.GetNullCount() != 0) {
+ return NullKeyName();
}
- DCHECK_EQ(unexpected_key_behavior_, PivotWiderOptions::kRaise);
- return Status::KeyError("Unexpected pivot key: ", key_name);
+ return MapKeysInternal(array, array.length);
}
- Result<PivotWiderKeyIndex> LookupKey(std::string_view key_name) {
- const auto it = this->key_name_map_.find(key_name);
- if (ARROW_PREDICT_FALSE(it == this->key_name_map_.end())) {
- return KeyNotFound(key_name);
+ Result<std::optional<PivotWiderKeyIndex>> MapKey(const Scalar& scalar) override {
+ if (!scalar.is_valid) {
+ return NullKeyName();
+ }
+ ARROW_ASSIGN_OR_RAISE(auto group_id_array, MapKeysInternal(&scalar, /*length=*/1));
+ DCHECK_EQ(group_id_array->length, 1);
+ if (group_id_array->GetNullCount() == 0) {
+ return group_id_array->GetValues<uint32_t>(1)[0];
} else {
- return it->second;
+ return std::nullopt;
+ }
+ }
+
+ protected:
+ Result<std::shared_ptr<ArrayData>> MapKeysInternal(const ExecValue& values,
+ int64_t length) {
+ ARROW_ASSIGN_OR_RAISE(auto result, grouper_->Lookup(ExecSpan({values}, length)));
+ DCHECK(result.is_array());
+ DCHECK_EQ(result.type()->id(), Type::UINT32);
+ auto group_id_array = result.array();
+ if (ARROW_PREDICT_FALSE(group_id_array->GetNullCount() != 0) &&
Review Comment:
Maybe extract `group_id_array->GetNullCount() != 0` as `has_nulls` variable for better readability? `ARROW_PREDICT_FALSE(.. != 0)` is slightly harder to interpret.
--
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
Re: [PR] GH-45732: [C++][Compute] Accept more pivot key types [arrow]
Posted by "zanmato1984 (via GitHub)" <gi...@apache.org>.
zanmato1984 commented on code in PR #45945:
URL: https://github.com/apache/arrow/pull/45945#discussion_r2022321506
##########
docs/source/cpp/compute.rst:
##########
@@ -199,57 +199,57 @@ Aggregations
Scalar aggregations operate on a (chunked) array or scalar value and reduce
the input to a single output value.
-+--------------------+---------+------------------+------------------------+----------------------------------+-------+
-| Function name | Arity | Input types | Output type | Options class | Notes |
-+====================+=========+==================+========================+==================================+=======+
-| all | Unary | Boolean | Scalar Boolean | :struct:`ScalarAggregateOptions` | \(1) |
-+--------------------+---------+------------------+------------------------+----------------------------------+-------+
-| any | Unary | Boolean | Scalar Boolean | :struct:`ScalarAggregateOptions` | \(1) |
-+--------------------+---------+------------------+------------------------+----------------------------------+-------+
-| approximate_median | Unary | Numeric | Scalar Float64 | :struct:`ScalarAggregateOptions` | |
-+--------------------+---------+------------------+------------------------+----------------------------------+-------+
-| count | Unary | Any | Scalar Int64 | :struct:`CountOptions` | \(2) |
-+--------------------+---------+------------------+------------------------+----------------------------------+-------+
-| count_all | Nullary | | Scalar Int64 | | |
-+--------------------+---------+------------------+------------------------+----------------------------------+-------+
-| count_distinct | Unary | Non-nested types | Scalar Int64 | :struct:`CountOptions` | \(2) |
-+--------------------+---------+------------------+------------------------+----------------------------------+-------+
-| first | Unary | Numeric, Binary | Scalar Input type | :struct:`ScalarAggregateOptions` | \(3) |
-+--------------------+---------+------------------+------------------------+----------------------------------+-------+
-| first_last | Unary | Numeric, Binary | Scalar Struct | :struct:`ScalarAggregateOptions` | \(3) |
-+--------------------+---------+------------------+------------------------+----------------------------------+-------+
-| index | Unary | Any | Scalar Int64 | :struct:`IndexOptions` | \(4) |
-+--------------------+---------+------------------+------------------------+----------------------------------+-------+
-| kurtosis | Unary | Numeric | Scalar Float64 | :struct:`SkewOptions` | \(11) |
-+--------------------+---------+------------------+------------------------+----------------------------------+-------+
-| last | Unary | Numeric, Binary | Scalar Input type | :struct:`ScalarAggregateOptions` | \(3) |
-+--------------------+---------+------------------+------------------------+----------------------------------+-------+
-| max | Unary | Non-nested types | Scalar Input type | :struct:`ScalarAggregateOptions` | |
-+--------------------+---------+------------------+------------------------+----------------------------------+-------+
-| mean | Unary | Numeric | Scalar Decimal/Float64 | :struct:`ScalarAggregateOptions` | \(5) |
-+--------------------+---------+------------------+------------------------+----------------------------------+-------+
-| min | Unary | Non-nested types | Scalar Input type | :struct:`ScalarAggregateOptions` | |
-+--------------------+---------+------------------+------------------------+----------------------------------+-------+
-| min_max | Unary | Non-nested types | Scalar Struct | :struct:`ScalarAggregateOptions` | \(6) |
-+--------------------+---------+------------------+------------------------+----------------------------------+-------+
-| mode | Unary | Numeric | Struct | :struct:`ModeOptions` | \(7) |
-+--------------------+---------+------------------+------------------------+----------------------------------+-------+
-| pivot_wider | Binary | Binary, Any | Scalar Struct | :struct:`PivotWiderOptions` | \(8) |
-+--------------------+---------+------------------+------------------------+----------------------------------+-------+
-| product | Unary | Numeric | Scalar Numeric | :struct:`ScalarAggregateOptions` | \(9) |
-+--------------------+---------+------------------+------------------------+----------------------------------+-------+
-| quantile | Unary | Numeric | Scalar Numeric | :struct:`QuantileOptions` | \(10) |
-+--------------------+---------+------------------+------------------------+----------------------------------+-------+
-| skew | Unary | Numeric | Scalar Float64 | :struct:`SkewOptions` | \(11) |
-+--------------------+---------+------------------+------------------------+----------------------------------+-------+
-| stddev | Unary | Numeric | Scalar Float64 | :struct:`VarianceOptions` | \(11) |
-+--------------------+---------+------------------+------------------------+----------------------------------+-------+
-| sum | Unary | Numeric | Scalar Numeric | :struct:`ScalarAggregateOptions` | \(9) |
-+--------------------+---------+------------------+------------------------+----------------------------------+-------+
-| tdigest | Unary | Numeric | Float64 | :struct:`TDigestOptions` | \(12) |
-+--------------------+---------+------------------+------------------------+----------------------------------+-------+
-| variance | Unary | Numeric | Scalar Float64 | :struct:`VarianceOptions` | \(11) |
-+--------------------+---------+------------------+------------------------+----------------------------------+-------+
++--------------------+---------+-------------------------+------------------------+----------------------------------+-------+
+| Function name | Arity | Input types | Output type | Options class | Notes |
++====================+=========+=========================+========================+==================================+=======+
+| all | Unary | Boolean | Scalar Boolean | :struct:`ScalarAggregateOptions` | \(1) |
++--------------------+---------+-------------------------+------------------------+----------------------------------+-------+
+| any | Unary | Boolean | Scalar Boolean | :struct:`ScalarAggregateOptions` | \(1) |
++--------------------+---------+-------------------------+------------------------+----------------------------------+-------+
+| approximate_median | Unary | Numeric | Scalar Float64 | :struct:`ScalarAggregateOptions` | |
++--------------------+---------+-------------------------+------------------------+----------------------------------+-------+
+| count | Unary | Any | Scalar Int64 | :struct:`CountOptions` | \(2) |
++--------------------+---------+-------------------------+------------------------+----------------------------------+-------+
+| count_all | Nullary | | Scalar Int64 | | |
++--------------------+---------+-------------------------+------------------------+----------------------------------+-------+
+| count_distinct | Unary | Non-nested types | Scalar Int64 | :struct:`CountOptions` | \(2) |
++--------------------+---------+-------------------------+------------------------+----------------------------------+-------+
+| first | Unary | Numeric, Binary | Scalar Input type | :struct:`ScalarAggregateOptions` | \(3) |
++--------------------+---------+-------------------------+------------------------+----------------------------------+-------+
+| first_last | Unary | Numeric, Binary | Scalar Struct | :struct:`ScalarAggregateOptions` | \(3) |
++--------------------+---------+-------------------------+------------------------+----------------------------------+-------+
+| index | Unary | Any | Scalar Int64 | :struct:`IndexOptions` | \(4) |
++--------------------+---------+-------------------------+------------------------+----------------------------------+-------+
+| kurtosis | Unary | Numeric | Scalar Float64 | :struct:`SkewOptions` | \(11) |
++--------------------+---------+-------------------------+------------------------+----------------------------------+-------+
+| last | Unary | Numeric, Binary | Scalar Input type | :struct:`ScalarAggregateOptions` | \(3) |
++--------------------+---------+-------------------------+------------------------+----------------------------------+-------+
+| max | Unary | Non-nested types | Scalar Input type | :struct:`ScalarAggregateOptions` | |
++--------------------+---------+-------------------------+------------------------+----------------------------------+-------+
+| mean | Unary | Numeric | Scalar Decimal/Float64 | :struct:`ScalarAggregateOptions` | \(5) |
++--------------------+---------+-------------------------+------------------------+----------------------------------+-------+
+| min | Unary | Non-nested types | Scalar Input type | :struct:`ScalarAggregateOptions` | |
++--------------------+---------+-------------------------+------------------------+----------------------------------+-------+
+| min_max | Unary | Non-nested types | Scalar Struct | :struct:`ScalarAggregateOptions` | \(6) |
++--------------------+---------+-------------------------+------------------------+----------------------------------+-------+
+| mode | Unary | Numeric | Struct | :struct:`ModeOptions` | \(7) |
++--------------------+---------+-------------------------+------------------------+----------------------------------+-------+
+| pivot_wider | Binary | Binary or Integer, Any | Scalar Struct | :struct:`PivotWiderOptions` | \(8) |
Review Comment:
This feels weird (why an argument type can be `Binary` or `Integer` and `Any`), until I realized it is for each of the two arguments.
I see for non-unary functions we tend to specify the types for each individual argument, like: `Binary/String (Arg 0); Integral (Arg 1)`. Maybe we should follow this convention.
##########
cpp/src/arrow/compute/kernels/pivot_internal.cc:
##########
@@ -18,110 +18,139 @@
#include "arrow/compute/kernels/pivot_internal.h"
#include <cstdint>
+#include <string_view>
+#include <unordered_set>
+#include "arrow/array/array_primitive.h"
+#include "arrow/array/builder_binary.h"
+#include "arrow/compute/cast.h"
#include "arrow/compute/exec.h"
#include "arrow/compute/kernels/codegen_internal.h"
+#include "arrow/compute/row/grouper.h"
#include "arrow/scalar.h"
#include "arrow/type_traits.h"
+#include "arrow/util/bit_run_reader.h"
#include "arrow/util/checked_cast.h"
+#include "arrow/util/logging.h"
+#include "arrow/util/unreachable.h"
#include "arrow/visit_type_inline.h"
namespace arrow::compute::internal {
using ::arrow::util::span;
-struct BasePivotKeyMapper : public PivotWiderKeyMapper {
- Status Init(const PivotWiderOptions* options) override {
- if (options->key_names.size() > static_cast<size_t>(kMaxPivotKey) + 1) {
+struct ConcretePivotWiderKeyMapper : public PivotWiderKeyMapper {
+ Status Init(const DataType& key_type, const PivotWiderOptions* options,
+ ExecContext* ctx) {
+ if (options->key_names.size() > static_cast<size_t>(kMaxPivotKey)) {
return Status::NotImplemented("Pivoting to more than ",
- static_cast<size_t>(kMaxPivotKey) + 1,
- " columns: got ", options->key_names.size());
+ static_cast<size_t>(kMaxPivotKey), " columns: got ",
+ options->key_names.size());
}
- key_name_map_.reserve(options->key_names.size());
- PivotWiderKeyIndex index = 0;
- for (const auto& key_name : options->key_names) {
- bool inserted =
- key_name_map_.try_emplace(std::string_view(key_name), index++).second;
- if (!inserted) {
- return Status::KeyError("Duplicate key name '", key_name,
- "' in PivotWiderOptions");
+ unexpected_key_behavior_ = options->unexpected_key_behavior;
+ ARROW_ASSIGN_OR_RAISE(grouper_, Grouper::Make({&key_type}, ctx));
+ // Build a binary array of the pivot key values, and cast it to the desired key type
+ BinaryBuilder builder(ctx->memory_pool());
+ RETURN_NOT_OK(builder.Reserve(options->key_names.size()));
+ int64_t total_length = 0;
+ for (const auto& key : options->key_names) {
+ total_length += static_cast<int64_t>(key.length());
+ }
+ RETURN_NOT_OK(builder.ReserveData(total_length));
+ for (const auto& key : options->key_names) {
+ builder.UnsafeAppend(key);
+ }
+ ARROW_ASSIGN_OR_RAISE(auto binary_key_array, builder.Finish());
+ ARROW_ASSIGN_OR_RAISE(auto key_array,
+ Cast(*binary_key_array, &key_type, CastOptions::Safe(), ctx));
+ // Populate the grouper with the keys from the array
+ ExecSpan batch({ExecValue(*key_array->data())}, key_array->length());
+ RETURN_NOT_OK(grouper_->Populate(batch));
Review Comment:
Aha, nice use of the enriched interface of `Grouper`.
##########
cpp/src/arrow/compute/kernels/aggregate_pivot.cc:
##########
@@ -42,47 +44,56 @@ struct PivotImpl : public ScalarAggregator {
values_.push_back(MakeNullScalar(value_type));
}
out_type_ = struct_(std::move(fields));
- ARROW_ASSIGN_OR_RAISE(key_mapper_, PivotWiderKeyMapper::Make(*key_type_, options_));
+ ARROW_ASSIGN_OR_RAISE(key_mapper_,
+ PivotWiderKeyMapper::Make(*key_type_, options_, ctx));
return Status::OK();
}
Status Consume(KernelContext*, const ExecSpan& batch) override {
DCHECK_EQ(batch.num_values(), 2);
if (batch[0].is_array()) {
- ARROW_ASSIGN_OR_RAISE(span<const PivotWiderKeyIndex> keys,
- key_mapper_->MapKeys(batch[0].array));
+ ARROW_ASSIGN_OR_RAISE(auto keys_array, key_mapper_->MapKeys(batch[0].array));
Review Comment:
Shall we assert the `keys_array`'s data type?
##########
cpp/src/arrow/compute/api_aggregate.h:
##########
@@ -202,9 +202,10 @@ class ARROW_EXPORT TDigestOptions : public FunctionOptions {
/// - The corresponding `Aggregate::target` must have two FieldRef elements;
/// the first one points to the pivot key column, the second points to the
/// pivoted data column.
-/// - The pivot key column must be string-like; its values will be matched
-/// against `key_names` in order to dispatch the pivoted data into the
-/// output.
+/// - The pivot key column can be string-like or integer; its values will be
Review Comment:
Make it identical to the description in function doc.
##########
cpp/src/arrow/compute/api_aggregate.h:
##########
@@ -202,9 +202,10 @@ class ARROW_EXPORT TDigestOptions : public FunctionOptions {
/// - The corresponding `Aggregate::target` must have two FieldRef elements;
/// the first one points to the pivot key column, the second points to the
/// pivoted data column.
-/// - The pivot key column must be string-like; its values will be matched
-/// against `key_names` in order to dispatch the pivoted data into the
-/// output.
+/// - The pivot key column can be string-like or integer; its values will be
Review Comment:
```suggestion
/// - The pivot key column can be string, binary or integer; its values will be
```
##########
cpp/src/arrow/compute/kernels/pivot_internal.h:
##########
@@ -20,32 +20,29 @@
#include <cstdint>
#include <limits>
#include <memory>
+#include <optional>
#include "arrow/compute/api_aggregate.h"
#include "arrow/compute/type_fwd.h"
#include "arrow/result.h"
#include "arrow/status.h"
#include "arrow/type_fwd.h"
-#include "arrow/util/span.h"
namespace arrow::compute::internal {
-using PivotWiderKeyIndex = uint8_t;
+using PivotWiderKeyIndex = uint32_t;
-constexpr PivotWiderKeyIndex kNullPivotKey =
+constexpr PivotWiderKeyIndex kMaxPivotKey =
std::numeric_limits<PivotWiderKeyIndex>::max();
-constexpr PivotWiderKeyIndex kMaxPivotKey = kNullPivotKey - 1;
struct PivotWiderKeyMapper {
virtual ~PivotWiderKeyMapper() = default;
- virtual Status Init(const PivotWiderOptions* options) = 0;
- virtual Result<::arrow::util::span<const PivotWiderKeyIndex>> MapKeys(
- const ArraySpan&) = 0;
- virtual Result<PivotWiderKeyIndex> MapKey(const Scalar&) = 0;
+ virtual Result<std::shared_ptr<ArrayData>> MapKeys(const ArraySpan&) = 0;
+ virtual Result<std::optional<PivotWiderKeyIndex>> MapKey(const Scalar&) = 0;
static Result<std::unique_ptr<PivotWiderKeyMapper>> Make(
- const DataType& key_type, const PivotWiderOptions* options);
+ const DataType& key_type, const PivotWiderOptions* options, ExecContext*);
Review Comment:
Maybe we should name the third argument just like the other two?
--
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
Re: [PR] GH-45732: [C++][Compute] Accept more pivot key types [arrow]
Posted by "kszucs (via GitHub)" <gi...@apache.org>.
kszucs commented on code in PR #45945:
URL: https://github.com/apache/arrow/pull/45945#discussion_r2020650540
##########
cpp/src/arrow/compute/kernels/pivot_internal.cc:
##########
@@ -18,110 +18,137 @@
#include "arrow/compute/kernels/pivot_internal.h"
#include <cstdint>
+#include <string_view>
+#include <unordered_set>
+#include "arrow/array/array_primitive.h"
+#include "arrow/array/builder_binary.h"
+#include "arrow/compute/cast.h"
#include "arrow/compute/exec.h"
#include "arrow/compute/kernels/codegen_internal.h"
+#include "arrow/compute/row/grouper.h"
#include "arrow/scalar.h"
#include "arrow/type_traits.h"
+#include "arrow/util/bit_run_reader.h"
#include "arrow/util/checked_cast.h"
+#include "arrow/util/logging.h"
+#include "arrow/util/unreachable.h"
#include "arrow/visit_type_inline.h"
namespace arrow::compute::internal {
using ::arrow::util::span;
-struct BasePivotKeyMapper : public PivotWiderKeyMapper {
- Status Init(const PivotWiderOptions* options) override {
- if (options->key_names.size() > static_cast<size_t>(kMaxPivotKey) + 1) {
+struct ConcretePivotWiderKeyMapper : public PivotWiderKeyMapper {
+ Status Init(const DataType& key_type, const PivotWiderOptions* options,
+ ExecContext* ctx) {
+ if (options->key_names.size() > static_cast<size_t>(kMaxPivotKey)) {
return Status::NotImplemented("Pivoting to more than ",
- static_cast<size_t>(kMaxPivotKey) + 1,
- " columns: got ", options->key_names.size());
+ static_cast<size_t>(kMaxPivotKey), " columns: got ",
+ options->key_names.size());
}
- key_name_map_.reserve(options->key_names.size());
- PivotWiderKeyIndex index = 0;
- for (const auto& key_name : options->key_names) {
- bool inserted =
- key_name_map_.try_emplace(std::string_view(key_name), index++).second;
- if (!inserted) {
- return Status::KeyError("Duplicate key name '", key_name,
- "' in PivotWiderOptions");
+ unexpected_key_behavior_ = options->unexpected_key_behavior;
+ ARROW_ASSIGN_OR_RAISE(grouper_, Grouper::Make({&key_type}, ctx));
+ // Build a binary array of the pivot key values, and cast it to the desired key type
+ BinaryBuilder builder(ctx->memory_pool());
+ RETURN_NOT_OK(builder.Reserve(options->key_names.size()));
+ int64_t total_length = 0;
+ for (const auto& key : options->key_names) {
+ total_length += static_cast<int64_t>(key.length());
+ }
+ RETURN_NOT_OK(builder.ReserveData(total_length));
+ for (const auto& key : options->key_names) {
+ builder.UnsafeAppend(key);
+ }
+ ARROW_ASSIGN_OR_RAISE(auto binary_key_array, builder.Finish());
+ ARROW_ASSIGN_OR_RAISE(auto key_array,
+ Cast(*binary_key_array, &key_type, CastOptions::Safe(), ctx));
+ // Populate the grouper with the keys from the array
+ ExecSpan batch({ExecValue(*key_array->data())}, key_array->length());
+ RETURN_NOT_OK(grouper_->Populate(batch));
+ if (grouper_->num_groups() != options->key_names.size()) {
+ // There's a duplicate key, find it to emit a nicer error message
+ std::unordered_set<std::string_view> seen;
+ for (const auto& key : options->key_names) {
+ auto [_, inserted] = seen.emplace(key);
+ if (!inserted) {
+ return Status::KeyError("Duplicate key name '", key, "' in PivotWiderOptions");
+ }
}
+ Unreachable("Grouper doesn't agree with std::unordered_set");
}
- unexpected_key_behavior_ = options->unexpected_key_behavior;
return Status::OK();
}
- protected:
- Result<PivotWiderKeyIndex> KeyNotFound(std::string_view key_name) {
- if (unexpected_key_behavior_ == PivotWiderOptions::kIgnore) {
- return kNullPivotKey;
+ Result<std::shared_ptr<ArrayData>> MapKeys(const ArraySpan& array) override {
+ if (array.GetNullCount() != 0) {
+ return NullKeyName();
}
- DCHECK_EQ(unexpected_key_behavior_, PivotWiderOptions::kRaise);
- return Status::KeyError("Unexpected pivot key: ", key_name);
+ return MapKeysInternal(array, array.length);
}
- Result<PivotWiderKeyIndex> LookupKey(std::string_view key_name) {
- const auto it = this->key_name_map_.find(key_name);
- if (ARROW_PREDICT_FALSE(it == this->key_name_map_.end())) {
- return KeyNotFound(key_name);
+ Result<std::optional<PivotWiderKeyIndex>> MapKey(const Scalar& scalar) override {
+ if (!scalar.is_valid) {
+ return NullKeyName();
+ }
+ ARROW_ASSIGN_OR_RAISE(auto group_id_array, MapKeysInternal(&scalar, /*length=*/1));
+ DCHECK_EQ(group_id_array->length, 1);
+ if (group_id_array->GetNullCount() == 0) {
+ return group_id_array->GetValues<uint32_t>(1)[0];
} else {
- return it->second;
+ return std::nullopt;
Review Comment:
Maybe add a comment that this is for the `UnexpectedKeyBehavior::kIgnore` case.
--
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
Re: [PR] GH-45732: [C++][Compute] Accept more pivot key types [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #45945:
URL: https://github.com/apache/arrow/pull/45945#discussion_r2021003973
##########
cpp/src/arrow/compute/exec.h:
##########
@@ -276,7 +276,7 @@ struct ExecValue {
ArraySpan array = {};
const Scalar* scalar = NULLPTR;
- ExecValue(Scalar* scalar) // NOLINT implicit conversion
+ ExecValue(const Scalar* scalar) // NOLINT implicit conversion
Review Comment:
No, because a `Scalar*` can be accepted as `const Scalar*`, so this actually makes this constructor more broadly applicable.
--
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
Re: [PR] GH-45732: [C++][Compute] Accept more pivot key types [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #45945:
URL: https://github.com/apache/arrow/pull/45945#discussion_r2024469418
##########
cpp/src/arrow/compute/kernels/aggregate_pivot.cc:
##########
@@ -42,47 +44,56 @@ struct PivotImpl : public ScalarAggregator {
values_.push_back(MakeNullScalar(value_type));
}
out_type_ = struct_(std::move(fields));
- ARROW_ASSIGN_OR_RAISE(key_mapper_, PivotWiderKeyMapper::Make(*key_type_, options_));
+ ARROW_ASSIGN_OR_RAISE(key_mapper_,
+ PivotWiderKeyMapper::Make(*key_type_, options_, ctx));
return Status::OK();
}
Status Consume(KernelContext*, const ExecSpan& batch) override {
DCHECK_EQ(batch.num_values(), 2);
if (batch[0].is_array()) {
- ARROW_ASSIGN_OR_RAISE(span<const PivotWiderKeyIndex> keys,
- key_mapper_->MapKeys(batch[0].array));
+ ARROW_ASSIGN_OR_RAISE(auto keys_array, key_mapper_->MapKeys(batch[0].array));
Review Comment:
Will do.
--
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
Re: [PR] GH-45732: [C++][Compute] Accept more pivot key types [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #45945:
URL: https://github.com/apache/arrow/pull/45945#issuecomment-2757291485
By the way, this PR (the move to a Grouper-based pivot implementation) results in a ~15% decrease in hash_pivot_wider performance in the simplistic benchmarks I posted in https://github.com/apache/arrow/issues/45741 .
I assume that `unordered_map` is reasonably fast on a very small hash table with short keys, and Grouper may suffer from a high setup cost (there seems to be a columnar-to-row conversion step, for example).
--
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
Re: [PR] GH-45732: [C++][Compute] Accept more pivot key types [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #45945:
URL: https://github.com/apache/arrow/pull/45945#issuecomment-2758507231
@github-actions crossbow submit -g cpp
--
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
Re: [PR] GH-45732: [C++][Compute] Accept more pivot key types [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #45945:
URL: https://github.com/apache/arrow/pull/45945#discussion_r2021022438
##########
cpp/src/arrow/compute/kernels/pivot_internal.cc:
##########
@@ -18,110 +18,137 @@
#include "arrow/compute/kernels/pivot_internal.h"
#include <cstdint>
+#include <string_view>
+#include <unordered_set>
+#include "arrow/array/array_primitive.h"
+#include "arrow/array/builder_binary.h"
+#include "arrow/compute/cast.h"
#include "arrow/compute/exec.h"
#include "arrow/compute/kernels/codegen_internal.h"
+#include "arrow/compute/row/grouper.h"
#include "arrow/scalar.h"
#include "arrow/type_traits.h"
+#include "arrow/util/bit_run_reader.h"
#include "arrow/util/checked_cast.h"
+#include "arrow/util/logging.h"
+#include "arrow/util/unreachable.h"
#include "arrow/visit_type_inline.h"
namespace arrow::compute::internal {
using ::arrow::util::span;
-struct BasePivotKeyMapper : public PivotWiderKeyMapper {
- Status Init(const PivotWiderOptions* options) override {
- if (options->key_names.size() > static_cast<size_t>(kMaxPivotKey) + 1) {
+struct ConcretePivotWiderKeyMapper : public PivotWiderKeyMapper {
+ Status Init(const DataType& key_type, const PivotWiderOptions* options,
+ ExecContext* ctx) {
+ if (options->key_names.size() > static_cast<size_t>(kMaxPivotKey)) {
return Status::NotImplemented("Pivoting to more than ",
- static_cast<size_t>(kMaxPivotKey) + 1,
- " columns: got ", options->key_names.size());
+ static_cast<size_t>(kMaxPivotKey), " columns: got ",
+ options->key_names.size());
}
- key_name_map_.reserve(options->key_names.size());
- PivotWiderKeyIndex index = 0;
- for (const auto& key_name : options->key_names) {
- bool inserted =
- key_name_map_.try_emplace(std::string_view(key_name), index++).second;
- if (!inserted) {
- return Status::KeyError("Duplicate key name '", key_name,
- "' in PivotWiderOptions");
+ unexpected_key_behavior_ = options->unexpected_key_behavior;
+ ARROW_ASSIGN_OR_RAISE(grouper_, Grouper::Make({&key_type}, ctx));
+ // Build a binary array of the pivot key values, and cast it to the desired key type
+ BinaryBuilder builder(ctx->memory_pool());
+ RETURN_NOT_OK(builder.Reserve(options->key_names.size()));
+ int64_t total_length = 0;
+ for (const auto& key : options->key_names) {
+ total_length += static_cast<int64_t>(key.length());
+ }
+ RETURN_NOT_OK(builder.ReserveData(total_length));
+ for (const auto& key : options->key_names) {
+ builder.UnsafeAppend(key);
+ }
+ ARROW_ASSIGN_OR_RAISE(auto binary_key_array, builder.Finish());
+ ARROW_ASSIGN_OR_RAISE(auto key_array,
+ Cast(*binary_key_array, &key_type, CastOptions::Safe(), ctx));
+ // Populate the grouper with the keys from the array
+ ExecSpan batch({ExecValue(*key_array->data())}, key_array->length());
+ RETURN_NOT_OK(grouper_->Populate(batch));
+ if (grouper_->num_groups() != options->key_names.size()) {
+ // There's a duplicate key, find it to emit a nicer error message
+ std::unordered_set<std::string_view> seen;
+ for (const auto& key : options->key_names) {
+ auto [_, inserted] = seen.emplace(key);
+ if (!inserted) {
+ return Status::KeyError("Duplicate key name '", key, "' in PivotWiderOptions");
+ }
}
+ Unreachable("Grouper doesn't agree with std::unordered_set");
}
- unexpected_key_behavior_ = options->unexpected_key_behavior;
return Status::OK();
}
- protected:
- Result<PivotWiderKeyIndex> KeyNotFound(std::string_view key_name) {
- if (unexpected_key_behavior_ == PivotWiderOptions::kIgnore) {
- return kNullPivotKey;
+ Result<std::shared_ptr<ArrayData>> MapKeys(const ArraySpan& array) override {
+ if (array.GetNullCount() != 0) {
+ return NullKeyName();
}
- DCHECK_EQ(unexpected_key_behavior_, PivotWiderOptions::kRaise);
- return Status::KeyError("Unexpected pivot key: ", key_name);
+ return MapKeysInternal(array, array.length);
}
- Result<PivotWiderKeyIndex> LookupKey(std::string_view key_name) {
- const auto it = this->key_name_map_.find(key_name);
- if (ARROW_PREDICT_FALSE(it == this->key_name_map_.end())) {
- return KeyNotFound(key_name);
+ Result<std::optional<PivotWiderKeyIndex>> MapKey(const Scalar& scalar) override {
+ if (!scalar.is_valid) {
+ return NullKeyName();
+ }
+ ARROW_ASSIGN_OR_RAISE(auto group_id_array, MapKeysInternal(&scalar, /*length=*/1));
+ DCHECK_EQ(group_id_array->length, 1);
+ if (group_id_array->GetNullCount() == 0) {
+ return group_id_array->GetValues<uint32_t>(1)[0];
} else {
- return it->second;
+ return std::nullopt;
+ }
+ }
+
+ protected:
+ Result<std::shared_ptr<ArrayData>> MapKeysInternal(const ExecValue& values,
+ int64_t length) {
+ ARROW_ASSIGN_OR_RAISE(auto result, grouper_->Lookup(ExecSpan({values}, length)));
+ DCHECK(result.is_array());
+ DCHECK_EQ(result.type()->id(), Type::UINT32);
+ auto group_id_array = result.array();
+ if (ARROW_PREDICT_FALSE(group_id_array->GetNullCount() != 0) &&
Review Comment:
Ok, will do.
##########
cpp/src/arrow/compute/kernels/pivot_internal.cc:
##########
@@ -18,110 +18,137 @@
#include "arrow/compute/kernels/pivot_internal.h"
#include <cstdint>
+#include <string_view>
+#include <unordered_set>
+#include "arrow/array/array_primitive.h"
+#include "arrow/array/builder_binary.h"
+#include "arrow/compute/cast.h"
#include "arrow/compute/exec.h"
#include "arrow/compute/kernels/codegen_internal.h"
+#include "arrow/compute/row/grouper.h"
#include "arrow/scalar.h"
#include "arrow/type_traits.h"
+#include "arrow/util/bit_run_reader.h"
#include "arrow/util/checked_cast.h"
+#include "arrow/util/logging.h"
+#include "arrow/util/unreachable.h"
#include "arrow/visit_type_inline.h"
namespace arrow::compute::internal {
using ::arrow::util::span;
-struct BasePivotKeyMapper : public PivotWiderKeyMapper {
- Status Init(const PivotWiderOptions* options) override {
- if (options->key_names.size() > static_cast<size_t>(kMaxPivotKey) + 1) {
+struct ConcretePivotWiderKeyMapper : public PivotWiderKeyMapper {
+ Status Init(const DataType& key_type, const PivotWiderOptions* options,
+ ExecContext* ctx) {
+ if (options->key_names.size() > static_cast<size_t>(kMaxPivotKey)) {
return Status::NotImplemented("Pivoting to more than ",
- static_cast<size_t>(kMaxPivotKey) + 1,
- " columns: got ", options->key_names.size());
+ static_cast<size_t>(kMaxPivotKey), " columns: got ",
+ options->key_names.size());
}
- key_name_map_.reserve(options->key_names.size());
- PivotWiderKeyIndex index = 0;
- for (const auto& key_name : options->key_names) {
- bool inserted =
- key_name_map_.try_emplace(std::string_view(key_name), index++).second;
- if (!inserted) {
- return Status::KeyError("Duplicate key name '", key_name,
- "' in PivotWiderOptions");
+ unexpected_key_behavior_ = options->unexpected_key_behavior;
+ ARROW_ASSIGN_OR_RAISE(grouper_, Grouper::Make({&key_type}, ctx));
+ // Build a binary array of the pivot key values, and cast it to the desired key type
+ BinaryBuilder builder(ctx->memory_pool());
+ RETURN_NOT_OK(builder.Reserve(options->key_names.size()));
+ int64_t total_length = 0;
+ for (const auto& key : options->key_names) {
+ total_length += static_cast<int64_t>(key.length());
+ }
+ RETURN_NOT_OK(builder.ReserveData(total_length));
+ for (const auto& key : options->key_names) {
+ builder.UnsafeAppend(key);
+ }
+ ARROW_ASSIGN_OR_RAISE(auto binary_key_array, builder.Finish());
+ ARROW_ASSIGN_OR_RAISE(auto key_array,
+ Cast(*binary_key_array, &key_type, CastOptions::Safe(), ctx));
+ // Populate the grouper with the keys from the array
+ ExecSpan batch({ExecValue(*key_array->data())}, key_array->length());
+ RETURN_NOT_OK(grouper_->Populate(batch));
+ if (grouper_->num_groups() != options->key_names.size()) {
+ // There's a duplicate key, find it to emit a nicer error message
+ std::unordered_set<std::string_view> seen;
+ for (const auto& key : options->key_names) {
+ auto [_, inserted] = seen.emplace(key);
+ if (!inserted) {
+ return Status::KeyError("Duplicate key name '", key, "' in PivotWiderOptions");
+ }
}
+ Unreachable("Grouper doesn't agree with std::unordered_set");
}
- unexpected_key_behavior_ = options->unexpected_key_behavior;
return Status::OK();
}
- protected:
- Result<PivotWiderKeyIndex> KeyNotFound(std::string_view key_name) {
- if (unexpected_key_behavior_ == PivotWiderOptions::kIgnore) {
- return kNullPivotKey;
+ Result<std::shared_ptr<ArrayData>> MapKeys(const ArraySpan& array) override {
+ if (array.GetNullCount() != 0) {
+ return NullKeyName();
}
- DCHECK_EQ(unexpected_key_behavior_, PivotWiderOptions::kRaise);
- return Status::KeyError("Unexpected pivot key: ", key_name);
+ return MapKeysInternal(array, array.length);
}
- Result<PivotWiderKeyIndex> LookupKey(std::string_view key_name) {
- const auto it = this->key_name_map_.find(key_name);
- if (ARROW_PREDICT_FALSE(it == this->key_name_map_.end())) {
- return KeyNotFound(key_name);
+ Result<std::optional<PivotWiderKeyIndex>> MapKey(const Scalar& scalar) override {
+ if (!scalar.is_valid) {
+ return NullKeyName();
+ }
+ ARROW_ASSIGN_OR_RAISE(auto group_id_array, MapKeysInternal(&scalar, /*length=*/1));
+ DCHECK_EQ(group_id_array->length, 1);
+ if (group_id_array->GetNullCount() == 0) {
+ return group_id_array->GetValues<uint32_t>(1)[0];
} else {
- return it->second;
+ return std::nullopt;
Review Comment:
Yes, will do.
--
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
Re: [PR] GH-45732: [C++][Compute] Accept more pivot key types [arrow]
Posted by "kszucs (via GitHub)" <gi...@apache.org>.
kszucs commented on code in PR #45945:
URL: https://github.com/apache/arrow/pull/45945#discussion_r2020654596
##########
cpp/src/arrow/compute/exec.h:
##########
@@ -276,7 +276,7 @@ struct ExecValue {
ArraySpan array = {};
const Scalar* scalar = NULLPTR;
- ExecValue(Scalar* scalar) // NOLINT implicit conversion
+ ExecValue(const Scalar* scalar) // NOLINT implicit conversion
Review Comment:
Isn't this a breaking change? Perhaps apply it in a separate PR?
--
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
Re: [PR] GH-45732: [C++][Compute] Accept more pivot key types [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #45945:
URL: https://github.com/apache/arrow/pull/45945#issuecomment-2772362130
I'm going to merge this for 21.0. Thanks for the reviews!
--
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
Re: [PR] GH-45732: [C++][Compute] Accept more pivot key types [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #45945:
URL: https://github.com/apache/arrow/pull/45945#issuecomment-2773720623
After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 4d625b37ccb6d1cff1f1c47138318acc6751d4b0.
There were no benchmark performance regressions. 🎉
The [full Conbench report](https://github.com/apache/arrow/runs/39876640735) has more details. It also includes information about 11 possible false positives for unstable benchmarks that are known to sometimes produce them.
--
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
Re: [PR] GH-45732: [C++][Compute] Accept more pivot key types [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #45945:
URL: https://github.com/apache/arrow/pull/45945#issuecomment-2772054028
@github-actions crossbow submit -g cpp
--
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
Re: [PR] GH-45732: [C++][Compute] Accept more pivot key types [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #45945:
URL: https://github.com/apache/arrow/pull/45945#issuecomment-2772060457
Revision: 73f9f797abbd7259a62e8df499abd07402c85e63
Submitted crossbow builds: [ursacomputing/crossbow @ actions-246ac2532b](https://github.com/ursacomputing/crossbow/branches/all?query=actions-246ac2532b)
|Task|Status|
|----|------|
|example-cpp-minimal-build-static|[](https://github.com/ursacomputing/crossbow/actions/runs/14216798881/job/39835287361)|
|example-cpp-minimal-build-static-system-dependency|[](https://github.com/ursacomputing/crossbow/actions/runs/14216798718/job/39835286792)|
|example-cpp-tutorial|[](https://github.com/ursacomputing/crossbow/actions/runs/14216798655/job/39835286441)|
|test-alpine-linux-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/14216798995/job/39835288430)|
|test-build-cpp-fuzz|[](https://github.com/ursacomputing/crossbow/actions/runs/14216799236/job/39835289424)|
|test-conda-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/14216799001/job/39835288204)|
|test-conda-cpp-meson|[](https://github.com/ursacomputing/crossbow/actions/runs/14216798866/job/39835287199)|
|test-conda-cpp-valgrind|[](https://github.com/ursacomputing/crossbow/actions/runs/14216798684/job/39835286490)|
|test-cuda-cpp-ubuntu-22.04-cuda-11.7.1|[](https://github.com/ursacomputing/crossbow/actions/runs/14216798524/job/39835285889)|
|test-debian-12-cpp-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/14216798261/job/39835285364)|
|test-debian-12-cpp-i386|[](https://github.com/ursacomputing/crossbow/actions/runs/14216798922/job/39835287898)|
|test-fedora-39-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/14216798656/job/39835286376)|
|test-ubuntu-22.04-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/14216798461/job/39835285629)|
|test-ubuntu-22.04-cpp-20|[](https://github.com/ursacomputing/crossbow/actions/runs/14216799149/job/39835288978)|
|test-ubuntu-22.04-cpp-bundled|[](https://github.com/ursacomputing/crossbow/actions/runs/14216799016/job/39835288224)|
|test-ubuntu-22.04-cpp-emscripten|[](https://github.com/ursacomputing/crossbow/actions/runs/14216798924/job/39835287961)|
|test-ubuntu-22.04-cpp-no-threading|[](https://github.com/ursacomputing/crossbow/actions/runs/14216798833/job/39835287147)|
|test-ubuntu-24.04-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/14216798871/job/39835287349)|
|test-ubuntu-24.04-cpp-bundled-offline|[](https://github.com/ursacomputing/crossbow/actions/runs/14216798418/job/39835285383)|
|test-ubuntu-24.04-cpp-gcc-13-bundled|[](https://github.com/ursacomputing/crossbow/actions/runs/14216798732/job/39835286937)|
|test-ubuntu-24.04-cpp-gcc-14|[](https://github.com/ursacomputing/crossbow/actions/runs/14216799096/job/39835288770)|
|test-ubuntu-24.04-cpp-minimal-with-formats|[](https://github.com/ursacomputing/crossbow/actions/runs/14216799138/job/39835288989)|
|test-ubuntu-24.04-cpp-thread-sanitizer|[](https://github.com/ursacomputing/crossbow/actions/runs/14216798512/job/39835285806)|
--
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