GH-39688: [R] "Error: Filter expression not supported for Arrow Datasets" using "date" expression rigth hand side of a filter

[thisisnic]

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

• GitHub Issue: [R] "Error: Filter expression not supported for Arrow Datasets" using "date" expression rigth hand side of a filter #39688

[github-actions]

⚠️ GitHub issue #39688 has been automatically assigned in GitHub to PR creator.

[Copilot]

🟡 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() and mutate() when symbols like date/day collide 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.

[jonkeane]

I have a few questions on this one

[Copilot]

🔵 Needs a closer look

The newly added mutate regression test doesn’t actually exercise the function-binding-vs-variable resolution scenario it claims to cover, leaving the key behavior under-tested.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

r/tests/testthat/test-dplyr-mutate.R:788

• This new test claims to verify that the day binding is still used when day appears in function position, but lubridate::day(date) bypasses that resolution (it will work even if the unqualified day() binding were broken). To actually cover the GH-39688 scenario, call day(date) unqualified (with lubridate attached so regular dplyr can resolve it) while still using the local day variable as a value.

      mutate(avg_int = mean(int)) |>
      collect(),
    tbl
  )
  # A row limit between arrange() and mutate() still uses the sorted rows

• Files reviewed: 3/3 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

The core fix targets Dataset behavior (GH-39688) but the added tests only exercise the Table path, and the new mask-binding logic adds avoidable per-expression overhead.

Get a fresh assessment by requesting another Copilot review.

Review details

• Files reviewed: 3/3 changed files
• Comments generated: 2
• Review effort level: Lite

[Copilot]

Copilot review overview

🟡 Changes recommended

The fix targets a Dataset-specific failure mode, but the new regression tests appear to exercise only the Table path, so a Dataset regression test should be added to prevent recurrence.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 3

Open (3)

• Add Dataset regression test for name-shadowing in dplyr evaluation · New
• These new regression tests exercise the Table path via compare_dplyr_binding(), but GH-39688…
• shadowed is computed by calling ls(function_env, ...) and intersecting, which scans the entire…

[jonkeane]

Thanks for this, that my_constant test might be slightly duplicative of tests elsewhere, but having that in line is nice and clear that we care about both of those.