-
Notifications
You must be signed in to change notification settings - Fork 4.3k
GH-39688: [R] "Error: Filter expression not supported for Arrow Datasets" using "date" expression rigth hand side of a filter - #51291
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 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.
Uh oh!
There was an error while loading. Please reload this page.
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