westonpace commented on a change in pull request #10845:
URL: https://github.com/apache/arrow/pull/10845#discussion_r699831758
##########
File path: cpp/src/arrow/compute/kernels/hash_aggregate_test.cc
##########
@@ -433,6 +443,18 @@ TEST(Grouper, NumericKey) {
g.ExpectConsume("[[3], [27], [3], [27], [null], [81], [27], [81]]",
"[0, 1, 0, 1, 3, 2, 1, 2]");
+
+ g.ExpectFind("[[3], [3]]", "[0, 0]");
+
+ g.ExpectFind("[[3], [3]]", "[0, 0]");
Review comment:
Did you mean to run an identical test twice? Not sure if this is a
copy/paste or you are testing for idempotence/deterministic behavior.
##########
File path: cpp/src/arrow/compute/kernels/hash_aggregate.cc
##########
@@ -2129,9 +2179,10 @@ Result<FieldVector> ResolveKernels(
Result<std::unique_ptr<Grouper>> Grouper::Make(const std::vector<ValueDescr>&
descrs,
ExecContext* ctx) {
- if (GrouperFastImpl::CanUse(descrs)) {
- return GrouperFastImpl::Make(descrs, ctx);
- }
+ // TODO(niranda) re-enable this!
+ // if (GrouperFastImpl::CanUse(descrs)) {
+ // return GrouperFastImpl::Make(descrs, ctx);
+ // }
Review comment:
Can you make a JIRA for this?
##########
File path: cpp/src/arrow/compute/exec/hash_join_node_benchmark.cc
##########
@@ -0,0 +1,18 @@
+// Licensed to the Apache Software Foundation (ASF) under one
Review comment:
Remove this file?
##########
File path: cpp/src/arrow/compute/exec/util.h
##########
@@ -19,6 +19,8 @@
#include <atomic>
#include <cstdint>
+#include <thread>
Review comment:
Hmm, I hate to add this in late but I didn't notice it earlier. So far
we have managed to keep `<thread>` out of the public API surface. Is there
anyway you can push this into the `util.cc`? This utility
(https://github.com/apache/arrow/blob/4591d76fce2846a29dac33bf01e9ba0337b118e9/cpp/src/arrow/util/io_util.h#L346)
can probably prevent you from having to resort to pimpl.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
