Skip to content

Commit

Permalink
Wrap translation of if_any/all() in parentheses (#1257)
Browse files Browse the repository at this point in the history
  • Loading branch information
mgirlich authored Apr 25, 2023
1 parent c65c820 commit 23c5f36
Show file tree
Hide file tree
Showing 5 changed files with 29 additions and 15 deletions.
3 changes: 3 additions & 0 deletions NEWS.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
# dbplyr (development version)

* The translation of `if_any()` and `if_all()` is now wrapped in parentheses.
This makes sure it can be combined via `&` with other conditions (@mgirlich, #1153).

* `pivot_wider()` now matches tidyr `NA` column handling (@ejneer #1238).

* Using a function with a namespace in `across()` now works, e.g.
Expand Down
4 changes: 2 additions & 2 deletions R/tidyeval-across.R
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,8 @@ partial_eval_if <- function(call, data, env, reduce = "&", error_call = caller_e
} else {
fn <- "if_any()"
}
out <- across_setup(data, call, env, allow_rename = FALSE, fn = fn, error_call = error_call)
Reduce(function(x, y) call2(reduce, x, y), out)
conditions <- across_setup(data, call, env, allow_rename = FALSE, fn = fn, error_call = error_call)
Reduce(function(x, y) call2(reduce, x, y), conditions)
}

deprecate_across_dots <- function(call, env, user_env) {
Expand Down
6 changes: 4 additions & 2 deletions R/tidyeval.R
Original file line number Diff line number Diff line change
Expand Up @@ -73,9 +73,11 @@ partial_eval <- function(call, data, env = caller_env(), vars = NULL, error_call
} else if (is_quosure(call)) {
partial_eval(get_expr(call), data, get_env(call), error_call = error_call)
} else if (is_call(call, "if_any")) {
partial_eval_if(call, data, env, reduce = "|", error_call = error_call)
out <- partial_eval_if(call, data, env, reduce = "|", error_call = error_call)
expr(((!!out)))
} else if (is_call(call, "if_all")) {
partial_eval_if(call, data, env, reduce = "&", error_call = error_call)
out <- partial_eval_if(call, data, env, reduce = "&", error_call = error_call)
expr(((!!out)))
} else if (is_call(call, "across")) {
partial_eval_across(call, data, env, error_call)
} else if (is_call(call, "pick")) {
Expand Down
16 changes: 8 additions & 8 deletions tests/testthat/_snaps/tidyeval-across.md
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@
<SQL>
SELECT *
FROM `df`
WHERE (`a` > 0.0 AND `b` > 0.0)
WHERE ((`a` > 0.0 AND `b` > 0.0))

---

Expand All @@ -154,15 +154,15 @@
<SQL>
SELECT *
FROM `df`
WHERE (`a` > 0.0 OR `b` > 0.0)
WHERE ((`a` > 0.0 OR `b` > 0.0))

# if_all/any works in mutate()

Code
lf %>% mutate(c = if_all(a:b, ~ . > 0))
Output
<SQL>
SELECT *, `a` > 0.0 AND `b` > 0.0 AS `c`
SELECT *, (`a` > 0.0 AND `b` > 0.0) AS `c`
FROM `df`

---
Expand All @@ -171,7 +171,7 @@
lf %>% mutate(c = if_any(a:b, ~ . > 0))
Output
<SQL>
SELECT *, `a` > 0.0 OR `b` > 0.0 AS `c`
SELECT *, (`a` > 0.0 OR `b` > 0.0) AS `c`
FROM `df`

# if_all/any uses every colum as default
Expand All @@ -182,7 +182,7 @@
<SQL>
SELECT *
FROM `df`
WHERE (`a` > 0.0 AND `b` > 0.0)
WHERE ((`a` > 0.0 AND `b` > 0.0))

---

Expand All @@ -192,7 +192,7 @@
<SQL>
SELECT *
FROM `df`
WHERE (`a` > 0.0 OR `b` > 0.0)
WHERE ((`a` > 0.0 OR `b` > 0.0))

# if_all/any works without `.fns` argument

Expand All @@ -202,7 +202,7 @@
<SQL>
SELECT *
FROM `df`
WHERE (`a` AND `b`)
WHERE ((`a` AND `b`))

---

Expand All @@ -212,7 +212,7 @@
<SQL>
SELECT *
FROM `df`
WHERE (`a` OR `b`)
WHERE ((`a` OR `b`))

# if_all() cannot rename variables

Expand Down
15 changes: 12 additions & 3 deletions tests/testthat/test-tidyeval-across.R
Original file line number Diff line number Diff line change
Expand Up @@ -252,7 +252,7 @@ test_that("across() uses environment from the current quosure (dplyr#5460)", {

expect_equal(
partial_eval_dots(lf, if_all(all_of(y), ~ .x < 2)),
list(quo(x < 2)),
list(quo((x < 2))),
ignore_attr = "names"
)
})
Expand Down Expand Up @@ -448,6 +448,15 @@ test_that("if_all/any works in filter()", {
expect_snapshot(lf %>% filter(if_any(a:b, ~ . > 0)))
})

test_that("if_all/any is wrapped in parantheses #1153", {
lf <- lazy_frame(a = 1, b = 2, c = 3)

expect_equal(
lf %>% filter(if_any(c(a, b)) & c == 3) %>% remote_query(),
sql("SELECT *\nFROM `df`\nWHERE ((`a` OR `b`) AND `c` = 3.0)")
)
})

test_that("if_all/any works in mutate()", {
lf <- lazy_frame(a = 1, b = 2)

Expand Down Expand Up @@ -488,11 +497,11 @@ test_that("if_any() and if_all() expansions deal with single inputs", {
# Single inputs
expect_equal(
filter(d, if_any(x, ~ FALSE)) %>% remote_query(),
sql("SELECT *\nFROM `df`\nWHERE (FALSE)")
sql("SELECT *\nFROM `df`\nWHERE ((FALSE))")
)
expect_equal(
filter(d, if_all(x, ~ FALSE)) %>% remote_query(),
sql("SELECT *\nFROM `df`\nWHERE (FALSE)")
sql("SELECT *\nFROM `df`\nWHERE ((FALSE))")
)
})

Expand Down

0 comments on commit 23c5f36

Please sign in to comment.