Copilot commented on code in PR #51291:
URL: https://github.com/apache/arrow/pull/51291#discussion_r4041578814
##########
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 we bind it into the mask to match.
+ if (is_quosure(expr)) {
+ function_env <- parent.env(parent.env(mask))
+ quo_env <- quo_get_env(expr)
+ # all.vars() returns symbols that aren't in function position
+ vars_in_expr <- all.vars(quo_get_expr(expr))
+ columns <- names(mask$.data)
+ shadowed <- setdiff(intersect(vars_in_expr, ls(function_env, all.names =
TRUE)), columns)
Review Comment:
`shadowed` is computed by calling `ls(function_env, ...)` and intersecting,
which scans the entire bindings environment for every evaluated expression.
Since `vars_in_expr` is typically small, checking `exists()` per symbol avoids
repeatedly enumerating all bindings and should reduce overhead in dplyr-heavy
workloads.
##########
r/tests/testthat/test-dplyr-filter.R:
##########
@@ -564,3 +564,25 @@ test_that("filter() and filter_out() with across() warn
about deprecation", {
class = "lifecycle_warning_deprecated"
)
})
+
+test_that("filter() with a variable from the calling environment", {
+ my_constant <- "d"
+ compare_dplyr_binding(
+ .input |>
+ filter(chr == my_constant) |>
Review Comment:
These new regression tests exercise the Table path via
`compare_dplyr_binding()`, but GH-39688 reproduces on `open_dataset()` (Dataset
filter planning) with a calling-env variable named like a function binding
(e.g. `date`). Since the repo already has Dataset dplyr tests (e.g.
`test-dataset-dplyr.R`), adding a Dataset-specific assertion for this scenario
would better prevent regressions in the path users hit.
--
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]
