[GitHub] [arrow] wesm commented on a change in pull request #7478: ARROW-9055: [C++] Add sum/mean/minmax kernels for Boolean type

[GitBox <gi...@apache.org>]

wesm commented on a change in pull request #7478:
URL: https://github.com/apache/arrow/pull/7478#discussion_r444996273



##########
File path: cpp/src/arrow/compute/kernels/aggregate_test.cc
##########
@@ -399,15 +434,59 @@ class TestNumericMinMaxKernel : public ::testing::Test {
 };
 
 template <typename ArrowType>
-class TestFloatingMinMaxKernel : public TestNumericMinMaxKernel<ArrowType> {};
+class TestBooleanMinMaxKernel : public TestPrimitiveMinMaxKernel<ArrowType> {};
+
+template <typename ArrowType>
+class TestIntegerMinMaxKernel : public TestPrimitiveMinMaxKernel<ArrowType> {};
+
+template <typename ArrowType>
+class TestFloatingMinMaxKernel : public TestPrimitiveMinMaxKernel<ArrowType> {};
+
+TYPED_TEST_SUITE(TestBooleanMinMaxKernel, BooleanArrowType);

Review comment:
       This seems overwrought since there is only one type, can we do it without TYPED_TEST?

##########
File path: cpp/src/arrow/compute/kernels/aggregate_basic.cc
##########
@@ -397,24 +452,26 @@ struct MinMaxImpl : public ScalarAggregator {
 
     ArrayType arr(batch[0].array());
 
-    local.has_nulls = arr.null_count() > 0;
+    const auto null_count = arr.null_count();
+    local.has_nulls = null_count > 0;
+    local.has_values = (arr.length() - null_count) > 0;
+
     if (local.has_nulls && options.null_handling == MinMaxOptions::OUTPUT_NULL) {
       this->state = local;
       return;
     }
 
-    const auto values = arr.raw_values();
-    if (arr.null_count() > 0) {
+    if (local.has_nulls) {
       BitmapReader reader(arr.null_bitmap_data(), arr.offset(), arr.length());
       for (int64_t i = 0; i < arr.length(); i++) {
         if (reader.IsSet()) {
-          local.MergeOne(values[i]);
+          local.MergeOne(arr.Value(i));
         }
         reader.Next();
       }
     } else {
       for (int64_t i = 0; i < arr.length(); i++) {
-        local.MergeOne(values[i]);
+        local.MergeOne(arr.Value(i));
       }

Review comment:
       To be honest this seems wasteful. Shouldn't we compute the true, false, and null count and then set the min/max based on those? 

##########
File path: cpp/src/arrow/testing/gtest_util.h
##########
@@ -137,6 +137,8 @@ namespace arrow {
 // ----------------------------------------------------------------------
 // Useful testing::Types declarations
 
+typedef ::testing::Types<BooleanType> BooleanArrowType;

Review comment:
       Would prefer not to have this per above




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org