[GitHub] [arrow] nealrichardson commented on a change in pull request #9758: ARROW-9054: [C++] Add ScalarAggregateOptions

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

nealrichardson commented on a change in pull request #9758:
URL: https://github.com/apache/arrow/pull/9758#discussion_r634901907



##########
File path: r/R/compute.R
##########
@@ -110,15 +114,16 @@ max.ArrowDatum <- function(..., na.rm = FALSE) {
   scalar_aggregate("min_max", ..., na.rm = na.rm)$GetFieldByName("max")
 }
 
-scalar_aggregate <- function(FUN, ..., na.rm = FALSE) {
+scalar_aggregate <- function(FUN, ..., na.rm = FALSE, na.min_count = 0) {
   a <- collect_arrays_from_dots(list(...))
-  if (!na.rm && a$null_count > 0 && (FUN %in% c("mean", "sum"))) {
-    # Arrow sum/mean function always drops NAs so handle that here
-    # https://issues.apache.org/jira/browse/ARROW-9054
-    return(Scalar$create(NA_real_))
+  if (FUN %in% c("mean", "sum") && !na.rm) {
+    na.min_count = length(a)
+  }
+  if (FUN == "min_max" && na.rm && a$null_count == length(a)) {
+    Array$create(data.frame(min=Inf, max=-Inf))

Review comment:
       Why this special case? Is there a reason the C++ library shouldn't handle this? (Leave a comment explaining and/or linking to the JIRA issue to do this in C++)

##########
File path: r/R/compute.R
##########
@@ -110,15 +114,16 @@ max.ArrowDatum <- function(..., na.rm = FALSE) {
   scalar_aggregate("min_max", ..., na.rm = na.rm)$GetFieldByName("max")
 }
 
-scalar_aggregate <- function(FUN, ..., na.rm = FALSE) {
+scalar_aggregate <- function(FUN, ..., na.rm = FALSE, na.min_count = 0) {
   a <- collect_arrays_from_dots(list(...))
-  if (!na.rm && a$null_count > 0 && (FUN %in% c("mean", "sum"))) {
-    # Arrow sum/mean function always drops NAs so handle that here
-    # https://issues.apache.org/jira/browse/ARROW-9054
-    return(Scalar$create(NA_real_))
+  if (FUN %in% c("mean", "sum") && !na.rm) {
+    na.min_count = length(a)

Review comment:
       Could you leave a comment explaining this? It's not obvious to me what `na.min_count` does since it's not an R concept.




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