From ae1debb9a1a7423e49c64f4043ae716c48fbfee9 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Tue, 20 Feb 2024 07:39:36 -0600 Subject: [PATCH] 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 | 4 +- NAMESPACE | 15 +- NEWS.md | 22 ++ R/backend-hana.R | 6 +- R/backend-mssql.R | 11 +- R/backend-mysql.R | 2 +- R/backend-postgres-old.R | 3 +- R/backend-postgres.R | 2 +- R/backend-spark-sql.R | 1 - R/build-sql.R | 4 +- R/db-io.R | 13 +- R/db-sql.R | 88 +++---- R/lazy-join-query.R | 26 +- R/lazy-ops.R | 11 +- R/query-join.R | 44 ++-- R/query-select.R | 6 +- R/query-set-op.R | 8 +- R/remote.R | 13 +- R/simulate.R | 6 +- R/sql-build.R | 17 +- R/table-ident.R | 262 --------------------- R/table-name.R | 197 ++++++++++++++++ R/tbl-lazy.R | 4 +- R/tbl-sql.R | 33 +-- R/utils.R | 28 ++- R/verb-compute.R | 2 +- R/verb-copy-to.R | 6 +- R/verb-joins.R | 34 +-- tests/testthat/_snaps/backend-.md | 10 +- tests/testthat/_snaps/backend-mssql.md | 7 + tests/testthat/_snaps/db-io.md | 2 +- tests/testthat/_snaps/db-sql.md | 2 +- tests/testthat/_snaps/lazy-select-query.md | 8 +- tests/testthat/_snaps/query-join.md | 9 +- tests/testthat/_snaps/query-select.md | 3 +- tests/testthat/_snaps/query-semi-join.md | 6 +- tests/testthat/_snaps/query-set-op.md | 9 +- tests/testthat/_snaps/table-ident.md | 109 --------- tests/testthat/_snaps/table-name.md | 52 ++++ tests/testthat/_snaps/verb-joins.md | 8 +- tests/testthat/test-backend-mssql.R | 13 + tests/testthat/test-lazy-select-query.R | 4 +- tests/testthat/test-query-join.R | 8 +- tests/testthat/test-query-select.R | 2 +- tests/testthat/test-remote.R | 20 +- tests/testthat/test-table-ident.R | 144 ----------- tests/testthat/test-table-name.R | 90 +++++++ tests/testthat/test-tbl-lazy.R | 2 +- tests/testthat/test-tbl-sql.R | 14 +- tests/testthat/test-verb-compute.R | 4 +- tests/testthat/test-verb-joins.R | 94 ++++---- tests/testthat/test-verb-set-ops.R | 2 +- vignettes/setup/_postgres.Rmd | 21 +- 53 files changed, 673 insertions(+), 838 deletions(-) delete mode 100644 R/table-ident.R create mode 100644 R/table-name.R delete mode 100644 tests/testthat/_snaps/table-ident.md create mode 100644 tests/testthat/_snaps/table-name.md delete mode 100644 tests/testthat/test-table-ident.R create mode 100644 tests/testthat/test-table-name.R diff --git a/DESCRIPTION b/DESCRIPTION index 563b5ca84..a23c0ea8c 100644 --- a/DESCRIPTION +++ b/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' diff --git a/NAMESPACE b/NAMESPACE index 9061b09c0..79b18ef8a 100644 --- a/NAMESPACE +++ b/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) diff --git a/NEWS.md b/NEWS.md index b1050c0b2..426c953a6 100644 --- a/NEWS.md +++ b/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 diff --git a/R/backend-hana.R b/R/backend-hana.R index 1b64f93bf..9491d97b1 100644 --- a/R/backend-hana.R +++ b/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 ) } diff --git a/R/backend-mssql.R b/R/backend-mssql.R index 94aba0b4d..e4e9c4671 100644 --- a/R/backend-mssql.R +++ b/R/backend-mssql.R @@ -510,20 +510,13 @@ mssql_version <- function(con) { # #' @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, diff --git a/R/backend-mysql.R b/R/backend-mysql.R index f40093475..2cf4ab963 100644 --- a/R/backend-mysql.R +++ b/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"]]) } diff --git a/R/backend-postgres-old.R b/R/backend-postgres-old.R index 0fe4dc780..e79379a3e 100644 --- a/R/backend-postgres-old.R +++ b/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, ..., diff --git a/R/backend-postgres.R b/R/backend-postgres.R index 3a3a6a9b3..42265cb4f 100644 --- a/R/backend-postgres.R +++ b/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) diff --git a/R/backend-spark-sql.R b/R/backend-spark-sql.R index 2893da6a6..502f65f12 100644 --- a/R/backend-spark-sql.R +++ b/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 ", diff --git a/R/build-sql.R b/R/build-sql.R index 97449ae1a..4c3de18b9 100644 --- a/R/build-sql.R +++ b/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) diff --git a/R/db-io.R b/R/db-io.R index 690dfec13..b22ccd494 100644 --- a/R/db-io.R +++ b/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, diff --git a/R/db-sql.R b/R/db-sql.R index 8634711e9..b8f934d46 100644 --- a/R/db-sql.R +++ b/R/db-sql.R @@ -158,7 +158,7 @@ sql_table_index <- function(con, unique = FALSE, ..., call = caller_env()) { - as_table_ident(table, error_call = call) + check_table_id(table, call = call) check_character(columns, call = call) check_name(name, allow_null = TRUE, call = call) check_bool(unique, call = call) @@ -173,10 +173,12 @@ sql_table_index.DBIConnection <- function(con, unique = FALSE, ..., call = caller_env()) { - table <- as_table_ident(table, error_call = call) - table_name <- collapse_table_ident(table, sep = "_") + table <- as_table_path(table, con) - name <- name %||% paste0(c(table_name, columns), collapse = "_") + if (is.null(name)) { + table_name <- table_name(table, con) + name <- name %||% paste0(c(table_name, columns), collapse = "_") + } glue_sql2( con, "CREATE ", if (unique) "UNIQUE ", "INDEX {.name name}", @@ -201,23 +203,21 @@ sql_query_explain.DBIConnection <- function(con, sql, ...) { #' @rdname db-sql #' @export sql_query_fields <- function(con, sql, ...) { - as_from(sql) + check_table_source(sql) check_dots_used() UseMethod("sql_query_fields") } #' @export sql_query_fields.DBIConnection <- function(con, sql, ...) { - sql <- as_from(sql) - + sql <- as_table_source(sql, con) dbplyr_query_select(con, sql("*"), dbplyr_sql_subquery(con, sql), where = sql("0 = 1")) } #' @rdname db-sql #' @export sql_query_save <- function(con, sql, name, temporary = TRUE, ...) { - as_from(sql) - as_table_ident(name) + check_table_id(name) check_bool(temporary) check_dots_used() @@ -225,8 +225,7 @@ sql_query_save <- function(con, sql, name, temporary = TRUE, ...) { } #' @export sql_query_save.DBIConnection <- function(con, sql, name, temporary = TRUE, ...) { - sql <- as_from(sql) - name <- as_table_ident(name) + name <- as_table_path(name, con) glue_sql2( con, @@ -245,7 +244,7 @@ sql_query_wrap <- function(con, from, name = NULL, ..., lvl = 0) { } #' @export sql_query_wrap.DBIConnection <- function(con, from, name = NULL, ..., lvl = 0) { - from <- as_from(from) + from <- as_table_source(from, con) if (is.sql(from)) { if (db_supports_table_alias_with_as(con)) { @@ -257,11 +256,14 @@ sql_query_wrap.DBIConnection <- function(con, from, name = NULL, ..., lvl = 0) { from <- sql_indent_subquery(from, con, lvl) # some backends, e.g. Postgres, require an alias for a subquery name <- name %||% unique_subquery_name() - out <- glue_sql2(con, "{from}", as_sql, "{.name name}") - return(out) + glue_sql2(con, "{from}", as_sql, "{.tbl name}") + } else { # must be a table_path + if (!is.null(name)) { + table <- table_name(name, con) + names(from) <- as_table_path(table, con) + } + from } - - set_table_ident_alias(from, name) } #' @export @@ -286,14 +288,14 @@ sql_indent_subquery <- function(from, con, lvl = 0) { #' @rdname db-sql #' @export sql_query_rows <- function(con, sql, ...) { - as_from(sql) + check_table_source(sql) check_dots_used() UseMethod("sql_query_rows") } #' @export sql_query_rows.DBIConnection <- function(con, sql, ...) { - sql <- as_from(sql) + sql <- as_table_source(sql, con) from <- dbplyr_sql_subquery(con, sql, "master") glue_sql2(con, "SELECT COUNT(*) FROM {.from from}") } @@ -770,8 +772,8 @@ sql_query_insert <- function(con, conflict = c("error", "ignore"), returning_cols = NULL, method = NULL) { - as_table_ident(table) - as_from(from) + check_table_id(table) + check_table_source(from) check_character(insert_cols) check_character(by) check_character(returning_cols, allow_null = TRUE) @@ -790,8 +792,8 @@ sql_query_insert.DBIConnection <- function(con, conflict = c("error", "ignore"), returning_cols = NULL, method = NULL) { - table <- as_table_ident(table) - from <- as_from(from) + table <- as_table_path(table, con) + from <- as_table_source(from, con) method <- method %||% "where_not_exists" arg_match(method, "where_not_exists", error_arg = "method") @@ -838,8 +840,8 @@ sql_query_append <- function(con, return(out) } - as_table_ident(table) - as_from(from) + check_table_id(table) + check_table_source(from) check_character(insert_cols) check_character(returning_cols, allow_null = TRUE) @@ -854,8 +856,8 @@ sql_query_append.DBIConnection <- function(con, insert_cols, ..., returning_cols = NULL) { - table <- as_table_ident(table) - from <- as_from(from) + table <- as_table_path(table, con) + from <- as_table_source(from, con) # https://stackoverflow.com/questions/25969/insert-into-values-select-from parts <- rows_prep(con, table, from, by = list(), lvl = 0) @@ -880,8 +882,8 @@ sql_query_update_from <- function(con, update_values, ..., returning_cols = NULL) { - as_table_ident(table) - as_from(from) + check_table_id(table) + check_table_source(from) check_character(by) check_character(update_values) check_named(update_values) @@ -899,8 +901,8 @@ sql_query_update_from.DBIConnection <- function(con, update_values, ..., returning_cols = NULL) { - table <- as_table_ident(table) - from <- as_from(from) + table <- as_table_path(table, con) + from <- as_table_source(from, con) # https://stackoverflow.com/questions/2334712/how-do-i-update-from-a-select-in-sql-server parts <- rows_prep(con, table, from, by, lvl = 0) @@ -928,8 +930,8 @@ sql_query_upsert <- function(con, ..., returning_cols = NULL, method = NULL) { - as_table_ident(table) - as_from(from) + check_table_id(table) + check_table_source(from) check_character(by) check_character(update_cols) check_character(returning_cols, allow_null = TRUE) @@ -949,8 +951,8 @@ sql_query_upsert.DBIConnection <- function(con, ..., returning_cols = NULL, method = NULL) { - table <- as_table_ident(table) - from <- as_from(from) + table <- as_table_path(table, con) + from <- as_table_source(from, con) method <- method %||% "cte_update" arg_match(method, "cte_update", error_arg = "method") @@ -998,8 +1000,8 @@ sql_query_delete <- function(con, by, ..., returning_cols = NULL) { - as_table_ident(table) - as_from(from) + check_table_id(table) + check_table_source(from) check_character(by) check_character(returning_cols, allow_null = TRUE) @@ -1014,8 +1016,8 @@ sql_query_delete.DBIConnection <- function(con, by, ..., returning_cols = NULL) { - table <- as_table_ident(table) - from <- as_from(from) + table <- as_table_path(table, con) + from <- as_table_source(from, con) parts <- rows_prep(con, table, from, by, lvl = 1) clauses <- list2( @@ -1119,16 +1121,16 @@ db_save_query.DBIConnection <- function(con, temporary = TRUE, ..., overwrite = FALSE) { + name <- as_table_path(name, con) + sql <- sql_query_save(con, sql, name, temporary = temporary, ...) + if (overwrite) { - name <- as_table_ident(name) - name_id <- table_ident_to_id(name) - found <- DBI::dbExistsTable(con, name_id) + found <- DBI::dbExistsTable(con, SQL(name)) if (found) { - DBI::dbRemoveTable(con, name_id) + DBI::dbRemoveTable(con, SQL(name)) } } - sql <- sql_query_save(con, sql, name, temporary = temporary, ...) db_execute(con, sql, "Can't save query to table {.table {format(name, con = con)}}.") name diff --git a/R/lazy-join-query.R b/R/lazy-join-query.R index 97f9de32e..03209a7bd 100644 --- a/R/lazy-join-query.R +++ b/R/lazy-join-query.R @@ -153,7 +153,9 @@ op_vars.lazy_semi_join_query <- function(op) { #' @export sql_build.lazy_multi_join_query <- function(op, con, ..., sql_options = NULL) { - table_names_out <- generate_join_table_names(op$table_names) + + table_names_out <- generate_join_table_names(op$table_names, con) + tables <- set_names(c(list(op$x), op$joins$table), table_names_out) table_vars <- purrr::map(tables, op_vars) select_sql <- sql_multi_join_vars( @@ -185,21 +187,21 @@ sql_build.lazy_multi_join_query <- function(op, con, ..., sql_options = NULL) { ) } -generate_join_table_names <- function(table_names) { - table_name_length_max <- max(nchar(table_names$name)) +generate_join_table_names <- function(table_names, con) { + names <- table_name(table_names$name, con) + table_name_length_max <- max(nchar(names)) if (length(table_names$name) != 2) { - table_names_repaired <- vctrs::vec_as_names(table_names$name, repair = "unique", quiet = TRUE) + table_names_repaired <- vctrs::vec_as_names(names, repair = "unique", quiet = TRUE) may_repair_name <- table_names$from != "as" - table_names$name[may_repair_name] <- table_names_repaired[may_repair_name] - table_names_prepared <- table_names$name + names[may_repair_name] <- table_names_repaired[may_repair_name] } else{ - table_names_prepared <- join_two_table_alias(table_names$name, table_names$from) + names <- join_two_table_alias(names, table_names$from) } # avoid database aliases exceeding the database-specific maximum length - abbreviate( - table_names_prepared, + abbr_names <- abbreviate( + names, # arbitrarily floor at identifier limit for Postgres backend to avoid unnecessarily truncating reasonable lengths # Source: https://www.postgresql.org/docs/current/sql-syntax-lexical.html#SQL-SYNTAX-IDENTIFIERS # "By default, NAMEDATALEN is 64 so the maximum identifier length is 63 bytes." @@ -213,11 +215,13 @@ generate_join_table_names <- function(table_names) { # since we opt to add qualifiers (...1, _{R/L}HS, etc.) to end of table name method = "both.sides" ) + + as_table_paths(abbr_names, con) } #' @export sql_build.lazy_rf_join_query <- function(op, con, ..., sql_options = NULL) { - table_names_out <- generate_join_table_names(op$table_names) + table_names_out <- generate_join_table_names(op$table_names, con) vars_classic <- as.list(op$vars) vars_classic$all_x <- op_vars(op$x) @@ -260,7 +264,7 @@ sql_build.lazy_semi_join_query <- function(op, con, ..., sql_options = NULL) { } y_vars <- op_vars(op$y) - replacements <- purrr::map(y_vars, ~ call2("$", sym(op$by$y_as), sym(.x))) + replacements <- purrr::map(y_vars, ~ call2("$", call2("sql", op$by$y_as), sym(.x))) where <- purrr::map( op$where, ~ replace_sym(.x, y_vars, replacements) diff --git a/R/lazy-ops.R b/R/lazy-ops.R index 14199a42c..e2fa8635e 100644 --- a/R/lazy-ops.R +++ b/R/lazy-ops.R @@ -31,17 +31,12 @@ lazy_base_query <- function(x, vars, class = character(), ...) { ) } -lazy_query_local <- function(df, name) { - name <- as_table_ident(name) - lazy_base_query(df, names(df), class = "local", name = name) -} - lazy_query_remote <- function(x, vars) { lazy_base_query(x, vars, class = "remote") } base_query <- function(from) { - from <- as_from(from) + check_table_source(from) structure( list(from = from), class = c("base_query", "query") @@ -50,7 +45,7 @@ base_query <- function(from) { #' @export print.lazy_base_remote_query <- function(x, ...) { - if (is_table_ident(x$x)) { + if (is_table_path(x$x)) { cat_line("From: ", format(x$x)) } else { cat_line("From: ") @@ -94,7 +89,7 @@ sql_render.base_query <- function(query, } #' @export -flatten_query.base_query <- function(qry, query_list) { +flatten_query.base_query <- function(qry, query_list, con) { query_list } diff --git a/R/query-join.R b/R/query-join.R index afe2840ff..733781148 100644 --- a/R/query-join.R +++ b/R/query-join.R @@ -115,19 +115,19 @@ sql_render.multi_join_query <- function(query, flatten_query.join_query <- flatten_query_2_tables #' @export -flatten_query.multi_join_query <- function(qry, query_list) { +flatten_query.multi_join_query <- function(qry, query_list, con) { x <- qry$x - query_list_new <- flatten_query(x, query_list) + query_list_new <- flatten_query(x, query_list, con) qry$x <- get_subquery_name(x, query_list_new) for (i in vctrs::vec_seq_along(qry$joins)) { y <- qry$joins$table[[i]] - query_list_new <- flatten_query(y, query_list_new) + query_list_new <- flatten_query(y, query_list_new, con) qry$joins$table[[i]] <- get_subquery_name(y, query_list_new) } # TODO reuse query - name <- unique_subquery_name() + name <- as_table_path(unique_subquery_name(), con) wrapped_query <- set_names(list(qry), name) query_list$queries <- c(query_list_new$queries, wrapped_query) @@ -182,12 +182,11 @@ sql_multi_join_vars <- function(con, vars, table_vars, use_star, qualify_all_col duplicated_vars <- all_vars[vctrs::vec_duplicate_detect(all_vars)] duplicated_vars <- unique(duplicated_vars) } - table_names <- names(table_vars) + table_paths <- table_path(names(table_vars)) - # FIXME vectorise `sql_table_prefix()` (need to update `ident()` and friends for this...) out <- rep_named(vars$name, list()) - for (i in seq_along(table_names)) { + for (i in seq_along(table_paths)) { all_vars_i <- table_vars[[i]] vars_idx_i <- which(vars$table == i) used_vars_i <- vars$var[vars_idx_i] @@ -195,12 +194,12 @@ sql_multi_join_vars <- function(con, vars, table_vars, use_star, qualify_all_col if (use_star && join_can_use_star(all_vars_i, used_vars_i, out_vars_i, vars_idx_i)) { id <- vars_idx_i[[1]] - out[[id]] <- sql_star(con, table_names[i]) + out[[id]] <- sql_star(con, table_paths[i]) names(out)[id] <- "" } else { out[vars_idx_i] <- purrr::map2( used_vars_i, i, - ~ sql_multi_join_var(con, .x, .y, table_names, duplicated_vars) + ~ sql_multi_join_var(con, .x, .y, table_paths, duplicated_vars) ) } @@ -250,7 +249,10 @@ sql_rf_join_vars <- function(con, use_star, qualify_all_columns) { type <- arg_match0(type, c("right", "full")) - table_names <- c(unclass(x_as), unclass(y_as)) + + check_table_path(x_as) + check_table_path(y_as) + table_names <- c(x_as, y_as) if (type == "full") { duplicated_vars <- intersect(tolower(vars$all_x), tolower(vars$all_y)) @@ -334,22 +336,20 @@ sql_table_prefix <- function(con, var, table = NULL) { if (!is_bare_character(var)) { cli_abort("{.arg var} must be a bare character.", .internal = TRUE) } - var <- sql_escape_ident(con, var) - - if (!is.null(table)) { - table <- as_table_ident(table) - table <- escape(table, collapse = NULL, con = con) - sql(paste0(table, ".", var)) - } else { - var - } + sql_qualify_var(con, table, var) } sql_star <- function(con, table = NULL) { - var <- sql("*") + sql_qualify_var(con, table, SQL("*")) +} + +sql_qualify_var <- function(con, table, var) { + var <- sql_escape_ident(con, var) + if (!is.null(table)) { - table <- as_table_ident(table) - table <- escape(table, collapse = NULL, con = con) + table <- table_name(table, con) + table <- as_table_paths(table, con) + sql(paste0(table, ".", var)) } else { var diff --git a/R/query-select.R b/R/query-select.R index 9839ba53d..39c54ac70 100644 --- a/R/query-select.R +++ b/R/query-select.R @@ -145,10 +145,10 @@ warn_drop_order_by <- function() { } #' @export -flatten_query.select_query <- function(qry, query_list) { +flatten_query.select_query <- function(qry, query_list, con) { from <- qry$from - query_list <- flatten_query(from, query_list) + query_list <- flatten_query(from, query_list, con) qry$from <- get_subquery_name(from, query_list) - querylist_reuse_query(qry, query_list) + querylist_reuse_query(qry, query_list, con) } diff --git a/R/query-set-op.R b/R/query-set-op.R index 7a657d51e..7758934f5 100644 --- a/R/query-set-op.R +++ b/R/query-set-op.R @@ -105,19 +105,19 @@ sql_render.union_query <- function(query, flatten_query.set_op_query <- flatten_query_2_tables #' @export -flatten_query.union_query <- function(qry, query_list) { +flatten_query.union_query <- function(qry, query_list, con) { x <- qry$x - query_list_new <- flatten_query(x, query_list) + query_list_new <- flatten_query(x, query_list, con) qry$x <- get_subquery_name(x, query_list_new) for (i in vctrs::vec_seq_along(qry$unions)) { y <- qry$unions$table[[i]] - query_list_new <- flatten_query(y, query_list_new) + query_list_new <- flatten_query(y, query_list_new, con) qry$unions$table[[i]] <- get_subquery_name(y, query_list_new) } # TODO reuse query - name <- unique_subquery_name() + name <- as_table_path(unique_subquery_name(), con) wrapped_query <- set_names(list(qry), name) query_list$queries <- c(query_list_new$queries, wrapped_query) diff --git a/R/remote.R b/R/remote.R index 3366e2663..41a977c8f 100644 --- a/R/remote.R +++ b/R/remote.R @@ -33,11 +33,16 @@ remote_name <- function(x, null_if_local = TRUE) { table <- remote_table(x, null_if_local = null_if_local) - if (is.null(table) || is.sql(table)) { - return(NULL) + if (is.sql(table) || is.null(table)) { + NULL + } else { + con <- remote_con(x) + if (is.null(con)) { + table + } else { + table_name(table, con) + } } - - table_ident_name(table) } #' @export diff --git a/R/simulate.R b/R/simulate.R index fe3be85c7..04a950c94 100644 --- a/R/simulate.R +++ b/R/simulate.R @@ -32,7 +32,11 @@ sql_escape_ident.DBIConnection <- function(con, x) { } #' @export sql_escape_ident.TestConnection <- function(con, x) { - sql_quote(x, "`") + if (methods::is(x, "SQL")) { + x + } else { + sql_quote(x, "`") + } } sql_escape_string <- function(con, x) { diff --git a/R/sql-build.R b/R/sql-build.R index eb74f78f7..af37cce0b 100644 --- a/R/sql-build.R +++ b/R/sql-build.R @@ -119,7 +119,7 @@ sql_render.lazy_query <- function(query, qry <- sql_optimise(qry, con = con, subquery = subquery) if (sql_options$cte) { - query_list <- flatten_query(qry, list(queries = list(), name = NULL)) + query_list <- flatten_query(qry, list(queries = list(), name = NULL), con = con) queries <- query_list$queries rendered_queries <- purrr::map2( @@ -144,6 +144,7 @@ cte_render <- function(query_list, con) { ctes <- purrr::imap( query_list[-n], function(query, name) { + name <- table_path(name) glue_sql2(con, "{.name name} {.kw 'AS'} (\n{query}\n)") } ) @@ -158,17 +159,17 @@ get_subquery_name <- function(x, query_list) { base_query(query_list$name) } -flatten_query <- function(qry, query_list) { +flatten_query <- function(qry, query_list, con) { UseMethod("flatten_query") } -querylist_reuse_query <- function(qry, query_list) { +querylist_reuse_query <- function(qry, query_list, con) { id <- vctrs::vec_match(list(unclass(qry)), purrr::map(query_list$queries, unclass)) if (!is.na(id)) { query_list$name <- names(query_list$queries)[[id]] } else { - name <- unique_subquery_name() + name <- as_table_path(unique_subquery_name(), con) wrapped_query <- set_names(list(qry), name) query_list$queries <- c(query_list$queries, wrapped_query) query_list$name <- name @@ -177,16 +178,16 @@ querylist_reuse_query <- function(qry, query_list) { query_list } -flatten_query_2_tables <- function(qry, query_list) { +flatten_query_2_tables <- function(qry, query_list, con) { x <- qry$x - query_list_x <- flatten_query(x, query_list) + query_list_x <- flatten_query(x, query_list, con) qry$x <- get_subquery_name(x, query_list_x) y <- qry$y - query_list_y <- flatten_query(y, query_list_x) + query_list_y <- flatten_query(y, query_list_x, con) qry$y <- get_subquery_name(y, query_list_y) - querylist_reuse_query(qry, query_list_y) + querylist_reuse_query(qry, query_list_y, con) } diff --git a/R/table-ident.R b/R/table-ident.R deleted file mode 100644 index 50bb029c7..000000000 --- a/R/table-ident.R +++ /dev/null @@ -1,262 +0,0 @@ -new_table_ident <- function(..., - table = NA_character_, - schema = NA_character_, - catalog = NA_character_, - quoted = FALSE, - error_call = caller_env()) { - check_dots_empty(call = error_call) - - check_character(table, call = error_call) - check_character(schema, call = error_call) - check_character(catalog, call = error_call) - check_logical(quoted, call = error_call) - - data <- vctrs::vec_recycle_common( - table = table, - schema = schema, - catalog = catalog, - quoted = quoted, - alias = NA_character_, - .call = error_call - ) - - purrr::pmap( - data, - function(table, schema, catalog, quoted, alias) { - check_db_table_args( - table = table, - schema = schema, - catalog = catalog, - quoted = quoted, - call = error_call - ) - } - ) - - vctrs::new_rcrd(data, class = "dbplyr_table_ident") -} - -check_db_table_args <- function(table, - schema, - catalog, - quoted, - call = caller_env()) { - if (!is.na(catalog) && is.na(schema)) { - cli_abort("Must supply {.arg schema} when {.arg catalog} is supplied.", call = call) - } - if (!is.na(schema) && is.na(table)) { - cli_abort("Must supply {.arg table} when {.arg schema} is supplied.", call = call) - } - - if (quoted && !is.na(schema)) { - cli_abort("Can't supply a schema when {.arg table} is quoted.", call = call) - } -} - -is_table_ident <- function(x) { - inherits(x, "dbplyr_table_ident") -} - -as_table_ident <- function(x, ..., error_call = caller_env()) { - check_dots_empty() - UseMethod("as_table_ident") -} - -#' @export -as_table_ident.dbplyr_table_ident <- function(x, ..., error_call = caller_env()) { - x -} - -#' @export -as_table_ident.character <- function(x, ..., error_call = caller_env()) { - new_table_ident(table = unclass(x)) -} - -#' @export -as_table_ident.ident <- function(x, ..., error_call = caller_env()) { - new_table_ident(table = unclass(x)) -} - -#' @export -as_table_ident.ident_q <- function(x, ..., error_call = caller_env()) { - inform_unwanted_table_ident("ident_q") - new_table_ident(table = unclass(x), quoted = TRUE) -} - -#' @export -as_table_ident.sql <- function(x, ..., error_call = caller_env()) { - inform_unwanted_table_ident("sql") - new_table_ident(table = unclass(x), quoted = TRUE) -} - -inform_unwanted_table_ident <- function(f) { - cli::cli_inform( - c( - i = "Using {.fn {f}} for a table identifier is intended as fallback in case of bugs.", - i = "If you need it to work around a bug please open an issue {.url https://github.com/tidyverse/dbplyr/issues}." - ), - .frequency = "regularly", - .frequency_id = paste0("dbplyr_", f) - ) -} - -#' @export -as_table_ident.dbplyr_schema <- function(x, ..., error_call = caller_env()) { - new_table_ident( - table = unclass(x$table), - schema = unclass(x$schema) - ) -} - -#' @export -as_table_ident.dbplyr_catalog <- function(x, ..., error_call = caller_env()) { - new_table_ident( - table = unclass(x$table), - schema = unclass(x$schema), - catalog = unclass(x$catalog) - ) -} - -#' @export -as_table_ident.Id <- function(x, ..., error_call = caller_env()) { - id <- x@name - nms <- names(id) - known_names <- c("catalog", "schema", "table") - unknown_names <- setdiff(nms, known_names) - if (!is_empty(unknown_names)) { - cli_abort(c( - "{.arg table} is an with unknown names {.val {unknown_names}}.", - i = "Only {.val {known_names}} are supported." - ), call = error_call) - } - - new_table_ident( - table = if ("table" %in% nms) id[["table"]] else NA_character_, - schema = if ("schema" %in% nms) id[["schema"]] else NA_character_, - catalog = if ("catalog" %in% nms) id[["catalog"]] else NA_character_ - ) -} - -as_table_ident_or_sql <- function(x, ..., error_call = caller_env()) { - if (is.sql(x)) { - return(x) - } - - as_table_ident(x, error_call = error_call) -} - -#' @export -format.dbplyr_table_ident <- function(x, ..., sep = ".", con = NULL) { - quoted <- vctrs::field(x, "quoted") - con <- con %||% simulate_dbi() - x <- quote_table_ident(x, con) - - collapse_table_ident(x, sep = sep) -} - -is_table_ident <- function(x) { - inherits(x, "dbplyr_table_ident") -} - -#' @export -escape.dbplyr_table_ident <- function(x, parens = FALSE, collapse = ", ", con = NULL) { - # this ignores `parens` and `collapse`; at least for now - x_quoted <- format(x, con = con) - - canonical_alias <- purrr::map_chr(x, ~ table_ident_name(.x) %||% "") - alias <- table_ident_alias(x) %||% vctrs::vec_rep("", vctrs::vec_size(x)) - - if (db_supports_table_alias_with_as(con)) { - as_sql <- style_kw(" AS ") - } else { - as_sql <- " " - } - - alias_esc <- sql_escape_ident(con, alias) - out <- ifelse( - alias == "" | alias == canonical_alias, - x_quoted, - paste0(x_quoted, as_sql, alias_esc) - ) - - sql_vector(out, parens, collapse, con = con) -} - -quote_table_ident <- function(x, con) { - quoted <- vctrs::field(x, "quoted") - - for (field in c("table", "schema", "catalog")) { - xf <- vctrs::field(x, field) - idx <- !is.na(xf) & !quoted - xf[idx] <- sql_escape_ident(con, xf[idx]) - x <- vctrs::`field<-`(x, field, xf) - } - - x -} - -collapse_table_ident <- function(x, sep = ".") { - table <- vctrs::field(x, "table") - schema <- vctrs::field(x, "schema") - catalog <- vctrs::field(x, "catalog") - - out <- table - out[!is.na(schema)] <- paste0(schema, sep, table)[!is.na(schema)] - out[!is.na(catalog)] <- paste0(catalog, sep, schema, sep, table)[!is.na(catalog)] - - out -} - -table_ident_name <- function(x) { - table <- vctrs::field(x, "table") - quoted <- vctrs::field(x, "quoted") - if (quoted) { - NULL - } else { - table - } -} - -table_ident_alias <- function(x) { - alias <- vctrs::field(x, "alias") - if (all(is.na(alias))) { - return(NULL) - } - - alias -} - -set_table_ident_alias <- function(x, alias) { - alias <- alias %||% vctrs::vec_rep(NA_character_, times = vctrs::vec_size(x)) - vctrs::`field<-`(x, "alias", alias) -} - -as_from <- function(x, ..., arg = caller_arg(x), error_call = caller_env()) { - if (is.sql(x)) { - return(x) - } - - as_table_ident(x, error_call = error_call) -} - -table_ident_to_id <- function(x) { - vctrs::vec_check_size(x, 1) - - quoted <- vctrs::field(x, "quoted") - if (quoted) { - out <- DBI::SQL(sql) - return(out) - } - - catalog <- vctrs::field(x, "catalog") - schema <- vctrs::field(x, "schema") - table <- vctrs::field(x, "table") - - if (!is.na(catalog)) { - DBI::Id(catalog = catalog, schema = schema, table = table) - } else if (!is.na(schema)) { - DBI::Id(schema = schema, table = table) - } else { - DBI::Id(table = table) - } -} diff --git a/R/table-name.R b/R/table-name.R new file mode 100644 index 000000000..d968257ea --- /dev/null +++ b/R/table-name.R @@ -0,0 +1,197 @@ +# table source = table id or sql + +# table id = +# * interface to outside world; many ways to specify +# * always refers to exactly one table +# * but all converted to table name ASAP + +# table path = +# * qualified table identifier (e.g. foo.bar.baz, bar.baz, baz) +# * always quoted +# * internal (and backend) use only; not user facing +# * can be vector containing multiple names +# * object names are always assumed to be table paths + +# table_path -------------------------------------------------------------- + +table_path <- function(x) { + check_character(x) + x <- unname(x) + structure(x, class = c("dbplyr_table_path", "character")) +} + +# So you can do SQL(table_path("foo")) +setOldClass(c("dbplyr_table_path", "character")) + +is_table_path <- function(x) { + inherits(x, "dbplyr_table_path") +} + +#' @export +print.dbplyr_table_path <- function(x, ...) { + cat(" ", paste0(style_kw(x), collapse = ", "), "\n", sep = "") +} + +#' @export +`[.dbplyr_table_path` <- function(x, ...) { + table_path(NextMethod()) +} +#' @export +`[[.dbplyr_table_path` <- function(x, ...) { + table_path(NextMethod()) +} + +#' @export +`c.dbplyr_table_path` <- function(x, ...) { + table_path(NextMethod()) +} + +make_table_path <- function(x, con, collapse = TRUE) { + needs_quote <- !vapply(x, component_is_escaped, logical(1)) + + x <- vapply(x, unclass, character(1)) + x[needs_quote] <- sql_escape_ident(con, x[needs_quote]) + if (collapse) { + x <- paste0(x, collapse = ".") + } + + table_path(x) +} + +component_is_escaped <- function(x) { + inherits(x, "AsIs") || is.sql(x) || inherits(x, "ident_q") +} + +as_table_paths <- function(x, con) { + make_table_path(x, con, collapse = FALSE) +} + +# Extract just the table name from a full identifier +table_name <- function(x, con) { + x <- as_table_path(x, con) + + vapply(x, FUN.VALUE = character(1), function(x) { + if (x == "") return("") + + out <- table_path_components(x, con) + out[[length(out)]] + }) +} +table_path_components <- function(x, con) { + quote_char <- substr(sql_escape_ident(con, ""), 1, 1) + + scan( + text = x, + what = character(), + quote = quote_char, + quiet = TRUE, + na.strings = character(), + sep = "." + ) +} + +#' @export +escape.dbplyr_table_path <- function(x, parens = FALSE, collapse = ", ", con = NULL) { + # names are always already escaped + alias <- names2(x) + table_path <- as_table_path(table_name(x, con), con) + has_alias <- alias == "" | alias == table_path + + if (db_supports_table_alias_with_as(con)) { + as_sql <- style_kw(" AS ") + } else { + as_sql <- " " + } + + out <- ifelse(has_alias, unname(x), paste0(x, as_sql, alias)) + sql_vector(out, parens, collapse, con = con) +} + +# table id ---------------------------------------------------------------- + +check_table_id <- function(x, arg = caller_arg(x), call = caller_env()) { + if (!is_table_id(x)) { + stop_input_type(x, "a table identifier") + } +} + +is_table_id <- function(x) { + is_table_path(x) || + is.ident(x) || + methods::is(x, "Id") || + is_catalog(x) || + is_schema(x) || + is.character(x) +} + +check_table_path <- function(x, + error_arg = caller_arg(x), + error_call = caller_env()) { + if (!is_table_path(x)) { + cli::cli_abort( + "{.arg {error_arg}} must be a , not {.obj_type_friendly x}.", + call = error_call, + .internal = TRUE + ) + } +} + +as_table_path <- function(x, + con, + error_arg = caller_arg(x), + error_call = caller_env()) { + check_required(con) + + if (is_table_path(x)) { + x + } else if (is.sql(x)) { + cli::cli_warn( + c( + "{.arg {error_arg}} uses SQL where a table identifier is expected.", + i = "If you want to use a literal (unquoted) identifier use {.fn I} instead." + ) + ) + table_path(unclass(x)) + } else if (inherits(x, "ident_q")) { + table_path(paste0(x, collapse = ".")) + } else if (is.ident(x)) { + make_table_path(unclass(x), con) + } else if (methods::is(x, "Id")) { + make_table_path(x@name, con) + } else if (inherits(x, "dbplyr_catalog")) { + make_table_path(list(x$catalog, x$schema, x$table), con) + } else if (inherits(x, "dbplyr_schema")) { + make_table_path(list(x$schema, x$table), con) + } else if (inherits(x, "AsIs")) { + check_string(unclass(x), allow_empty = FALSE, arg = error_arg, call = error_call) + table_path(unclass(x)) + } else if (is.character(x)) { + check_string(x, allow_empty = FALSE, arg = error_arg, call = error_call) + make_table_path(x, con, collapse = FALSE) + } else { + cli::cli_abort( + "{.arg {error_arg}} uses unknown specification for table name", + error_call = error_call + ) + } +} + +# table source ------------------------------------------------------------ + +# Returns either SQL (representing a custom query) or a table name +as_table_source <- function(x, con, ..., error_arg = caller_arg(x), error_call = caller_env()) { + if (is.sql(x)) { + x + } else if (is_table_id(x)) { + as_table_path(x, con = con, error_arg = error_arg, error_call = error_call) + } else { + check_table_source(x, arg = error_arg, call = error_call) + } +} + +check_table_source <- function(x, arg = caller_arg(x), call = caller_env()) { + if (!is.sql(x) && !is_table_id(x)) { + stop_input_type(x, "a table source (SQL or a table identifier)") + } +} + diff --git a/R/tbl-lazy.R b/R/tbl-lazy.R index ca2e0a689..d8856ca5f 100644 --- a/R/tbl-lazy.R +++ b/R/tbl-lazy.R @@ -17,9 +17,11 @@ tbl_lazy <- function(df, con = NULL, ..., name = "df") { con <- con %||% sql_current_con() %||% simulate_dbi() subclass <- class(con)[[1]] + name <- as_table_path(name, con) + dplyr::make_tbl( purrr::compact(c(subclass, "lazy")), - lazy_query = lazy_query_local(df, name), + lazy_query = lazy_base_query(df, names(df), class = "local", name = name), src = src_dbi(con) ) } diff --git a/R/tbl-sql.R b/R/tbl-sql.R index 31e205335..fd93ccc9e 100644 --- a/R/tbl-sql.R +++ b/R/tbl-sql.R @@ -17,11 +17,7 @@ tbl_sql <- function(subclass, src, from, ..., vars = NULL, check_from = TRUE) { # Can't use check_dots_used(), #1429 check_character(vars, allow_null = TRUE) - from <- as_from(from) - if (check_from) { - check_from_for_query_or_schema(from) - } - + from <- as_table_source(from, con = src$con) vars <- vars %||% dbplyr_query_fields(src$con, from) dplyr::make_tbl( @@ -31,33 +27,6 @@ tbl_sql <- function(subclass, src, from, ..., vars = NULL, check_from = TRUE) { ) } -check_from_for_query_or_schema <- function(from) { - if (!is_table_ident(from)) { - return() - } - - table <- vctrs::field(from, "table") - schema <- vctrs::field(from, "schema") - - if (grepl(" from ", tolower(table), fixed = TRUE)) { - cli::cli_inform(c( - "It looks like you tried to incorrectly use an SQL query as source.", - i = "If you want to select from a query wrap it in {.fn sql}.", - i = "If your table actually contains {.val FROM} in the name use {.arg check_from = FALSE} to silence this message." - )) - return() - } - - if (grepl(".", table, fixed = TRUE) && is.na(schema)) { - cli::cli_inform(c( - "It looks like you tried to incorrectly use a table in a schema as source.", - i = "If you want to specify a schema use {.fn in_schema} or {.fn in_catalog}.", - i = "If your table actually contains {.val .} in the name use {.arg check_from = FALSE} to silence this message." - )) - return() - } -} - #' @importFrom dplyr same_src #' @export same_src.tbl_sql <- function(x, y) { diff --git a/R/utils.R b/R/utils.R index 5febbb770..21f50bea6 100644 --- a/R/utils.R +++ b/R/utils.R @@ -82,13 +82,27 @@ res_warn_incomplete <- function(res, hint = "n = -1") { cli::cli_warn("Only first {rows} results retrieved. Use {hint} to retrieve all.") } -hash_temp <- function(name) { - name <- paste0("#", name) - cli::cli_inform( - "Created a temporary table named {name}", - class = c("dbplyr_message_temp_table", "dbplyr_message") - ) - name +add_temporary_prefix <- function(con, table, temporary = TRUE) { + check_table_path(table) + + if (!temporary) { + return(table) + } + + pieces <- table_path_components(table, con) + table_name <- pieces[length(pieces)] + + if (substr(table_name, 1, 1) != "#") { + new_name <- paste0("#", table_name) + cli::cli_inform( + paste0("Created a temporary table named ", new_name), + class = c("dbplyr_message_temp_table", "dbplyr_message") + ) + pieces[[length(pieces)]] <- new_name + table <- make_table_path(pieces, con) + } + + table } # nocov end diff --git a/R/verb-compute.R b/R/verb-compute.R index c4272bf9c..c04a885f7 100644 --- a/R/verb-compute.R +++ b/R/verb-compute.R @@ -51,7 +51,7 @@ compute.tbl_sql <- function(x, name <- unique_table_name() } - name <- as_table_ident(name) + name <- as_table_path(name, x$src$con) vars <- op_vars(x) compute_check_indexes(x, indexes) diff --git a/R/verb-copy-to.R b/R/verb-copy-to.R index 4bf9c5713..03489113a 100644 --- a/R/verb-copy-to.R +++ b/R/verb-copy-to.R @@ -62,7 +62,7 @@ copy_to.src_sql <- function(dest, cli_abort("{.var df} must be a local dataframe or a remote tbl_sql") } - name <- as_table_ident(name) + name <- as_table_path(name, dest$con) if (inherits(df, "tbl_sql") && same_src(df$src, dest)) { out <- compute(df, @@ -181,8 +181,8 @@ sql_render.values_query <- function(query, } #' @export -flatten_query.values_query <- function(qry, query_list) { - querylist_reuse_query(qry, query_list) +flatten_query.values_query <- function(qry, query_list, con) { + querylist_reuse_query(qry, query_list, con) } #' @export diff --git a/R/verb-joins.R b/R/verb-joins.R index 20ef52aa0..82d72164e 100644 --- a/R/verb-joins.R +++ b/R/verb-joins.R @@ -374,7 +374,7 @@ add_join <- function(x, # the table alias can only be determined after `select()` was inlined. # This works even though `by` is used in `join_inline_select()` and updated # because this does not touch `by$x_as` and `by$y_as`. - join_alias <- make_join_aliases(x_as, y_as, sql_on, call) + join_alias <- make_join_aliases(x$src$con, x_as, y_as, sql_on, call) inline_result <- join_inline_select(x$lazy_query, by$x, by$on) x_lq <- inline_result$lq @@ -681,14 +681,15 @@ add_semi_join <- function(x, by$na_matches <- na_matches # the table alias can only be determined after `select()` was inlined - join_alias <- make_join_aliases(x_as, y_as, sql_on, call) + join_alias <- make_join_aliases(x$src$con, x_as, y_as, sql_on, call) - x_alias <- make_table_names(join_alias$x, x_lq) - y_alias <- make_table_names(join_alias$y, y_lq) - by[c("x_as", "y_as")] <- join_two_table_alias( - c(x_alias$name, y_alias$name), - c(x_alias$from, y_alias$from) + aliases <- vctrs::vec_rbind( + make_table_names(join_alias$x, x_lq), + make_table_names(join_alias$y, y_lq) ) + names <- generate_join_table_names(aliases, remote_con(x)) + by$x_as <- names[1] + by$y_as <- names[2] lazy_semi_join_query( x_lq, @@ -737,7 +738,7 @@ semi_join_inline_select <- function(lq, by, on) { ) } -make_join_aliases <- function(x_as, y_as, sql_on, call) { +make_join_aliases <- function(con, x_as, y_as, sql_on, call) { x_as <- check_join_as1(x_as, arg = "x_as", sql_on, default = "LHS", call) y_as <- check_join_as1(y_as, arg = "y_as", sql_on, default = "RHS", call) @@ -745,18 +746,21 @@ make_join_aliases <- function(x_as, y_as, sql_on, call) { cli_abort("{.arg y_as} must be different from {.arg x_as}.", call = call) } - list(x = x_as, y = y_as) + list( + x = if (!is.null(x_as)) as_table_path(x_as, con), + y = if (!is.null(y_as)) as_table_path(y_as, con) + ) } make_table_names <- function(as, lq) { - name <- unclass(remote_name(lq, null_if_local = FALSE)) + name <- remote_name(lq, null_if_local = FALSE) if (!is.null(as)) { tibble(name = as, from = "as") } else if (!is.null(name)) { tibble(name = name, from = "name") } else { - tibble(name = "", from = "") + tibble(name = table_path(""), from = "") } } @@ -805,6 +809,8 @@ join_vars <- function(x_names, y_names, type, by, suffix = c(".x", ".y"), call = ) } +# names are bare table names without qualifiers +# only called from generate_join_table_names join_two_table_alias <- function(names, from) { check_character(names) check_character(from) @@ -820,11 +826,7 @@ join_two_table_alias <- function(names, from) { tables_have_same_name <- from[1] == "name" && from[2] == "name" && identical(names[1], names[2]) if (tables_have_same_name) { - out <- c( - paste0(names[1], "_LHS"), - paste0(names[2], "_RHS") - ) - return(out) + return(paste0(names, c("_LHS", "_RHS"))) } out_repaired <- vctrs::vec_as_names(out, repair = "unique", quiet = TRUE) diff --git a/tests/testthat/_snaps/backend-.md b/tests/testthat/_snaps/backend-.md index 76d74cb61..6d7dedab7 100644 --- a/tests/testthat/_snaps/backend-.md +++ b/tests/testthat/_snaps/backend-.md @@ -97,16 +97,14 @@ Code sql_query_wrap(con, ident("table")) Output - - [1] `table` + `table` --- Code sql_query_wrap(con, in_schema("schema", "tbl")) Output - - [1] `schema`.`tbl` + `schema`.`tbl` --- @@ -120,14 +118,14 @@ Code sql_table_index(con, in_schema("schema", "tbl"), c("a", "b")) Output - CREATE INDEX `schema_tbl_a_b` ON `schema`.`tbl` (`a`, `b`) + CREATE INDEX `tbl_a_b` ON `schema`.`tbl` (`a`, `b`) --- Code sql_table_index(con, in_schema("schema", "tbl"), "c", unique = TRUE) Output - CREATE UNIQUE INDEX `schema_tbl_c` ON `schema`.`tbl` (`c`) + CREATE UNIQUE INDEX `tbl_c` ON `schema`.`tbl` (`c`) --- diff --git a/tests/testthat/_snaps/backend-mssql.md b/tests/testthat/_snaps/backend-mssql.md index 03d84441b..f17f8175c 100644 --- a/tests/testthat/_snaps/backend-mssql.md +++ b/tests/testthat/_snaps/backend-mssql.md @@ -467,3 +467,10 @@ Message Created a temporary table named #dbplyr_{tmp} +# add prefix to temporary table + + Code + out <- db_table_temporary(con, table_path("foo.bar"), temporary = TRUE) + Message + Created a temporary table named #bar + diff --git a/tests/testthat/_snaps/db-io.md b/tests/testthat/_snaps/db-io.md index 671570c6a..537255689 100644 --- a/tests/testthat/_snaps/db-io.md +++ b/tests/testthat/_snaps/db-io.md @@ -35,7 +35,7 @@ Output Error in `db_save_query()`: - ! Can't save query to table tmp. + ! Can't save query to table `tmp`. i Using SQL: CREATE TEMPORARY TABLE `tmp` AS `SELECT 2 FROM tmp` Caused by error: ! dummy DBI error diff --git a/tests/testthat/_snaps/db-sql.md b/tests/testthat/_snaps/db-sql.md index f00e1048f..faec92a58 100644 --- a/tests/testthat/_snaps/db-sql.md +++ b/tests/testthat/_snaps/db-sql.md @@ -51,7 +51,7 @@ Output Error in `db_save_query()`: - ! Can't save query to table tbl. + ! Can't save query to table `tbl`. i Using SQL: CREATE TEMPORARY TABLE `tbl` AS `invalid sql` Caused by error: ! dummy DBI error diff --git a/tests/testthat/_snaps/lazy-select-query.md b/tests/testthat/_snaps/lazy-select-query.md index d83918dff..95f1e91c4 100644 --- a/tests/testthat/_snaps/lazy-select-query.md +++ b/tests/testthat/_snaps/lazy-select-query.md @@ -1,14 +1,12 @@ # can print lazy_select_query Code - lazy_select_query(x = lazy_query_local(tibble(x = 1, y = 2), "df"), select = quos( - x_mean = mean(x), y2 = y), where = quos(y > 1, x == y - 2), group_by = quos( - "x")) + lazy_select_query(x = lf$lazy_query, select = quos(x_mean = mean(x), y2 = y), + where = quos(y > 1, x == y - 2), group_by = quos("x")) Output From: - - [1] `df` + `df` Select: x_mean = mean(x), y2 = y Where: y > 1, x == y - 2 Group by: "x" diff --git a/tests/testthat/_snaps/query-join.md b/tests/testthat/_snaps/query-join.md index 5122bfba8..631506fbc 100644 --- a/tests/testthat/_snaps/query-join.md +++ b/tests/testthat/_snaps/query-join.md @@ -5,20 +5,17 @@ Output X: - - [1] `lf1` + `lf1` Type: left By: x-x Y: - - [1] `lf2` + `lf2` Type: left By: x-x Y: - - [1] `lf3` + `lf3` # generated sql doesn't change unexpectedly diff --git a/tests/testthat/_snaps/query-select.md b/tests/testthat/_snaps/query-select.md index 68004fa6a..a60a2ede3 100644 --- a/tests/testthat/_snaps/query-select.md +++ b/tests/testthat/_snaps/query-select.md @@ -5,8 +5,7 @@ Output From: - - [1] `df` + `df` Select: `df`.* Where: `x` > 1 Order by: `x` diff --git a/tests/testthat/_snaps/query-semi-join.md b/tests/testthat/_snaps/query-semi-join.md index 174da02a0..dfab9f2c6 100644 --- a/tests/testthat/_snaps/query-semi-join.md +++ b/tests/testthat/_snaps/query-semi-join.md @@ -11,11 +11,9 @@ Where: "`df_RHS`.`z` = 2.0" X: - - [1] `df` + `df` Y: - - [1] `df` + `df` # generated sql doesn't change unexpectedly diff --git a/tests/testthat/_snaps/query-set-op.md b/tests/testthat/_snaps/query-set-op.md index fa70aed79..643388822 100644 --- a/tests/testthat/_snaps/query-set-op.md +++ b/tests/testthat/_snaps/query-set-op.md @@ -5,24 +5,21 @@ Output From: - - [1] `lf1` + `lf1` Select: `lf1`.*, NULL UNION From: - - [1] `lf2` + `lf2` Select: `x`, NULL, `z` UNION ALL From: - - [1] `lf3` + `lf3` Select: `x`, NULL, `z` # generated sql doesn't change unexpectedly diff --git a/tests/testthat/_snaps/table-ident.md b/tests/testthat/_snaps/table-ident.md deleted file mode 100644 index 335b6a0e1..000000000 --- a/tests/testthat/_snaps/table-ident.md +++ /dev/null @@ -1,109 +0,0 @@ -# is properly vectorised - - Code - new_table_ident(table = c("A", "B", "c"), schema = c("schema1", "schema2")) - Condition - Error: - ! Can't recycle `table` (size 3) to match `schema` (size 2). - -# can't supply table and sql - - Code - new_table_ident(schema = "my schema", table = "my table", quoted = TRUE) - Condition - Error in `purrr::pmap()`: - i In index: 1. - Caused by error: - ! Can't supply a schema when `table` is quoted. - -# must supply table and schema when catalog is used - - Code - new_table_ident(table = "my table", catalog = "cat") - Condition - Error in `purrr::pmap()`: - i In index: 1. - Caused by error: - ! Must supply `schema` when `catalog` is supplied. - Code - new_table_ident(schema = "schema", catalog = "cat") - Condition - Error in `purrr::pmap()`: - i In index: 1. - Caused by error: - ! Must supply `table` when `schema` is supplied. - ---- - - Code - new_table_ident(table = "my table", schema = c("my schema", NA), catalog = "cat") - Condition - Error in `purrr::pmap()`: - i In index: 2. - Caused by error: - ! Must supply `schema` when `catalog` is supplied. - -# can't coerce or cast to character - - Code - c(table, "character") - Condition - Error in `vec_c()`: - ! Can't combine `..1` and `..2` . - Code - as.character(table) - Condition - Error in `as.character()`: - ! Can't convert `x` to . - -# can print - - Code - new_table_ident(table = "table") - Output - - [1] `table` - Code - new_table_ident(schema = "schema", table = "table") - Output - - [1] `schema`.`table` - Code - new_table_ident(catalog = "catalog", schema = "schema", table = "table") - Output - - [1] `catalog`.`schema`.`table` - Code - new_table_ident(table = "`my schema`.`my table`", quoted = TRUE) - Output - - [1] `my schema`.`my table` - ---- - - Code - new_table_ident(table = c("`my schema`.`my table`", "table1", "table2", - "table3"), schema = c(NA, NA, "schema2", "schema3"), catalog = c(NA, NA, NA, - "catalog3"), quoted = c(TRUE, FALSE, FALSE, FALSE)) - Output - - [1] `my schema`.`my table` `table1` - [3] `schema2`.`table2` `catalog3`.`schema3`.`table3` - -# as_table_ident works - - Code - expect_equal(as_table_ident(ident_q("my schema.my table")), new_table_ident( - table = "my schema.my table", quoted = TRUE)) - Message - i Using `ident_q()` for a table identifier is intended as fallback in case of bugs. - i If you need it to work around a bug please open an issue . - This message is displayed once every 8 hours. - Code - expect_equal(as_table_ident(sql("my schema.my table")), new_table_ident(table = "my schema.my table", - quoted = TRUE)) - Message - i Using `sql()` for a table identifier is intended as fallback in case of bugs. - i If you need it to work around a bug please open an issue . - This message is displayed once every 8 hours. - diff --git a/tests/testthat/_snaps/table-name.md b/tests/testthat/_snaps/table-name.md new file mode 100644 index 000000000..f6e0ba6ae --- /dev/null +++ b/tests/testthat/_snaps/table-name.md @@ -0,0 +1,52 @@ +# table_path possess key methods + + Code + name <- table_path(c("x", "y", "z")) + name + Output + x, y, z + +# can check for table name + + Code + foo(1) + Condition + Error in `foo()`: + ! `y` must be a , not a string. + i This is an internal error that was detected in the dbplyr package. + Please report it at with a reprex () and the full backtrace. + +# as_table_path validates its inputs + + Code + as_table_path("x") + Condition + Error in `as_table_path()`: + ! `con` is absent but must be supplied. + Code + as_table_path(c("x", "y"), con) + Condition + Error: + ! `c("x", "y")` must be a single string, not a character vector. + Code + as_table_path(1, con) + Condition + Error in `as_table_path()`: + ! `1` uses unknown specification for table name + Code + as_table_path(I(1), con) + Condition + Error: + ! `I(1)` must be a single string, not the number 1. + +# as_table_path warns when using sql + + Code + as_table_path(sql("x"), con) + Condition + Warning: + `sql("x")` uses SQL where a table identifier is expected. + i If you want to use a literal (unquoted) identifier use `I()` instead. + Output + x + diff --git a/tests/testthat/_snaps/verb-joins.md b/tests/testthat/_snaps/verb-joins.md index 97a5dc471..9292e57c1 100644 --- a/tests/testthat/_snaps/verb-joins.md +++ b/tests/testthat/_snaps/verb-joins.md @@ -45,10 +45,10 @@ Code left_join(df4, df5, by = "x") %>% remote_query() Output - SELECT `LHS`.*, `z` - FROM `foo`.`df` AS `LHS` - LEFT JOIN `foo2`.`df` AS `RHS` - ON (`LHS`.`x` = `RHS`.`x`) + SELECT `df_LHS`.*, `z` + FROM foo.df AS `df_LHS` + LEFT JOIN foo2.df AS `df_RHS` + ON (`df_LHS`.`x` = `df_RHS`.`x`) # alias truncates long table names at database limit diff --git a/tests/testthat/test-backend-mssql.R b/tests/testthat/test-backend-mssql.R index a7f53d0c9..dc875f279 100644 --- a/tests/testthat/test-backend-mssql.R +++ b/tests/testthat/test-backend-mssql.R @@ -389,6 +389,19 @@ test_that("can copy_to() and compute() with temporary tables (#438)", { expect_equal(db2 %>% pull(), 2:4) }) +test_that("add prefix to temporary table", { + con <- simulate_mssql() + expect_snapshot( + out <- db_table_temporary(con, table_path("foo.bar"), temporary = TRUE) + ) + expect_equal(out, list(table = table_path("`foo`.`#bar`"), temporary = FALSE)) + + expect_silent( + out <- db_table_temporary(con, table_path("foo.#bar"), temporary = TRUE) + ) + expect_equal(out, list(table = table_path("foo.#bar"), temporary = FALSE)) +}) + test_that("bit conversion works for important cases", { df <- tibble(x = 1:3, y = 3:1) db <- copy_to(src_test("mssql"), df, name = unique_table_name("#")) diff --git a/tests/testthat/test-lazy-select-query.R b/tests/testthat/test-lazy-select-query.R index 9117d9fb1..4efdbe835 100644 --- a/tests/testthat/test-lazy-select-query.R +++ b/tests/testthat/test-lazy-select-query.R @@ -1,7 +1,9 @@ test_that("can print lazy_select_query", { + lf <- lazy_frame(x = 1, y = 2) + expect_snapshot( lazy_select_query( - x = lazy_query_local(tibble(x = 1, y = 2), "df"), + x = lf$lazy_query, select = quos( x_mean = mean(x), y2 = y diff --git a/tests/testthat/test-query-join.R b/tests/testthat/test-query-join.R index 6e347dbde..89f2529d2 100644 --- a/tests/testthat/test-query-join.R +++ b/tests/testthat/test-query-join.R @@ -76,8 +76,8 @@ test_that("sql_multi_join_vars generates expected SQL", { all_x = c("x", "a"), all_y = c("x", "a", "b") ), - x_as = ident("left"), - y_as = ident("right"), + x_as = table_path("left"), + y_as = table_path("right"), use_star = TRUE, qualify_all_columns = FALSE ), @@ -101,8 +101,8 @@ test_that("sql_multi_join_vars generates expected SQL", { all_x = c("a", "b"), all_y = c("a", "B") ), - x_as = ident("left"), - y_as = ident("right"), + x_as = table_path("left"), + y_as = table_path("right"), use_star = TRUE, qualify_all_columns = FALSE ), diff --git a/tests/testthat/test-query-select.R b/tests/testthat/test-query-select.R index afd868b6c..0266468fc 100644 --- a/tests/testthat/test-query-select.R +++ b/tests/testthat/test-query-select.R @@ -26,7 +26,7 @@ test_that("optimisation is turned on by default", { lf <- lazy_frame(x = 1, y = 2) %>% arrange(x) %>% head(5) qry <- lf %>% sql_build() - expect_equal(qry$from, base_query(ident("df"))) + expect_equal(qry$from, base_query(table_path("`df`"))) }) test_that("group by then limit is collapsed", { diff --git a/tests/testthat/test-remote.R b/tests/testthat/test-remote.R index 6509a17e2..be97dec5b 100644 --- a/tests/testthat/test-remote.R +++ b/tests/testthat/test-remote.R @@ -4,13 +4,13 @@ test_that("remote_table returns name when it makes sense", { # produces name after `group_by()` expect_equal( mf %>% group_by(x) %>% remote_table(), - as_table_ident(ident("refxiudlph")) + table_path("`refxiudlph`") ) # produces name after unarranging expect_equal( mf %>% arrange(x) %>% arrange() %>% remote_table(), - as_table_ident(ident("refxiudlph")) + table_path("`refxiudlph`") ) # produces name after compute() @@ -19,7 +19,7 @@ test_that("remote_table returns name when it makes sense", { test_that("remote_table returns null for computed tables", { mf <- copy_to_test("sqlite", tibble(x = 5, y = 1), name = "refxiudlph") - expect_equal(remote_table(mf), as_table_ident(ident("refxiudlph"))) + expect_equal(remote_table(mf), table_path("`refxiudlph`")) expect_null(mf %>% filter(x == 3) %>% remote_table()) expect_null(mf %>% distinct(x) %>% remote_table()) @@ -34,15 +34,15 @@ test_that("remote_table returns null for computed tables", { expect_null(lf %>% group_by(x) %>% remote_table()) lf <- lazy_frame(x = 1) - expect_equal(lf %>% remote_table(null_if_local = FALSE), as_table_ident(ident("df"))) - expect_equal(lf %>% group_by(x) %>% remote_table(null_if_local = FALSE), as_table_ident(ident("df"))) + expect_equal(lf %>% remote_table(null_if_local = FALSE), table_path("`df`")) + expect_equal(lf %>% group_by(x) %>% remote_table(null_if_local = FALSE), table_path("`df`")) }) test_that("remote_name and remote_table can handle different table identifiers", { - test_remote_table <- function(x, exp_tbl = as_table_ident(x), exp_name = "tbl") { + test_remote_table <- function(x, exp_tbl = as_table_path(x, simulate_sqlite())) { lf <- lazy_frame(x = 1, .name = x) expect_equal(remote_table(lf, null_if_local = FALSE), exp_tbl) - expect_equal(remote_name(lf, null_if_local = FALSE), exp_name) + expect_equal(remote_name(lf, null_if_local = FALSE), "tbl") } test_remote_table("tbl") @@ -50,10 +50,8 @@ test_that("remote_name and remote_table can handle different table identifiers", test_remote_table(in_schema("schema", "tbl")) test_remote_table(in_catalog("catalog", "schema", "tbl")) test_remote_table(DBI::Id(catalog = "catalog", schema = "schema", table = "tbl")) - - withr::local_options(rlib_message_verbosity = "quiet") - test_remote_table(ident_q("schema.tbl"), exp_name = NULL) - test_remote_table(sql("schema.tbl"), exp_name = NULL) + test_remote_table(ident_q("schema.tbl")) + test_remote_table(I("schema.tbl")) }) test_that("can retrieve query, src and con metadata", { diff --git a/tests/testthat/test-table-ident.R b/tests/testthat/test-table-ident.R deleted file mode 100644 index 5636abde1..000000000 --- a/tests/testthat/test-table-ident.R +++ /dev/null @@ -1,144 +0,0 @@ -test_that("can create table idents", { - expect_equal( - new_table_ident( - table = c("A", "B", "C"), - schema = "schema" - ), - vctrs::new_rcrd(list( - table = c("A", "B", "C"), - schema = vctrs::vec_rep("schema", 3), - catalog = vctrs::vec_rep(NA_character_, 3), - quoted = vctrs::vec_rep(FALSE, 3), - alias = vctrs::vec_rep(NA_character_, 3) - ), class = "dbplyr_table_ident") - ) -}) - -test_that("is properly vectorised", { - expect_equal( - new_table_ident( - table = c("A", "B"), - schema = c("schema1", "schema2"), - catalog = c("cat1", "cat2") - ), - vctrs::new_rcrd(list( - table = c("A", "B"), - schema = c("schema1", "schema2"), - catalog = c("cat1", "cat2"), - quoted = vctrs::vec_rep(FALSE, 2), - alias = vctrs::vec_rep(NA_character_, 2) - ), class = "dbplyr_table_ident") - ) - - expect_snapshot(error = TRUE, { - new_table_ident(table = c("A", "B", "c"), schema = c("schema1", "schema2")) - }) -}) - -test_that("can't supply table and sql", { - expect_snapshot(error = TRUE, { - new_table_ident(schema = "my schema", table = "my table", quoted = TRUE) - }) -}) - -test_that("must supply table and schema when catalog is used", { - expect_snapshot(error = TRUE, { - new_table_ident(table = "my table", catalog = "cat") - new_table_ident(schema = "schema", catalog = "cat") - }) - - # also works when schema is a vector - expect_snapshot(error = TRUE, { - new_table_ident(table = "my table", schema = c("my schema", NA), catalog = "cat") - }) -}) - -test_that("can't coerce or cast to character", { - table <- new_table_ident(table = "table") - expect_snapshot(error = TRUE, { - c(table, "character") - as.character(table) - }) -}) - -test_that("can print", { - expect_snapshot({ - new_table_ident(table = "table") - new_table_ident(schema = "schema", table = "table") - new_table_ident(catalog = "catalog", schema = "schema", table = "table") - new_table_ident(table = "`my schema`.`my table`", quoted = TRUE) - }) - - # is correctly vectorised - expect_snapshot( - new_table_ident( - table = c("`my schema`.`my table`", "table1", "table2", "table3"), - schema = c( NA, NA, "schema2", "schema3"), - catalog = c( NA, NA, NA, "catalog3"), - quoted = c( TRUE, FALSE, FALSE, FALSE) - ) - ) -}) - -test_that("as_table_ident works", { - expect_equal( - as_table_ident("table"), - new_table_ident(table = "table") - ) - - expect_equal( - as_table_ident(ident("table")), - new_table_ident(table = "table") - ) - - expect_equal( - as_table_ident(in_schema("schema", "table")), - new_table_ident(schema = "schema", table = "table") - ) - - expect_equal( - as_table_ident(in_catalog("catalog", "schema", "table")), - new_table_ident(catalog = "catalog", schema = "schema", table = "table") - ) - - expect_equal( - as_table_ident(DBI::Id(catalog = "catalog", schema = "schema", table = "table")), - new_table_ident(catalog = "catalog", schema = "schema", table = "table") - ) - - table <- new_table_ident(table = "table") - expect_equal( - as_table_ident(table), - table - ) - - withr::local_options(rlib_message_verbosity = "verbose") - expect_snapshot({ - expect_equal( - as_table_ident(ident_q("my schema.my table")), - new_table_ident(table = "my schema.my table", quoted = TRUE) - ) - expect_equal( - as_table_ident(sql("my schema.my table")), - new_table_ident(table = "my schema.my table", quoted = TRUE) - ) - }) -}) - -test_that("escaped as needed", { - con <- simulate_dbi() - out <- c( - as_table_ident(ident("table1")), - as_table_ident(in_schema("schema2", "table2")), - as_table_ident(in_catalog("catalog3", "schema3", "table3")) - ) - - expect_equal( - escape(out, collapse = NULL, con = simulate_dbi()), - sql( - "`table1`", - "`schema2`.`table2`", - "`catalog3`.`schema3`.`table3`" - ) - ) -}) diff --git a/tests/testthat/test-table-name.R b/tests/testthat/test-table-name.R new file mode 100644 index 000000000..ae1a73868 --- /dev/null +++ b/tests/testthat/test-table-name.R @@ -0,0 +1,90 @@ +test_that("table_path possess key methods", { + expect_snapshot({ + name <- table_path(c("x", "y", "z")) + name + }) + + expect_equal(name[c(1, 3)], table_path(c("x", "z"))) + expect_equal(name[[2]], table_path("y")) + expect_equal(c(name[[1]], name[[2]]), table_path(c("x", "y"))) +}) + +test_that("can check for table name", { + foo <- function(y) check_table_path(y) + expect_snapshot(foo(1), error = TRUE) +}) + +# as_table_path ----------------------------------------------------------- + +test_that("can coerce all user facing inputs", { + con <- simulate_dbi() + + x_esc <- table_path("`x`") + x_raw <- table_path("x") + + id <- table_path("x") + expect_true(is_table_id(id)) + expect_equal(as_table_path(id, con), x_raw) + + id <- "x" + expect_true(is_table_id(id)) + expect_equal(as_table_path(id, con), x_esc) + + id <- I("x") + expect_true(is_table_id(id)) + expect_equal(as_table_path(id, con), x_raw) + + id <- ident("x") + expect_true(is_table_id(id)) + expect_equal(as_table_path(id, con), x_esc) + + id <- ident_q("x") + expect_true(is_table_id(id)) + expect_equal(as_table_path(id, con), x_raw) + + id <- DBI::Id(schema = "foo", table = "bar") + expect_true(is_table_id(id)) + expect_equal(as_table_path(id, con), table_path("`foo`.`bar`")) + + # strip names, simulating DBI 1.2.0 + names(id@name) <- NULL + expect_equal(as_table_path(id, con), table_path("`foo`.`bar`")) + + id <- in_schema("foo", "bar") + expect_true(is_table_id(id)) + expect_equal(as_table_path(id, con), table_path("`foo`.`bar`")) + + id <- in_catalog("foo", "bar", "baz") + expect_true(is_table_id(id)) + expect_equal(as_table_path(id, con), table_path("`foo`.`bar`.`baz`")) + + id <- in_catalog("foo", sql("bar"), ident_q("baz")) + expect_true(is_table_id(id)) + expect_equal(as_table_path(id, con), table_path("`foo`.bar.baz")) +}) + +test_that("strips names", { + con <- simulate_dbi() + expect_equal(as_table_path(c(x = "x"), con), table_path("`x`")) + + id <- in_schema(c(x = "a"), "b") + expect_equal(as_table_path(id, con), table_path("`a`.`b`")) + + id <- in_catalog("a", "b", c(x = "c")) + expect_equal(as_table_path(id, con), table_path("`a`.`b`.`c`")) +}) + +test_that("as_table_path validates its inputs", { + con <- simulate_dbi() + expect_snapshot(error = TRUE, { + as_table_path("x") + as_table_path(c("x", "y"), con) + as_table_path(1, con) + as_table_path(I(1), con) + }) +}) + +test_that("as_table_path warns when using sql", { + con <- simulate_dbi() + expect_snapshot(as_table_path(sql("x"), con)) +}) diff --git a/tests/testthat/test-tbl-lazy.R b/tests/testthat/test-tbl-lazy.R index f239d063b..710d93ae3 100644 --- a/tests/testthat/test-tbl-lazy.R +++ b/tests/testthat/test-tbl-lazy.R @@ -38,7 +38,7 @@ test_that("support colwise variants", { test_that("base source of tbl_lazy is always 'df'", { out <- lazy_frame(x = 1, y = 5) %>% sql_build() - expect_equal(out, base_query(ident("df"))) + expect_equal(out, base_query(table_path("`df`"))) }) test_that("names() inform that they aren't meant to be used", { diff --git a/tests/testthat/test-tbl-sql.R b/tests/testthat/test-tbl-sql.R index 774674081..5177faaff 100644 --- a/tests/testthat/test-tbl-sql.R +++ b/tests/testthat/test-tbl-sql.R @@ -44,18 +44,6 @@ test_that("can refer to default schema explicitly", { }) -test_that("checks for incorrectly specified SQL or schema", { - con <- local_sqlite_connection() - DBI::dbExecute(con, "CREATE TABLE 'table.with_dot' (a, b, c)") - - expect_message(tbl(con, ident("table.with_dot")), "in a schema") - expect_no_message(tbl(con, ident("table.with_dot"), check_from = FALSE)) - - expect_error( - expect_message(tbl(con, ident("SELECT * FROM table.with_dot")), "an SQL query as source") - ) -}) - test_that("can distinguish 'schema.table' from 'schema'.'table'", { con <- local_sqlite_con_with_aux() DBI::dbExecute(con, "CREATE TABLE aux.t1 (x, y, z)") @@ -85,7 +73,7 @@ test_that("ungrouped output", { out1 <- tbl_sum(mf) expect_named(out1, c("Source", "Database")) - expect_equal(out1[["Source"]], "table [?? x 2]") + expect_equal(out1[["Source"]], "table<`tbl_sum_test`> [?? x 2]") expect_match(out1[["Database"]], "sqlite (.*) \\[:memory:\\]") out2 <- tbl_sum(mf %>% group_by(x, y)) diff --git a/tests/testthat/test-verb-compute.R b/tests/testthat/test-verb-compute.R index 2b9b7392f..21ac3ac49 100644 --- a/tests/testthat/test-verb-compute.R +++ b/tests/testthat/test-verb-compute.R @@ -83,10 +83,10 @@ test_that("compute keeps window and groups", { }) test_that("compute can handle named name", { - name <- set_names(unique_subquery_name(), unique_subquery_name()) + con <- simulate_dbi() expect_equal( memdb_frame(x = 1:10) %>% - compute() %>% + compute(name = c(x = unique_table_name())) %>% collect(), tibble(x = 1:10) ) diff --git a/tests/testthat/test-verb-joins.R b/tests/testthat/test-verb-joins.R index 7bd93e7c9..7a17172ff 100644 --- a/tests/testthat/test-verb-joins.R +++ b/tests/testthat/test-verb-joins.R @@ -84,16 +84,17 @@ test_that("join works with in_schema", { left_join(df1, df3, by = "x") %>% remote_query() ) - # use auto name for `ident_q` - # suppress info about only meant as a workaround - withr::local_options(rlib_message_verbosity = "quiet") - df4 <- tbl(con, ident_q("`foo`.`df`")) - df5 <- tbl(con, ident_q("`foo2`.`df`")) + # use auto name for escaped table names + df4 <- tbl(con, I("foo.df")) + df5 <- tbl(con, I("foo2.df")) expect_snapshot( left_join(df4, df5, by = "x") %>% remote_query() ) out <- left_join(df4, df5, by = "x") - expect_equal(out$lazy_query$table_names, tibble(name = c("", ""), from = "")) + expect_equal(out$lazy_query$table_names, tibble( + name = table_path(c("foo.df", "foo2.df")), + from = c("name", "name") + )) }) test_that("alias truncates long table names at database limit", { @@ -101,11 +102,11 @@ test_that("alias truncates long table names at database limit", { # Source: https://www.postgresql.org/docs/current/sql-syntax-lexical.html#SQL-SYNTAX-IDENTIFIERS con <- src_test("postgres") - nm1 <- paste0("a", paste0(0:61 %% 10, collapse = "")) - mf1 <- local_db_table(con, tibble(x = 1:3, y = "a"), nm1) + nm1 <- table_path(paste0("a", paste0(0:61 %% 10, collapse = ""))) + mf1 <- local_db_table(con, tibble(x = 1:3, y = "a"), unclass(nm1)) - nm2 <- paste0("b", paste0(0:61 %% 10, collapse = "")) - mf2 <- local_db_table(con, tibble(x = 2:3, y = "b"), nm2) + nm2 <- table_path(paste0("b", paste0(0:61 %% 10, collapse = ""))) + mf2 <- local_db_table(con, tibble(x = 2:3, y = "b"), unclass(nm2)) # 2 tables # aliased names are as expected @@ -113,10 +114,10 @@ test_that("alias truncates long table names at database limit", { tibble::tibble( name = c(nm1, nm1), from = "name" - ) + ), + con ) - - expect_equal(max(nchar(self_join2_names)), 63) + expect_equal(max(nchar(self_join2_names)), 65) expect_equal( length(self_join2_names), length(unique(self_join2_names)) @@ -140,10 +141,11 @@ test_that("alias truncates long table names at database limit", { tibble::tibble( name = c(nm1, nm1, nm2), from = "name" - ) + ), + con ) - expect_equal(max(nchar(self_join3_names)), 63) + expect_equal(max(nchar(self_join3_names)), 65) expect_equal( length(self_join3_names), length(unique(self_join3_names)) @@ -288,57 +290,57 @@ test_that("join uses correct table alias", { y <- lazy_frame(a = 1, y = 1, .name = "y") # self joins - table_names <- sql_build(left_join(x, x, by = "a"))$table_names - expect_equal(table_names, c("x_LHS", "x_RHS")) + table_paths <- sql_build(left_join(x, x, by = "a"))$table_names + expect_equal(table_paths, table_path(c("`x_LHS`", "`x_RHS`"))) - table_names <- sql_build(left_join(x, x, by = "a", x_as = "my_x"))$table_names - expect_equal(table_names, c("my_x", "x")) + table_paths <- sql_build(left_join(x, x, by = "a", x_as = "my_x"))$table_names + expect_equal(table_paths, table_path(c("`my_x`", "`x`"))) - table_names <- sql_build(left_join(x, x, by = "a", y_as = "my_y"))$table_names - expect_equal(table_names, c("x", "my_y")) + table_paths <- sql_build(left_join(x, x, by = "a", y_as = "my_y"))$table_names + expect_equal(table_paths, table_path(c("`x`", "`my_y`"))) - table_names <- sql_build(left_join(x, x, by = "a", x_as = "my_x", y_as = "my_y"))$table_names - expect_equal(table_names, c("my_x", "my_y")) + table_paths <- sql_build(left_join(x, x, by = "a", x_as = "my_x", y_as = "my_y"))$table_names + expect_equal(table_paths, table_path(c("`my_x`", "`my_y`"))) # x-y joins - table_names <- sql_build(left_join(x, y, by = "a"))$table_names - expect_equal(table_names, c("x", "y")) + table_paths <- sql_build(left_join(x, y, by = "a"))$table_names + expect_equal(table_paths, table_path(c("`x`", "`y`"))) - table_names <- sql_build(left_join(x, y, by = "a", x_as = "my_x"))$table_names - expect_equal(table_names, c("my_x", "y")) + table_paths <- sql_build(left_join(x, y, by = "a", x_as = "my_x"))$table_names + expect_equal(table_paths, table_path(c("`my_x`", "`y`"))) - table_names <- sql_build(left_join(x, y, by = "a", y_as = "my_y"))$table_names - expect_equal(table_names, c("x", "my_y")) + table_paths <- sql_build(left_join(x, y, by = "a", y_as = "my_y"))$table_names + expect_equal(table_paths, table_path(c("`x`", "`my_y`"))) - table_names <- sql_build(left_join(x, y, by = "a", x_as = "my_x", y_as = "my_y"))$table_names - expect_equal(table_names, c("my_x", "my_y")) + table_paths <- sql_build(left_join(x, y, by = "a", x_as = "my_x", y_as = "my_y"))$table_names + expect_equal(table_paths, table_path(c("`my_x`", "`my_y`"))) # x_as same name as `y` - table_names <- sql_build(left_join(x, y, by = "a", x_as = "y"))$table_names - expect_equal(table_names, c("y", "y...2")) + table_paths <- sql_build(left_join(x, y, by = "a", x_as = "y"))$table_names + expect_equal(table_paths, table_path(c("`y`", "`y...2`"))) - table_names <- sql_build(left_join(x %>% filter(x == 1), x, by = "x", y_as = "LHS"))$table_names - expect_equal(table_names, c("LHS...1", "LHS")) + table_paths <- sql_build(left_join(x %>% filter(x == 1), x, by = "x", y_as = "LHS"))$table_names + expect_equal(table_paths, table_path(c("`LHS...1`", "`LHS`"))) # sql_on -> use alias or LHS/RHS - table_names <- sql_build(left_join(x, y, sql_on = sql("LHS.a = RHS.a")))$table_names - expect_equal(table_names, c("LHS", "RHS")) + table_paths <- sql_build(left_join(x, y, sql_on = sql("LHS.a = RHS.a")))$table_names + expect_equal(table_paths, table_path(c("`LHS`", "`RHS`"))) - table_names <- sql_build(left_join(x, y, x_as = "my_x", sql_on = sql("my_x.a = RHS.a")))$table_names - expect_equal(table_names, c("my_x", "RHS")) + table_paths <- sql_build(left_join(x, y, x_as = "my_x", sql_on = sql("my_x.a = RHS.a")))$table_names + expect_equal(table_paths, table_path(c("`my_x`", "`RHS`"))) # triple join z <- lazy_frame(a = 1, z = 1, .name = "z") out <- left_join(x, y, by = "a") %>% left_join(z, by = "a") %>% sql_build() - expect_equal(out$table_names, c("x", "y", "z")) + expect_equal(out$table_names, table_path(c("`x`", "`y`", "`z`"))) # triple join where names need to be repaired out <- left_join(x, x, by = "a") %>% left_join(z, by = "a") %>% sql_build() - expect_equal(out$table_names, c("x...1", "x...2", "z")) + expect_equal(out$table_names, table_path(c("`x...1`", "`x...2`", "`z`"))) }) test_that("select() before join is inlined", { @@ -623,7 +625,10 @@ test_that("multiple joins create a single query", { inner_join(lf3, by = "x") lq <- out$lazy_query expect_s3_class(lq, "lazy_multi_join_query") - expect_equal(lq$table_names, tibble(name = c("df1", "df2", "df3"), from = "name")) + expect_equal(lq$table_names, tibble( + name = table_path(c("`df1`", "`df2`", "`df3`")), + from = "name" + )) expect_equal(lq$vars$name, c("x", "a", "b.x", "b.y")) expect_equal(lq$vars$table, c(1L, 1L, 2L, 3L)) expect_equal(lq$vars$var, c("x", "a", "b", "b")) @@ -722,7 +727,10 @@ test_that("multi joins work with x_as", { inner_join(lf3, by = "x", y_as = "lf3") lq <- out$lazy_query expect_s3_class(lq, "lazy_multi_join_query") - expect_equal(lq$table_names, tibble(name = c("lf1", "lf2", "lf3"), from = "as")) + expect_equal( + lq$table_names, + tibble(name = table_path(c("`lf1`", "`lf2`", "`lf3`")), from = "as") + ) # `x_as` provided twice with the same name -> one query out2 <- left_join(lf, lf2, by = "x", x_as = "lf1", y_as = "lf2") %>% diff --git a/tests/testthat/test-verb-set-ops.R b/tests/testthat/test-verb-set-ops.R index b5e221fd3..a8db07ca6 100644 --- a/tests/testthat/test-verb-set-ops.R +++ b/tests/testthat/test-verb-set-ops.R @@ -17,10 +17,10 @@ test_that("missing columns filled with NULL", { test_that("first edition works", { con <- structure(list(), class = c("Test", "DBIConnection")) - lf <- lazy_frame(x = 1, con = con) local_methods( sql_escape_ident.Test = function(con, x) sql_quote(x, "`") ) + lf <- lazy_frame(x = 1, con = con) local_options(rlib_warning_verbosity = "quiet") diff --git a/vignettes/setup/_postgres.Rmd b/vignettes/setup/_postgres.Rmd index c187c0bea..1a2744e1b 100644 --- a/vignettes/setup/_postgres.Rmd +++ b/vignettes/setup/_postgres.Rmd @@ -6,13 +6,12 @@ First install PostgreSQL, create a data directory, and create a default database ``` brew install postgresql -export PGDATA=~/db/postgres-9.5 # set this globally somewhere -initdb -E utf8 -createdb -createdb hadley +export PGDATA=~/db/postgres # set this globally somewhere +initdb createdb test -createdb lahman -createdb nycflights13 + +psql -c "CREATE USER postgres WITH PASSWORD 'password';" +psql -c "ALTER USER postgres WITH SUPERUSER;" ``` ## Start @@ -24,8 +23,14 @@ pg_ctl start ## Connect ```{r, eval = FALSE} -install.packages("RPostgreSQL") library(DBI) -con <- dbConnect(RPostgreSQL::PostgreSQL(), dbname = "hadley") +con <- dbConnect(RPostgres::Postgres(), dbname = "hadley") dbListTables(con) + +con <- dbConnect( + RPostgres::Postgres(), + dbname = "test", + user = "postgres", + password = "passowrd" +) ```