[GitHub] [arrow] pitrou commented on a change in pull request #11005: ARROW-13691: [C++] Support skip_nulls/min_count in VarianceOptions

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

pitrou commented on a change in pull request #11005:
URL: https://github.com/apache/arrow/pull/11005#discussion_r699452309



##########
File path: cpp/src/arrow/compute/kernels/aggregate_test.cc
##########
@@ -2264,6 +2264,16 @@ class TestPrimitiveVarStdKernel : public ::testing::Test {
     AssertVarStdIsInvalid(array, options);
   }
 
+  void AssertVarStdIsNull(const std::string& json, const VarianceOptions& options) {

Review comment:
       Isn't this exactly the same as `AssertVarStdIsInvalid`?

##########
File path: cpp/src/arrow/compute/kernels/aggregate_var_std.cc
##########
@@ -135,9 +142,11 @@ struct VarStdState {
                 &this->mean, &this->m2);
   }
 
+  VarianceOptions options;
   int64_t count = 0;
   double mean = 0;
   double m2 = 0;  // m2 = count*s2 = sum((X-mean)^2)
+  bool valid = true;

Review comment:
       Should this be called `all_valid` for clarity?

##########
File path: cpp/src/arrow/compute/kernels/aggregate_var_std.cc
##########
@@ -135,9 +142,11 @@ struct VarStdState {
                 &this->mean, &this->m2);
   }
 
+  VarianceOptions options;

Review comment:
       Nit: make this `const`?




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