Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

filter generates buggy SQL when comparing with list element #1368

Closed
misea opened this issue Sep 21, 2023 · 1 comment · Fixed by #1430
Closed

filter generates buggy SQL when comparing with list element #1368

misea opened this issue Sep 21, 2023 · 1 comment · Fixed by #1430
Labels
bug an unexpected problem or unintended behavior func trans 🌍 Translation of individual functions to SQL

Comments

@misea
Copy link

misea commented Sep 21, 2023

When using tbl via dbplyr, filter generates correct SQL when comparing a column with an R scalar variable, but when using $ syntax to access value in a list, buggy sql is generated. The reprex below produces errors both with Postgres and SQLite connections, though more of the generated SQL is visible with Postgres

library(DBI)
library(dbplyr)

init_table <- function(con) {
  # dbExecute(con, "DROP TABLE a")
  dbExecute(con, "CREATE TABLE a (a_id INTEGER, a_value VARCHAR)")
  dbExecute(con, "INSERT INTO a (a_id, a_value) VALUES (1, 'value 1')")
}

test_filter <- function(con) {
  tbl_a <- tbl(con, "a")
  # This works fine
  id <- 1
  tbl_a %>% filter(a_id == id)
  # This blows up
  l <- list(id = 1)
  tbl_a %>% filter(a_id == l$id)
}

slCon <- DBI::dbConnect(RSQLite::SQLite())
init_table(slCon)
test_filter(slCon)
# SQLite: Error in `collect()`:
# ! Failed to collect lazy table.
# Caused by error:
# ! near "AS": syntax error

# pgCon=dbConnect(RPostgres::Postgres(), host = "...", user = "...")
# init_table(pgCon)
# test_filter(pgCon)
# Error in `collect()`:
# ! Failed to collect lazy table.
# Failed to prepare query: ERROR:  syntax error at or near "AS"
# LINE 3: WHERE ("a_id" = (1.0 AS "id").1.0)

Something is wrong with the SQL quoting in this case.

@hadley
Copy link
Member

hadley commented Nov 2, 2023

Hmmmm, it looks like we're looking up id in the local environment, which doesn't make much sense to me:

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

lf <- lazy_frame(a_id = 1, a_value = "value 1")

id <- 10000
l <- list(id = 1)
lf |> filter(a_id == l$id)
#> <SQL>
#> SELECT `df`.*
#> FROM `df`
#> WHERE (`a_id` = (1.0 AS `id`).10000.0)

Created on 2023-11-02 with reprex v2.0.2

You can work around the problem by explicitly requesting local computation:

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

lf <- lazy_frame(a_id = 1, a_value = "value 1")

l <- list(id = 1)
lf |> filter(a_id == local(l$id))
#> <SQL>
#> SELECT `df`.*
#> FROM `df`
#> WHERE (`a_id` = 1.0)

Created on 2023-11-02 with reprex v2.0.2

@hadley hadley added bug an unexpected problem or unintended behavior func trans 🌍 Translation of individual functions to SQL labels Nov 2, 2023
hadley added a commit that referenced this issue Dec 22, 2023
hadley added a commit that referenced this issue Feb 14, 2024
hadley added a commit that referenced this issue Feb 22, 2024
Partially reverts #1368. See bcgov/bcdata#338 for details. If we do this again in the future, I think we have to do argument checking in individual translations. That's a lot of work but allows a whole suite of potentially useful translations for more complex R objects.
hadley added a commit that referenced this issue Feb 28, 2024
Revert to previous escaping method (i.e. reverts #1368). See bcgov/bcdata#338 for details. If we do this again in the future, I think we have to do argument checking in individual translations. That's a lot of work but allows a whole suite of potentially useful translations for more complex R objects.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug an unexpected problem or unintended behavior func trans 🌍 Translation of individual functions to SQL
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants