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)
}
```
--
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]
