Skip to content

Commit

Permalink
Fix across() in combination with summarise(.by) (#1494)
Browse files Browse the repository at this point in the history
  • Loading branch information
mgirlich authored May 2, 2024
1 parent f3baed5 commit 8f2fcb0
Show file tree
Hide file tree
Showing 6 changed files with 30 additions and 7 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)

* `across(everything())` doesn't select grouping columns created via `.by` in
`summarise()` (@mgirlich, #1493).

# dbplyr 2.5.0

## Improved tools for qualified table names
Expand Down
3 changes: 2 additions & 1 deletion R/verb-summarise.R
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@
#' show_query()
summarise.tbl_lazy <- function(.data, ..., .by = NULL, .groups = NULL) {
check_groups(.groups)
dots <- summarise_eval_dots(.data, ...)

by <- compute_by(
{{ .by }},
Expand All @@ -49,6 +48,8 @@ summarise.tbl_lazy <- function(.data, ..., .by = NULL, .groups = NULL) {
.data$lazy_query$group_vars <- by$names
.groups <- "drop"
}

dots <- summarise_eval_dots(.data, ...)
.data$lazy_query <- add_summarise(
.data, dots,
.groups = .groups,
Expand Down
6 changes: 1 addition & 5 deletions tests/testthat/_snaps/tbl-sql.md
Original file line number Diff line number Diff line change
Expand Up @@ -28,12 +28,8 @@
# check_from is deprecated

Code
tbl(con, "x", check_from = FALSE)
out <- tbl(con, "x", check_from = FALSE)
Condition
Warning:
The `check_from` argument of `tbl_sql()` is deprecated as of dbplyr 2.5.0.
Output
# Source: table<`x`> [0 x 1]
# Database: sqlite 3.45.0 [:memory:]
# i 1 variable: y <lgl>

10 changes: 10 additions & 0 deletions tests/testthat/_snaps/verb-summarise.md
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,16 @@
FROM `df`
GROUP BY `g`

# across doesn't select columns from `.by` #1493

Code
out
Output
<SQL>
SELECT `g`, SUM(`..x`) AS `x`
FROM `df`
GROUP BY `g`

# can't use `.by` with `.groups`

Code
Expand Down
2 changes: 1 addition & 1 deletion tests/testthat/test-tbl-sql.R
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ test_that("check_from is deprecated", {
con <- local_sqlite_connection()
DBI::dbExecute(con, "CREATE TABLE x (y)")

expect_snapshot(tbl(con, "x", check_from = FALSE))
expect_snapshot(out <- tbl(con, "x", check_from = FALSE))
})

# n_groups ----------------------------------------------------------------
Expand Down
13 changes: 13 additions & 0 deletions tests/testthat/test-verb-summarise.R
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,19 @@ test_that("can group transiently using `.by`", {
expect_equal(group_vars(out), character())
})

test_that("across doesn't select columns from `.by` #1493", {
lf <- lazy_frame(g = 1, x = 1)

out <- lf %>%
summarise(
across(everything(), ~ sum(..x, na.rm = TRUE)),
.by = g
)

expect_snapshot(out)
expect_equal(sql_build(out)$select[1], "`g`")
})

test_that("can't use `.by` with `.groups`", {
df <- lazy_frame(x = 1)

Expand Down

0 comments on commit 8f2fcb0

Please sign in to comment.