Skip to content

.keep_all=TRUE breaks distinct for MS SQL Server #1053

Closed
#1267
Closed
.keep_all=TRUE breaks distinct for MS SQL Server#1053
#1267
Labels
bugan unexpected problem or unintended behaviordplyr verbs 🤖Translation of dplyr verbs to SQL
Milestone
2.4.0

Description

@carlganz
carlganz
opened on Nov 23, 2022

It seems to work with SQLite but the use of row_number in distinct(.keep_all=TRUE) isn't quite right for MS SQL Server

library(dplyr)
library(dbplyr)

### need to setup your own SQL Server connection
con <- DBI::dbConnect(odbc::odbc)
copy_to(con, mtcars)
mtcars2 <- tbl(con, "mtcars")

### distinct works as expected
mtcars2 %>% distinct()

### gives error "The function 'ROW_NUMBER' must have an OVER clause with ORDER BY"
mtcars2 %>% distinct(carb, .keep_all = TRUE)

Activity

  1. hadley commented on Dec 5, 2022

    @hadley
    hadley
    on Dec 5, 2022
    Member

    Minimal reprex:

    library(dbplyr)
library(dplyr, warn.conflicts = FALSE)

lf <- lazy_frame(x = 1, y = 2, con = simulate_mysql())

lf |> distinct(x)
#> <SQL>
#> SELECT DISTINCT `x`
#> FROM `df`
lf |> distinct(x, .keep_all = TRUE)
#> <SQL>
#> SELECT `x`, `y`
#> FROM (
#>   SELECT *, ROW_NUMBER() OVER (PARTITION BY `x`) AS `q01`
#>   FROM `df`
#> ) `q01`
#> WHERE (`q01` = 1)

    Created on 2022-12-04 with reprex v2.0.2

  2. hadley commented on Dec 5, 2022

    @hadley
    hadley
    on Dec 5, 2022
    Member

    I suspect this will work in very few places since ROW_NUMBER() will always need an ORDER BY. Maybe we should just make this an error?

  3. hadley
    added
    bugan unexpected problem or unintended behavior
    dplyr verbs 🤖Translation of dplyr verbs to SQL
    on Dec 5, 2022

  4. mgirlich commented on Apr 26, 2023

    @mgirlich
    mgirlich
    on Apr 26, 2023
    Collaborator

    Apparently, it works in most databases according to modern SQL - of course it doesn't work in MS SQL and Oracle DB... If we want to support this, we could add a dummy window order. @hadley what do you think?

  5. hadley commented on Apr 26, 2023

    @hadley
    hadley
    on Apr 26, 2023
    Member

    Yeah, it would be nice to add a nice dummy window order, I think.

  6. mgirlich commented on Apr 28, 2023

    @mgirlich
    mgirlich
    on Apr 28, 2023
    Collaborator

    @carlganz As I don't have an SQL Server database I can't test this. Can you install the PR version via remotes::install_github("tidyverse/dbplyr#1267") and test whether this now works?

  7. mgirlich
    added this to the 2.4.0 milestone on Jun 23, 2023
  8. mgirlich
    closed this as completedin #1267on Jul 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugan unexpected problem or unintended behaviordplyr verbs 🤖Translation of dplyr verbs to SQL

    Type

    No type

    Projects

    No projects

      Milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions