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.

Sign up for GitHub

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

Jump to bottom

translate_sql() fails when the target for translation starts with a dot #1529

Open
MichaelChirico opened this issue Aug 1, 2024 · 3 comments · May be fixed by #1530
Open

translate_sql() fails when the target for translation starts with a dot #1529

MichaelChirico opened this issue Aug 1, 2024 · 3 comments · May be fixed by #1530

Comments

@MichaelChirico
Copy link
Contributor

I want to add a translation for .POSIXct() --> TIMESTAMP_FROM_UNIX_SECONDS():

https://cloud.google.com/dataflow/docs/reference/sql/timestamp_functions#timestamp_from_unix_seconds

However, the leading . appears to be throwing everything off. I added a simple placeholder translation in this branch:

https://github.com/MichaelChirico/dbplyr/tree/leading-dot

.POSIXct = sql_prefix("foo", 1)

With that, I get a failed translation:

library(dbplyr)
con=simulate_dbi()
translate_sql(.POSIXct(0), con=con)
# <SQL> .POSIXct(0.0)

(should be foo())

I looked into this a bit and I got lost in the details of the implementation. sql_variant() seems to be doing just fine, as base_scalar does contain the correct object still:

evalq(exists(".POSIXct", base_scalar), asNamespace("dbplyr"))
# [1] TRUE

However, this step apparently fails to find the correct translation:

dbplyr/R/translate-sql.R

Lines 148 to 149 in 860bd6a

mask <- sql_data_mask(x, variant, con = con, window = window)
escape(eval_tidy(x, mask), con = con)

I am not familiar enough with the details of tidy eval to know if there's an easy fix.
@hadley
Copy link
Member

hadley commented Aug 2, 2024

With zero recollection of the code my first guess this is because we use ls() somewhere.

@MichaelChirico
Copy link
Contributor Author

MichaelChirico commented Aug 2, 2024 •
edited
Loading

I had the same instinct. indeed I came across these, but there must be more (possibly in another package?)

dbplyr/R/translate-sql-helpers.R

Line 67 in 860bd6a

missing <- setdiff(ls(aggregate), ls(window))

@MichaelChirico MichaelChirico linked a pull request Aug 2, 2024 that will close this issue
Open
@MichaelChirico
Copy link
Contributor Author

trace() (and devcontainer!) is our friend of course... filed #1530.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Handle dot-prefixed names in custom translation MichaelChirico/dbplyr
2 participants
@hadley @MichaelChirico