nealrichardson commented on a change in pull request #10888:
URL: https://github.com/apache/arrow/pull/10888#discussion_r698706872
##########
File path: r/R/dataset-scan.R
##########
@@ -85,9 +86,30 @@ Scanner$create <- function(dataset,
# To handle mutate() on Table/RecordBatch, we need to
collect(as_data_frame=FALSE) now
dataset <- dplyr::collect(dataset, as_data_frame = FALSE)
}
+
+ proj <- c(dataset$selected_columns, dataset$temp_columns)
+
+ if (!is.null(projection)) {
+ if (is.character(projection)) {
+ proj <- proj[projection]
Review comment:
Should probably assert `all(projection %in% names(proj))`, otherwise
you'll get some garbage here.
##########
File path: r/R/dataset-scan.R
##########
@@ -85,9 +86,30 @@ Scanner$create <- function(dataset,
# To handle mutate() on Table/RecordBatch, we need to
collect(as_data_frame=FALSE) now
dataset <- dplyr::collect(dataset, as_data_frame = FALSE)
}
+
+ proj <- c(dataset$selected_columns, dataset$temp_columns)
+
+ if (!is.null(projection)) {
+ if (is.character(projection)) {
+ proj <- proj[projection]
+ } else if (is_list_of(projection, "Expression")) {
+ # TODO: need to check and see if there are any Expressions that are
simply
+ # field refs in projections, but are richer expressions in proj?
+ proj <- projection
Review comment:
I agree it's not great, but if we know that duckdb is just using the
character vector version, let's make a todo jira and punt this.
##########
File path: r/R/dplyr-filter.R
##########
@@ -71,8 +71,15 @@ filter.Dataset <- filter.ArrowTabular <-
filter.arrow_dplyr_query
set_filters <- function(.data, expressions) {
if (length(expressions)) {
- # expressions is a list of Expressions. AND them together and set them on
.data
- new_filter <- Reduce("&", expressions)
+ if (is.list(expressions)) {
+ # expressions is a list of Expressions. AND them together and set them
on .data
+ new_filter <- Reduce("&", expressions)
+ } else {
+ # expressions should be an expression or list of expressions already
+ stopifnot(is_list_of(expressions, "Expression") | inherits(expressions,
"Expression"))
+ new_filter <- expressions
+ }
Review comment:
how do you have a list of expressions here? won't `is.list(expressions)`
catch that? do you mean something like this?
```suggestion
} else if (inherits(expressions, "Expression")) {
new_filter <- expressions
} else {
stop("Some error message here", call. = FALSE)
}
```
##########
File path: r/R/dataset-scan.R
##########
@@ -85,9 +85,28 @@ Scanner$create <- function(dataset,
# To handle mutate() on Table/RecordBatch, we need to
collect(as_data_frame=FALSE) now
dataset <- dplyr::collect(dataset, as_data_frame = FALSE)
}
+
+ proj <- c(dataset$selected_columns, dataset$temp_columns)
+
+ if (!is.null(projection)) {
+ if (is.character(projection)) {
+ proj <- proj[projection]
+ } else {
+ # TODO: ARROW-13802 accepting lists of Expressions as a projectoin
Review comment:
```suggestion
# TODO: ARROW-13802 accepting lists of Expressions as a projection
```
##########
File path: r/R/dataset-scan.R
##########
@@ -28,7 +28,7 @@
#'
#' * `dataset`: A `Dataset` or `arrow_dplyr_query` object, as returned by the
#' `dplyr` methods on `Dataset`.
-#' * `projection`: A character vector of column names to select
+#' * `projection`: A character vector of column names to select columns
Review comment:
As it turns out, `projection` can be a named list of Expressions, just
not when dataset is an arrow_dplyr_query (and thus already contains a named
list of Expressions)
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
