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

Regression: migrating from build_sql() to glue_sql2() lost escaping #1351

Closed
fh-mthomson opened this issue Aug 10, 2023 · 6 comments · Fixed by #1352
Closed

Regression: migrating from build_sql() to glue_sql2() lost escaping #1351

fh-mthomson opened this issue Aug 10, 2023 · 6 comments · Fixed by #1352

Comments

@fh-mthomson
Copy link
Contributor

fh-mthomson commented Aug 10, 2023

Problem summary

In #1228, most references to build_sql() where replaced with glue_sql2().

This subtly broke some fundamental SQL translations, since escape() methods were no longer being applied.

Reprex (using Snowflake as my motivating example)

Previously:

local_con(dbplyr::simulate_snowflake())
months = function(x) {
  build_sql("INTERVAL '", x, " month'")
}
months(1)
  <SQL> INTERVAL '1.0 month'

current translation, which fails:

months = function(x) {
  glue_sql2(sql_current_con(), "INTERVAL '{x} month'")
}
months(1)

Error in `.transformer()`:
! `value` must be a string or scalar SQL, not the number 1.
Run `rlang::last_trace()` to see where the error occurred.

Solutions considered

  1. [hacky] Manually add escape(x) in applicable calls to glue_sql2() (e.g., add escaping for Snowflake interval functions #1350 to mitigate breaking Snowflake translations)
  2. [robust] Migrate relevant escape() functionality from build_sql() to gluesql2() (https://github.com/tidyverse/dbplyr/blob/main/R/build-sql.R#L32-L44)
@fh-mthomson
Copy link
Contributor Author

A simple Postgres translation also suffers from this error, when previously it was acceptable, suggesting a broader issue:

local_con(simulate_dbi())
test_translate_sql(if_else(x, -1, 2))

@fh-mthomson
Copy link
Contributor Author

Ah, yea, the straight glue here removes the escaping: 83045c5#diff-1d16344a03920504ed530dc7fa4a10614ffc383ba2c5762d18706280747d1f97R102

@fh-mthomson
Copy link
Contributor Author

Another (slightly more obscure) example:

mf <- lazy_frame(x = c(1:5), y = c(rep("A", 5)), con = simulate_snowflake())
> mf %>% dplyr::summarize(n = n_distinct(y))
<SQL>
SELECT COUNT(DISTINCT `y`) AS `n`
FROM `df`
> mf %>% dplyr::summarize(n = n_distinct(1))
Error in `.transformer()`:
! `value` must be a string or scalar SQL, not the number 1.
Run `rlang::last_trace()` to see where the error occurred.

@fh-mthomson fh-mthomson changed the title Regression: migrating from build_sql() to glue_sql2() lost escaping to characters Regression: migrating from build_sql() to glue_sql2() lost escaping Aug 11, 2023
@mgirlich
Copy link
Collaborator

I checked all uses of glue_sql2() and found different issues:

  1. enpar() didn't work for some special cases, e.g. enpar(-1).
  2. Some translations don't work but I don't think they make sense, e.g.
con <- simulate_dbi()
translate_sql(x[1L], con = con)
translate_sql(desc(1), con = con)
translate_sql(n_distinct(1), con = con)

We can fix them but is there a valid use case for them?

  1. Snowflake date interval functions

@mgirlich
Copy link
Collaborator

Note that glue_sql2() already provides several helpers for escaping. This was part of the motivation to switch to glue_sql2() instead of build_sql().

@fh-mthomson
Copy link
Contributor Author

Thank you, great call! Leaving (2) aside seems fine to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants