Simplify escaping strategy (#1396)

The big idea in this PR is to simplify the implementation by immediately escaping as soon as we get a table name. That allows user facing functions to accept a wide range of table specifications while internal functions only need to deal with a new `table_path` class. Additionally, only the `table_path` class is vectorised, so we can ensure that user supplies a single table name (or SQL, where appropriate), while still providing vectorised tools internally. Since we escape table ids early we also need a new tool to extract the name of just the table; this is sort of the opposite problem to `table_ident` which needed tools to stitch components together instead of pulling them apart.

I've also simplified the implementation a little by taking `I()` to mean "don't escape". My expectation is that `I("foo.bar")`/`I("foo.bar.baz"))` will become the preferred way to specify nested tables.

Fixes #1388. Fixes #1396. Fixes #1413. Fixes #1408. Closes #1385. Fixes #1416. Fixes #1404.

DESCRIPTION

@@ -43,7 +43,7 @@ Suggests:
     knitr,
     Lahman,
     nycflights13,
-    odbc,
+    odbc (>= 1.4.2),
     RMariaDB (>= 1.2.2),
     rmarkdown,
     RPostgres (>= 1.4.5),
@@ -128,7 +128,7 @@ Collate:
     'sql-expr.R'
     'src-sql.R'
     'src_dbi.R'
-    'table-ident.R'
+    'table-name.R'
     'tbl-lazy.R'
     'tbl-sql.R'
     'test-frame.R'

NAMESPACE

@@ -1,6 +1,8 @@
 # Generated by roxygen2: do not edit by hand
 
 S3method("$",tbl_lazy)
+S3method("[",dbplyr_table_path)
+S3method("[[",dbplyr_table_path)
 S3method(add_count,tbl_lazy)
 S3method(anti_join,tbl_lazy)
 S3method(arrange,tbl_lazy)
@@ -12,15 +14,8 @@ S3method(as.sql,dbplyr_catalog)
 S3method(as.sql,dbplyr_schema)
 S3method(as.sql,ident)
 S3method(as.sql,sql)
-S3method(as_table_ident,Id)
-S3method(as_table_ident,character)
-S3method(as_table_ident,dbplyr_catalog)
-S3method(as_table_ident,dbplyr_schema)
-S3method(as_table_ident,dbplyr_table_ident)
-S3method(as_table_ident,ident)
-S3method(as_table_ident,ident_q)
-S3method(as_table_ident,sql)
 S3method(auto_copy,tbl_sql)
+S3method(c,dbplyr_table_path)
 S3method(c,ident)
 S3method(c,sql)
 S3method(collapse,tbl_sql)
@@ -128,7 +123,7 @@ S3method(escape,blob)
 S3method(escape,character)
 S3method(escape,dbplyr_catalog)
 S3method(escape,dbplyr_schema)
-S3method(escape,dbplyr_table_ident)
+S3method(escape,dbplyr_table_path)
 S3method(escape,double)
 S3method(escape,factor)
 S3method(escape,ident)
@@ -149,7 +144,6 @@ S3method(flatten_query,union_query)
 S3method(flatten_query,values_query)
 S3method(format,dbplyr_catalog)
 S3method(format,dbplyr_schema)
-S3method(format,dbplyr_table_ident)
 S3method(format,ident)
 S3method(format,sql)
 S3method(format,src_sql)
@@ -186,6 +180,7 @@ S3method(print,base_query)
 S3method(print,dbplyr_catalog)
 S3method(print,dbplyr_schema)
 S3method(print,dbplyr_sql_options)
+S3method(print,dbplyr_table_path)
 S3method(print,ident)
 S3method(print,join_query)
 S3method(print,lazy_base_local_query)

NEWS.md

@@ -1,5 +1,27 @@
 # dbplyr (development version)
 
+* Specification of table names with schema/catalogs has been overhauled to
+  make it simpler. This includes the following features and fixes:
+
+  * The simplest way to refer to a qualified table is now to wrap it in 
+    `I()`, e.g. `I("schema_name.table_name")`.
+
+  * Use of `sql()` and `ident_q()` inside `in_catalog()` and `in_schema()`
+    is once again supported (#1388).
+
+  * It's ok to use `ident_q()` once again (#1413) and you should no longer
+    see unsuppressable warnings about using `in_schema()` (#1408).
+
+  * The names of the arguments to `Id()` no longer matter, only their order 
+    (#1416). Additionally, thanks to changes to the DBI package, you no 
+    longer need to name each argument.
+
+  * If you accidentally pass a named vector to any of the database identifer
+    functions, those names will be automatically stripped (#1404).
+
+* When dbplyr creates an index on a table in a schema (e.g. `schema.table`), 
+  it now only includes the table name in the index name, not the schema name.
+
 * The databricks backend now supports creating non-temporary tables too (#1418).
 
 * Clearer error if you attempt to embed non-atomic vectors inside of a generated 

R/backend-hana.R

@@ -58,12 +58,8 @@ sql_translation.HDB <- function(con) {
 # nocov start
 #' @export
 db_table_temporary.HDB <- function(con, table, temporary, ...) {
-  if (temporary && substr(table, 1, 1) != "#") {
-    table <- hash_temp(table)
-  }
-
   list(
-    table = table,
+    table = add_temporary_prefix(con, table, temporary = temporary),
     temporary = FALSE
   )
 }

R/backend-mssql.R

@@ -510,20 +510,13 @@ mssql_version <- function(con) {
 # <https://docs.microsoft.com/en-us/previous-versions/sql/sql-server-2008-r2/ms177399%28v%3dsql.105%29#temporary-tables>
 #' @export
 `db_table_temporary.Microsoft SQL Server` <- function(con, table, temporary, ...) {
-  table <- as_table_ident(table)
-  table_name <- vctrs::field(table, "table")
-
-  if (temporary && substr(table_name, 1, 1) != "#") {
-    table_name <- hash_temp(table_name)
-    vctrs::field(table, "table") <- table_name
-  }
-
   list(
-    table = table,
+    table = add_temporary_prefix(con, table, temporary = temporary),
     temporary = FALSE
   )
 }
 
+
 #' @export
 `sql_query_save.Microsoft SQL Server` <- function(con,
                                                   sql,

R/backend-mysql.R

@@ -56,7 +56,7 @@ db_connection_describe.MySQLConnection <- db_connection_describe.MariaDBConnecti
 
 #' @export
 db_col_types.MariaDBConnection <- function(con, table, call) {
-  table <- as_table_ident(table, error_call = call)
+  table <- as_table_path(table, con, error_call = call)
   col_info_df <- DBI::dbGetQuery(con, glue_sql2(con, "SHOW COLUMNS FROM {.tbl table};"))
   set_names(col_info_df[["Type"]], col_info_df[["Field"]])
 }

R/backend-postgres-old.R

@@ -15,7 +15,6 @@ db_write_table.PostgreSQLConnection <- function(con,
                                                 values,
                                                 temporary = TRUE,
                                                 ...) {
-  table <- as_table_ident(table)
   if (!isFALSE(temporary)) {
     cli_abort(c(
       "RPostgreSQL backend does not support creation of temporary tables",
@@ -27,7 +26,7 @@ db_write_table.PostgreSQLConnection <- function(con,
   # the bare table name
   dbWriteTable(
     con,
-    name = vctrs::field(table, "table"),
+    name = table_name(table, con),
     value = values,
     field.types = types,
     ...,

R/backend-postgres.R

@@ -442,7 +442,7 @@ db_supports_table_alias_with_as.PostgreSQL <- function(con) {
 
 #' @export
 db_col_types.PqConnection <- function(con, table, call) {
-  table <- as_table_ident(table, error_call = call)
+  table <- as_table_path(table, con, error_call = call)
   res <- DBI::dbSendQuery(con, glue_sql2(con, "SELECT * FROM {.tbl table} LIMIT 0"))
   on.exit(DBI::dbClearResult(res))
   DBI::dbFetch(res, n = 0)

R/backend-spark-sql.R

@@ -150,7 +150,6 @@ simulate_spark_sql <- function() simulate_dbi("Spark SQL")
     cli::cli_abort("Spark SQL only support temporary tables")
   }
 
-  table <- as_table_ident(table)
   sql <- glue_sql2(
     con,
     "CREATE ", if (overwrite) "OR REPLACE ",

R/build-sql.R

@@ -128,9 +128,9 @@ sql_quote_transformer <- function(connection) {
     glue_check_collapse(type, should_collapse)
 
     if (type == "tbl") {
-      value <- as_table_ident(value)
+      value <- as_table_path(value, connection)
     } else if (type == "from") {
-      value <- as_from(value)
+      value <- as_table_source(value, connection)
     } else if (type == "col") {
       if (is_bare_character(value)) {
         value <- ident(value)

R/db-io.R

@@ -42,7 +42,7 @@ db_copy_to <-  function(con,
                         indexes = NULL,
                         analyze = TRUE,
                         in_transaction = TRUE) {
-  as_table_ident(table)
+  check_table_id(table)
   check_bool(overwrite)
   check_character(types, allow_null = TRUE)
   check_named(types)
@@ -65,10 +65,11 @@ db_copy_to.DBIConnection <- function(con,
                                      indexes = NULL,
                                      analyze = TRUE,
                                      in_transaction = TRUE) {
-  table <- as_table_ident(table)
+  table <- as_table_path(table, con)
   new <- db_table_temporary(con, table, temporary)
   table <- new$table
   temporary <- new$temporary
+  call <- current_env()
 
   with_transaction(
     con,
@@ -103,7 +104,7 @@ db_compute <- function(con,
                        indexes = list(),
                        analyze = TRUE,
                        in_transaction = TRUE) {
-  as_table_ident(table)
+  check_table_id(table)
   check_scalar_sql(sql)
   check_bool(overwrite)
   check_bool(temporary)
@@ -123,7 +124,7 @@ db_compute.DBIConnection <- function(con,
                                      indexes = list(),
                                      analyze = TRUE,
                                      in_transaction = FALSE) {
-  table <- as_table_ident(table)
+  table <- as_table_path(table, con)
   new <- db_table_temporary(con, table, temporary)
   table <- new$table
   temporary <- new$temporary
@@ -179,7 +180,7 @@ db_write_table.DBIConnection <- function(con,
                                          temporary = TRUE,
                                          ...,
                                          overwrite = FALSE) {
-  table <- as_table_ident(table)
+  check_table_path(table)
   check_character(types, allow_null = TRUE)
   check_named(types)
   check_bool(temporary)
@@ -188,7 +189,7 @@ db_write_table.DBIConnection <- function(con,
   withCallingHandlers(
     dbWriteTable(
       con,
-      name = table_ident_to_id(table),
+      name = SQL(table),
       value = values,
       field.types = types,
       temporary = temporary,