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

Improve $ handling #1430

Merged
merged 9 commits into from
Feb 14, 2024
Merged

Improve $ handling #1430

merged 9 commits into from
Feb 14, 2024

Conversation

hadley
Copy link
Member

@hadley hadley commented Dec 22, 2023

Fixes #1368

@hadley hadley requested a review from mgirlich December 22, 2023 17:30
R/tidyeval.R Outdated
Comment on lines 161 to 164
} else if (is_bare_list(val)) {
error_embed("a list", name)
} else {
val
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should make this stricter and only inline atomic vectors?

@mgirlich
Copy link
Collaborator

We have an explicit error for lists but it also doesn't work for simple vectors, e.g.

devtools::load_all("~/GitHub/dbplyr/")

y <- c(y = 2)
lazy_frame(x = 1) %>% 
  filter(x == y$x)
#> Error in `.transformer()`:
#> ! `value` must be a string or scalar SQL, not the number 2.
#> Backtrace:
#>      ▆
#>   1. ├─base::tryCatch(...)
#>   2. │ └─base (local) tryCatchList(expr, classes, parentenv, handlers)
#>   3. │   ├─base (local) tryCatchOne(...)
#>   4. │   │ └─base (local) doTryCatch(return(expr), name, parentenv, handler)
#>   5. │   └─base (local) tryCatchList(expr, names[-nh], parentenv, handlers[-nh])
#>   6. │     └─base (local) tryCatchOne(expr, names, parentenv, handlers[[1L]])
#>   7. │       └─base (local) doTryCatch(return(expr), name, parentenv, handler)
#>   8. ├─base::withCallingHandlers(...)
#>   9. ├─base::saveRDS(...)
#>  10. ├─base::do.call(...)
#>  11. ├─base (local) `<fn>`(...)
#>  12. ├─global `<fn>`(input = base::quote("smoky-moose_reprex.R"))
#>  13. │ └─rmarkdown::render(input, quiet = TRUE, envir = globalenv(), encoding = "UTF-8")
#>  14. │   └─knitr::knit(knit_input, knit_output, envir = envir, quiet = quiet)
#>  15. │     └─knitr:::process_file(text, output)
#>  16. │       ├─knitr:::handle_error(...)
#>  17. │       │ └─base::withCallingHandlers(...)
#>  18. │       ├─base::withCallingHandlers(...)
#>  19. │       ├─knitr:::process_group(group)
#>  20. │       └─knitr:::process_group.block(group)
#>  21. │         └─knitr:::call_block(x)
#>  22. │           └─knitr:::block_exec(params)
#>  23. │             └─knitr:::eng_r(options)
#>  24. │               ├─knitr:::in_input_dir(...)
#>  25. │               │ └─knitr:::in_dir(input_dir(), expr)
#>  26. │               └─knitr (local) evaluate(...)
#>  27. │                 └─evaluate::evaluate(...)
#>  28. │                   └─evaluate:::evaluate_call(...)
#>  29. │                     ├─evaluate (local) handle(...)
#>  30. │                     │ └─base::try(f, silent = TRUE)
#>  31. │                     │   └─base::tryCatch(...)
#>  32. │                     │     └─base (local) tryCatchList(expr, classes, parentenv, handlers)
#>  33. │                     │       └─base (local) tryCatchOne(expr, names, parentenv, handlers[[1L]])
#>  34. │                     │         └─base (local) doTryCatch(return(expr), name, parentenv, handler)
#>  35. │                     ├─base::withCallingHandlers(...)
#>  36. │                     ├─base::withVisible(value_fun(ev$value, ev$visible))
#>  37. │                     └─knitr (local) value_fun(ev$value, ev$visible)
#>  38. │                       └─knitr (local) fun(x, options = options)
#>  39. │                         ├─base::withVisible(knit_print(x, ...))
#>  40. │                         ├─knitr::knit_print(x, ...)
#>  41. │                         └─knitr:::knit_print.default(x, ...)
#>  42. │                           └─evaluate (local) normal_print(x)
#>  43. │                             ├─base::print(x)
#>  44. │                             └─dbplyr:::print.tbl_lazy(x)
#>  45. │                               ├─dplyr::show_query(x) at dbplyr/R/tbl-lazy.R:47:3
#>  46. │                               └─dbplyr:::show_query.tbl_lazy(x)
#>  47. │                                 └─dbplyr::remote_query(x, cte = cte, sql_options = sql_options) at dbplyr/R/explain.R:5:3
#>  48. │                                   └─dbplyr::db_sql_render(remote_con(x), x, cte = cte, sql_options = sql_options) at dbplyr/R/remote.R:94:3
#>  49. │                                     ├─dbplyr::db_sql_render(con, sql, sql_options = sql_options) at dbplyr/R/db.R:76:5
#>  50. │                                     └─dbplyr:::db_sql_render.DBIConnection(con, sql, sql_options = sql_options) at dbplyr/R/db.R:80:3
#>  51. │                                       ├─dbplyr::sql_render(sql, con = con, ..., sql_options = sql_options) at dbplyr/R/db.R:84:3
#>  52. │                                       └─dbplyr:::sql_render.tbl_lazy(sql, con = con, ..., sql_options = sql_options) at dbplyr/R/sql-build.R:90:3
#>  53. │                                         ├─dbplyr::sql_render(...) at dbplyr/R/sql-build.R:101:3
#>  54. │                                         └─dbplyr:::sql_render.lazy_query(...) at dbplyr/R/sql-build.R:90:3
#>  55. │                                           ├─dbplyr::sql_build(query, con = con, sql_options = sql_options) at dbplyr/R/sql-build.R:118:3
#>  56. │                                           └─dbplyr:::sql_build.lazy_select_query(query, con = con, sql_options = sql_options) at dbplyr/R/sql-build.R:43:3
#>  57. │                                             └─dbplyr::translate_sql_(op$where, con = con, context = list(clause = "WHERE")) at dbplyr/R/lazy-select-query.R:174:3
#>  58. │                                               └─base::lapply(...) at dbplyr/R/translate-sql.R:142:3
#>  59. │                                                 └─dbplyr (local) FUN(X[[i]], ...)
#>  60. │                                                   ├─dbplyr::escape(eval_tidy(x, mask), con = con) at dbplyr/R/translate-sql.R:149:7
#>  61. │                                                   └─rlang::eval_tidy(x, mask) at dbplyr/R/escape.R:37:3
#>  62. ├─x == `<dbl>`$x
#>  63. │ └─dbplyr:::escape_infix_expr(enexpr(y), y) at dbplyr/R/translate-sql-helpers.R:161:7
#>  64. ├─`<dbl>`$x at dbplyr/R/translate-sql-helpers.R:185:3
#>  65. │ └─dbplyr:::glue_sql2(sql_current_con(), "{x}.{.col name}") at dbplyr/R/backend-.R:65:5
#>  66. │   ├─dbplyr::sql(...) at dbplyr/R/build-sql.R:92:3
#>  67. │   │ └─dbplyr:::c_character(...) at dbplyr/R/sql.R:11:3
#>  68. │   └─glue::glue(...) at dbplyr/R/utils.R:63:3
#>  69. │     └─glue::glue_data(...)
#>  70. └─glue (local) `<fn>`("x")
#>  71.   ├─.transformer(expr, env) %||% .null
#>  72.   └─dbplyr (local) .transformer(text = expr, envir = env)
#>  73.     └─dbplyr:::stop_input_type(value, what = c("a string", "scalar SQL")) at dbplyr/R/build-sql.R:149:9
#>  74.       └─rlang::abort(message, ..., call = call, arg = arg) at dbplyr/R/import-standalone-obj-type.R:337:3

Created on 2023-12-29 with reprex v2.0.2

Is this supposed to work?

@hadley
Copy link
Member Author

hadley commented Jan 10, 2024

@mgirlich I took another pass and made the criteria for inlining a bit stricter, so I think it's worth another look.

@mgirlich
Copy link
Collaborator

LGTM 👍

@hadley hadley merged commit 01df318 into main Feb 14, 2024
13 checks passed
@hadley hadley deleted the list-access branch February 14, 2024 18:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

filter generates buggy SQL when comparing with list element
2 participants