lidavidm commented on code in PR #13509:
URL: https://github.com/apache/arrow/pull/13509#discussion_r916983041
##########
cpp/src/arrow/compute/exec/plan_test.cc:
##########
@@ -933,6 +933,38 @@ TEST(ExecPlanExecution, SourceGroupedSum) {
}
}
+TEST(ExecPlanExecution, SourceMinMaxScalar) {
+ for (bool parallel : { false, true }) {
+ SCOPED_TRACE(parallel ? "parallel/merged" : "serial");
+
+ auto input = MakeGroupableBatches(/*multiplicity=*/parallel ? 100 : 1);
+ auto minmax_opts = std::make_shared<ScalarAggregateOptions>();
+ auto expected_value = StructScalar::Make(
Review Comment:
nit, but doesn't ScalarFromJSON handle StructScalar directly?
##########
cpp/src/arrow/compute/exec/options.h:
##########
@@ -109,6 +109,10 @@ class ARROW_EXPORT ProjectNodeOptions : public
ExecNodeOptions {
};
/// \brief Make a node which aggregates input batches, optionally grouped by
keys.
+///
+/// If the keys attribute is a non-empty vector, then each aggregate in
`aggregates` is
+/// expected to be a HashAggregate function. If the keys attribute is an empty
vector,
+/// then each aggregate is assumed to be a ScalarAggregate function.
Review Comment:
:heavy_check_mark: thank you!
##########
cpp/src/arrow/compute/exec/plan_test.cc:
##########
@@ -933,6 +933,38 @@ TEST(ExecPlanExecution, SourceGroupedSum) {
}
}
+TEST(ExecPlanExecution, SourceMinMaxScalar) {
Review Comment:
```suggestion
TEST(ExecPlanExecution, SourceMinMaxScalar) {
// Regression test for ARROW-16904
```
##########
cpp/src/arrow/compute/exec/plan_test.cc:
##########
@@ -933,6 +933,38 @@ TEST(ExecPlanExecution, SourceGroupedSum) {
}
}
+TEST(ExecPlanExecution, SourceMinMaxScalar) {
Review Comment:
Is there a task filed for testing all aggregates in this fashion?
##########
cpp/src/arrow/compute/exec/plan_test.cc:
##########
@@ -933,6 +933,38 @@ TEST(ExecPlanExecution, SourceGroupedSum) {
}
}
+TEST(ExecPlanExecution, SourceMinMaxScalar) {
Review Comment:
Preferably we'd update the existing aggregate tests to run all aggregates
"both ways" instead of manually duplicating tests like this on a case-by-case
basis
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
