GH-39688: [R] "Error: Filter expression not supported for Arrow Datasets" using "date" expression rigth hand side of a filter - #51291
Conversation
|
|
github-actions
Bot
added
Component: R
awaiting committer review
There was a problem hiding this comment.
🟡 Changes recommended
add_user_variables_to_mask() uses get0() without inherits=TRUE, which can fail to find lexically-scoped variables in parent environments and still diverge from dplyr’s name resolution.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR addresses GH-39688 in Arrow’s R dplyr integration by ensuring that user-defined variables referenced in expressions (e.g., date, day) are correctly resolved even when they share names with Arrow’s function bindings, matching dplyr’s intended masking behavior.
Changes:
- Extend
arrow_eval()to bind user variables into the evaluation mask when they would otherwise be shadowed by function bindings. - Add regression tests covering both
filter()andmutate()when symbols likedate/daycollide with function bindings.
File summaries
| File | Description |
|---|---|
| r/R/dplyr-eval.R | Adds logic to detect and bind shadowed user variables into the evaluation mask. |
| r/tests/testthat/test-dplyr-filter.R | Adds a regression test for filter() symbol collisions with bindings (date, day). |
| r/tests/testthat/test-dplyr-mutate.R | Adds a regression test for mutate() symbol collisions with bindings (day). |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| for (var_name in shadowed) { | ||
| user_var <- get0(var_name, quo_env) | ||
| # Functions from the user's environment (like lubridate::day) shouldn't | ||
| # take precedence over the bindings | ||
| if (!is.null(user_var) && !is.function(user_var)) { |
jonkeane
left a comment
jonkeane
left a comment
There was a problem hiding this comment.
I have a few questions on this one
| # 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. |
There was a problem hiding this comment.
the so ... so ... is a little awkward here
| }) | ||
|
|
||
| test_that("filter() with a variable that shares a name with a function binding", { | ||
| # GH-39688: `date` and `day` are also function bindings |
There was a problem hiding this 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?
Rationale for this change
Error when user tries to use local variable in dplyr pipeline in Arrow
What changes are included in this PR?
Make sure we attach them to the mask
Are these changes tested?
Yup
Are there any user-facing changes?
Yep