Add agg_all_by() to the R client and add error handling around empt…

…y aggregations in `agg_by()` (deephaven#4465)

* Support agg_all_by at the R level, provide a stable API that allows the underlying C++ to change freely

* Snake case

* A little more snake case

* Helpful errors in agg_by

* Code review suggestions

R/rdeephaven/R/aggregate_wrapper.R

@@ -1,95 +1,85 @@
-#' @export
 Aggregation <- R6Class("Aggregation",
   cloneable = FALSE,
   public = list(
     .internal_rcpp_object = NULL,
-    initialize = function(aggregation) {
-      if (class(aggregation) != "Rcpp_INTERNAL_Aggregate") {
-        stop("'aggregation' should be an internal Deephaven Aggregation. If you're seeing this,\n  you are trying to call the constructor of an Aggregation directly, which is not advised.\n  Please use one of the provided aggregation functions instead.")
+    .internal_num_cols = NULL,
+    .internal_agg_name = NULL,
+    initialize = function(aggregation, agg_name, ...) {
+      self$.internal_agg_name <- agg_name
+      args <- list(...)
+      if (any(names(args) == "cols")) {
+        self$.internal_num_cols <- length(args$cols)
       }
-      self$.internal_rcpp_object <- aggregation
+      self$.internal_rcpp_object <- do.call(aggregation, args)
     }
   )
 )
 
 ### All of the functions below return an instance of the above class
 
-#' @export
 agg_first <- function(cols = character()) {
   verify_string("cols", cols, FALSE)
-  return(Aggregation$new(INTERNAL_agg_first(cols)))
+  return(Aggregation$new(INTERNAL_agg_first, "agg_first", cols=cols))
 }
 
-#' @export
 agg_last <- function(cols = character()) {
   verify_string("cols", cols, FALSE)
-  return(Aggregation$new(INTERNAL_agg_last(cols)))
+  return(Aggregation$new(INTERNAL_agg_last, "agg_last", cols=cols))
 }
 
-#' @export
 agg_min <- function(cols = character()) {
   verify_string("cols", cols, FALSE)
-  return(Aggregation$new(INTERNAL_agg_min(cols)))
+  return(Aggregation$new(INTERNAL_agg_min, "agg_min", cols=cols))
 }
 
-#' @export
 agg_max <- function(cols = character()) {
   verify_string("cols", cols, FALSE)
-  return(Aggregation$new(INTERNAL_agg_max(cols)))
+  return(Aggregation$new(INTERNAL_agg_max, "agg_max", cols=cols))
 }
 
-#' @export
 agg_sum <- function(cols = character()) {
   verify_string("cols", cols, FALSE)
-  return(Aggregation$new(INTERNAL_agg_sum(cols)))
+  return(Aggregation$new(INTERNAL_agg_sum, "agg_sum", cols=cols))
 }
 
-#' @export
 agg_abs_sum <- function(cols = character()) {
   verify_string("cols", cols, FALSE)
-  return(Aggregation$new(INTERNAL_agg_abs_sum(cols)))
+  return(Aggregation$new(INTERNAL_agg_abs_sum, "agg_abs_sum", cols=cols))
 }
 
-#' @export
 agg_avg <- function(cols = character()) {
   verify_string("cols", cols, FALSE)
-  return(Aggregation$new(INTERNAL_agg_avg(cols)))
+  return(Aggregation$new(INTERNAL_agg_avg, "agg_avg", cols=cols))
 }
 
-#' @export
 agg_w_avg <- function(wcol, cols = character()) {
   verify_string("wcol", wcol, TRUE)
   verify_string("cols", cols, FALSE)
-  return(Aggregation$new(INTERNAL_agg_w_avg(wcol, cols)))
+  return(Aggregation$new(INTERNAL_agg_w_avg, "agg_w_avg", wcol=wcol, cols=cols))
 }
 
-#' @export
 agg_median <- function(cols = character()) {
   verify_string("cols", cols, FALSE)
-  return(Aggregation$new(INTERNAL_agg_median(cols)))
+  return(Aggregation$new(INTERNAL_agg_median, "agg_median", cols=cols))
 }
 
-#' @export
 agg_var <- function(cols = character()) {
   verify_string("cols", cols, FALSE)
-  return(Aggregation$new(INTERNAL_agg_var(cols)))
+  return(Aggregation$new(INTERNAL_agg_var, "agg_var", cols=cols))
 }
 
-#' @export
 agg_std <- function(cols = character()) {
   verify_string("cols", cols, FALSE)
-  return(Aggregation$new(INTERNAL_agg_std(cols)))
+  return(Aggregation$new(INTERNAL_agg_std, "agg_std", cols=cols))
 }
 
-#' @export
 agg_percentile <- function(percentile, cols = character()) {
   verify_in_unit_interval("percentile", percentile, TRUE)
   verify_string("cols", cols, FALSE)
-  return(Aggregation$new(INTERNAL_agg_percentile(percentile, cols)))
+  return(Aggregation$new(INTERNAL_agg_percentile, "agg_percentile", percentile=percentile, cols=cols))
 }
 
-#' @export
 agg_count <- function(col) {
   verify_string("col", col, TRUE)
-  return(Aggregation$new(INTERNAL_agg_count(col)))
-}
+  return(Aggregation$new(INTERNAL_agg_count, "agg_count", col=col))
+}

R/rdeephaven/R/helper_functions.R

@@ -20,7 +20,7 @@ verify_type <- function(arg_name, candidate, required_type, message_type_name, i
     } else {
       stop(paste0("'", arg_name, "' must be a single ", message_type_name, ". Got an object of class ", first_class(candidate), "."))
     }
-  } else if (is_scalar && (length(candidate) != 1)) {
+  } else if (is_scalar && (length(c(candidate)) != 1)) {
     stop(paste0("'", arg_name, "' must be a single ", message_type_name, ". Got a vector of length ", length(candidate), "."))
   }
 }

