jonkeane commented on code in PR #51291:
URL: https://github.com/apache/arrow/pull/51291#discussion_r4031389906
##########
r/tests/testthat/test-dplyr-filter.R:
##########
@@ -547,3 +547,15 @@ test_that("More complex select/filter_out", {
tbl
)
})
+
+test_that("filter() with a variable that shares a name with a function
binding", {
+ # GH-39688: `date` and `day` are also function bindings
Review Comment:
This is a nice test and I want to keep it, but should we also have one that
is more basic and shows off that `date` and `day` are edgecases here
demonstrating the functionality of something like this:
```
my_constant <- "three:
compare_dplyr_binding(
.input |>
filter(chr == my_constant) |>
collect(),
tbl
)
```
Because ^^^ _also_ didn't work before, yeah? Or did it actually work we just
didn't do the masking in the right order to let it work like dplyr does?
##########
r/R/dplyr-eval.R:
##########
@@ -128,6 +132,30 @@ add_user_functions_to_mask <- function(expr, mask) {
invisible()
}
+add_user_variables_to_mask <- function(expr, mask) {
+ # The function bindings environment sits between the columns and the user's
+ # environment in the mask, so a symbol like `date` in `filter(Date == date)`
+ # would resolve to the `date()` binding rather than the user's variable.
+ # dplyr would find the variable, so bind it into the mask so we do too.
Review Comment:
the so ... so ... is a little awkward here
--
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]
