untracked: Allow user to specify more aggregation functions, include … #54
Conversation
oliverbock
force-pushed
the
oliver-distinctby
branch
from
3dbd2ab
to
308dcf4
Compare
oliverbock
force-pushed
the
oliver-distinctby
branch
from
308dcf4
to
4d48340
Compare
|
Apologies. I was away the entire month of April and this slipped through the cracks on my return. Looking now. |
R/Factbase.R
Outdated
| if (!is.null(distinct_by)) | ||
| metric$distinctBy <- distinct_by |
There was a problem hiding this comment.
The if check is unnecessary. NULL assignments to lists only occur in the constructing call of a list. When modifying, assigning NULL to a list element is the same as removing that element.
> list(x = 1, y = 2, z = NULL)
$x
[1] 1
$y
[1] 2
$z
NULL
> l <- list(x = 1, y = 2)
> l$z <- NULL
> l
$x
[1] 1
$y
[1] 2
Also related is my confusion with distinct_by and distinctBy (there might be a good reason for this but I was confused in a later comment too).
R/Factbase.R
Outdated
| return (list(aggregation = aggregation, distinct_by=NULL)) | ||
| distinct_by <- str_match(aggregation, 'countdistinct\\(([^)]+)\\)')[1, 2] | ||
| if (is.character(distinct_by)) { | ||
| if (!(distinct_by %in% names(data))) | ||
| stop(paste0("Column '", distinct_by, "' is referred to in 'aggregation' but does not exist in 'data'")) | ||
| return (list(aggregation = 'count', distinctBy = distinct_by)) |
There was a problem hiding this comment.
return list has distinct_by in the first version and distinctBy in the second. I'm guessing one of these needs to be changed for consistency.
| validate_aggregation <- function(aggregation, data) { | ||
| if (!is.character(aggregation) || length(aggregation) != 1) | ||
| stop("'aggregation' must be a character vector of length 1") |
There was a problem hiding this comment.
Is data always intended to be a data.frame? If so, could add a validation check like the aggregation has?
There was a problem hiding this comment.
Yes, that is done separately in validate_dataframe() (the data frame is passed in here knowing that it has already been validated)
…coundistinct