R/rdeephaven/R/table_handle_wrapper.R

@@ -107,9 +107,19 @@ TableHandle <- R6Class("TableHandle",
       verify_type("aggs", aggs, "Aggregation", "Deephaven Aggregation", FALSE)
       verify_string("by", by, FALSE)
       aggs <- c(aggs)
+      for (i in 1:length(aggs)) {
+        if (!is.null(aggs[[i]]$.internal_num_cols) && aggs[[i]]$.internal_num_cols == 0) {
+          stop(paste0("Aggregations with no columns cannot be used in 'agg_by'. Got '", aggs[[i]]$.internal_agg_name, "' at index ", i, " with an empty 'cols' argument."))
+        }
+      }
       unwrapped_aggs <- lapply(aggs, strip_r6_wrapping)
       return(TableHandle$new(self$.internal_rcpp_object$agg_by(unwrapped_aggs, by)))
     },
+    agg_all_by = function(agg, by = character()) {
+      verify_type("agg", agg, "Aggregation", "Deephaven Aggregation", TRUE)
+      verify_string("by", by, FALSE)
+      return(TableHandle$new(self$.internal_rcpp_object$agg_all_by(agg$.internal_rcpp_object, by)))
+    },
     first_by = function(by = character()) {
       verify_string("by", by, FALSE)
       return(TableHandle$new(self$.internal_rcpp_object$first_by(by)))
@@ -170,7 +180,7 @@ TableHandle <- R6Class("TableHandle",
       verify_string("by", by, FALSE)
       return(TableHandle$new(self$.internal_rcpp_object$percentile_by(percentile, by)))
     },
-    count_by = function(col = "n", by = character()) {
+    count_by = function(col, by = character()) {
       verify_string("col", col, TRUE)
       verify_string("by", by, FALSE)
       return(TableHandle$new(self$.internal_rcpp_object$count_by(col, by)))