kszucs commented on a change in pull request #7478: URL: https://github.com/apache/arrow/pull/7478#discussion_r445007543
########## 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: I was thinking of that, but the current false_count imlementation triggers true_count, so we would't iterate over the values at least twice to get both true and false count - it might be more efficient though. I'm going to write a benchmark for 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org