Translate evaluated expression #674
Conversation
R/translate-sql-helpers.R
Outdated
| build_sql(x, sql(f), y) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| escape_infix_expr <- function(x) { | ||
| infix_calls <- c("+", "-", "*", "/", "%%", "^") | ||
| if (quo_is_call(x, n = 2) && call_name(x) %in% infix_calls) { |
There was a problem hiding this comment.
I think quo_is_call(x, name, n = 2) will work?
And I'm not sure n = 2 is correct? Doesn't the same problem arise with unary + and -?
There was a problem hiding this comment.
Hmm, when do we need parantheses for unary + and -? I came up with something like
translate_sql(-!!expr(-x))
#> <SQL> --`x`Do you see other cases?
Thanks to your question I just noticed that unary + isn't supported at all currently:
library(dplyr, warn.conflicts = FALSE)
library(dbplyr, warn.conflicts = FALSE)
# works
translate_sql(-2)
#> <SQL> -2.0
# error
translate_sql(+2)
#> Error in eval_bare(x, .env): argument "y" is missing, with no defaultCreated on 2021-12-09 by the reprex package (v2.0.1)
There was a problem hiding this comment.
Not sure how/whether we want to handle translate_sql(-!!expr(-x)). First the inner part, -x, translated to SQL -`x` . Then the outer minus is translated. Here one would have to analyse the SQL to decide whether parantheses are needed.
Probably it should be sufficient to check if the SQL starts with "-". What do you think?
There was a problem hiding this comment.
I was thinking of this case:
dbplyr::translate_sql(-!!rlang::expr(1+2))
#> <SQL> -1.0 + 2.0Created on 2021-12-09 by the reprex package (v2.0.1)
OTOH is this very common? Should we just suggest that if you're going to interpolate in expressions involving infix operators, you should wrap the whole thing in parentheses to avoid operator precedence issues?
|
I think this slightly changed approach should handle binary and unary minus. |
Co-authored-by: Hadley Wickham <h.wickham@gmail.com>
Co-authored-by: Hadley Wickham <h.wickham@gmail.com>
|
I forgot to say this earlier, but can you please do squash-merges? I think that gives a cleaner history, and in most cases we don't need to preserve the history within in a PR. |
Fixes #634.
I think the problem of partially evaluated expressions should only be an issue for infix expressions. I wasn't sure about adding operators like
|,&&, or==to the list but it might make sense as well.