Skip to content

fill.tbl_lazy() should set allow_rename = FALSE in eval_select() #1536

Open
@DavisVaughan

Description

@DavisVaughan

i.e. right here:

cols_to_fill <- tidyselect::eval_select(expr(c(...)), .data)

I'm 99% sure that adding allow_rename = FALSE is correct, as we also do it in tidyr's data frame method.

I think it would have caught some weirdness like this:

library(dbplyr)
library(tidyr) # dev tidyr with PR #1572

df <- data.frame(x = c(1, 2), y = c(2, 2), z = c(1, NA))

df_sqlite <- tbl_lazy(df, con = simulate_sqlite())

df_sqlite |> 
  window_order(x) |> 
  fill(.by = y)
#> <SQL>
#> SELECT `x`, `y`, `z`
#> FROM (
#>   SELECT
#>     `df`.*,
#>     SUM(CASE WHEN ((`.by` IS NULL)) THEN 0 ELSE 1 END) OVER (ORDER BY `x` ROWS UNBOUNDED PRECEDING) AS `..dbplyr_partition_1`
#>   FROM `df`
#> ) AS `q01`

This is a very specific case where in tidyverse/tidyr#1572 we are adding a new .by argument to the generic of fill() that dbplyr doesn't know about yet.

  • In the method, .by = y comes through the ... of fill.tbl_lazy() and should get immediately caught by eval_select(allow_rename = FALSE), but isn't
  • In the generic, .by = y comes through the .by = argument in the fill() generic, so it happens to not trigger check_dots_unnamed() in the generic

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions