From 8105d7b6b4b485d09e72c92589c758f94d4e980f Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Tue, 7 Nov 2023 14:15:57 -0600 Subject: [PATCH 01/51] First stab --- DESCRIPTION | 1 + NAMESPACE | 2 + R/backend-mssql.R | 5 +- R/backend-mysql.R | 2 +- R/backend-postgres-old.R | 4 +- R/backend-postgres.R | 2 +- R/backend-spark-sql.R | 2 +- R/build-sql.R | 4 +- R/db-io.R | 14 +- R/db-sql.R | 84 +++-- R/lazy-ops.R | 4 +- R/query-join.R | 5 +- R/remote.R | 8 +- R/table-ident.R | 8 - R/table-name.R | 109 ++++++ R/tbl-sql.R | 33 +- R/verb-compute.R | 2 +- R/verb-copy-to.R | 2 +- tests/testthat/_snaps/backend-mssql.new.md | 383 +++++++++++++++++++++ tests/testthat/_snaps/db-io.md | 2 +- tests/testthat/_snaps/db-sql.new.md | 5 + tests/testthat/test-tbl-sql.R | 12 - 22 files changed, 569 insertions(+), 124 deletions(-) create mode 100644 R/table-name.R create mode 100644 tests/testthat/_snaps/backend-mssql.new.md create mode 100644 tests/testthat/_snaps/db-sql.new.md diff --git a/DESCRIPTION b/DESCRIPTION index 2b4f3d173..ca586111a 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -127,6 +127,7 @@ Collate: '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 9677e24b3..9bba386d3 100644 --- a/NAMESPACE +++ b/NAMESPACE @@ -120,6 +120,7 @@ S3method(escape,data.frame) S3method(escape,dbplyr_catalog) S3method(escape,dbplyr_schema) S3method(escape,dbplyr_table_ident) +S3method(escape,dbplyr_table_name) S3method(escape,double) S3method(escape,factor) S3method(escape,ident) @@ -178,6 +179,7 @@ S3method(print,base_query) S3method(print,dbplyr_catalog) S3method(print,dbplyr_schema) S3method(print,dbplyr_sql_options) +S3method(print,dbplyr_table_name) S3method(print,ident) S3method(print,join_query) S3method(print,lazy_base_local_query) diff --git a/R/backend-mssql.R b/R/backend-mssql.R index 830f51b26..762355620 100644 --- a/R/backend-mssql.R +++ b/R/backend-mssql.R @@ -469,7 +469,10 @@ mssql_version <- function(con) { # #' @export `db_table_temporary.Microsoft SQL Server` <- function(con, table, temporary, ...) { - table <- as_table_ident(table) + table <- as_table_name(table, con) + + # TODO: FIX ME + table_name <- vctrs::field(table, "table") if (temporary && substr(table_name, 1, 1) != "#") { diff --git a/R/backend-mysql.R b/R/backend-mysql.R index f40093475..8e7aad7a3 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_name(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..36b406b77 100644 --- a/R/backend-postgres-old.R +++ b/R/backend-postgres-old.R @@ -15,7 +15,7 @@ db_write_table.PostgreSQLConnection <- function(con, values, temporary = TRUE, ...) { - table <- as_table_ident(table) + table <- as_table_name(table, con) if (!isFALSE(temporary)) { cli_abort(c( "RPostgreSQL backend does not support creation of temporary tables", @@ -27,7 +27,7 @@ db_write_table.PostgreSQLConnection <- function(con, # the bare table name dbWriteTable( con, - name = vctrs::field(table, "table"), + name = SQL(unclass(table)), value = values, field.types = types, ..., diff --git a/R/backend-postgres.R b/R/backend-postgres.R index 812c033bb..929b1114b 100644 --- a/R/backend-postgres.R +++ b/R/backend-postgres.R @@ -407,7 +407,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_name(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 79a556107..67911290b 100644 --- a/R/backend-spark-sql.R +++ b/R/backend-spark-sql.R @@ -115,7 +115,7 @@ simulate_spark_sql <- function() simulate_dbi("Spark SQL") cli::cli_abort("Spark SQL only support temporary tables") } - table <- as_table_ident(table) + table <- as_table_name(table, con) sql <- glue_sql2( con, "CREATE ", if (overwrite) "OR REPLACE ", diff --git a/R/build-sql.R b/R/build-sql.R index 97449ae1a..9f709dace 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_name(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 7dcdfba47..1d0b80f1d 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,14 +65,14 @@ db_copy_to.DBIConnection <- function(con, indexes = NULL, analyze = TRUE, in_transaction = TRUE) { - table <- as_table_ident(table) + table <- as_table_name(table, con) new <- db_table_temporary(con, table, temporary) table <- new$table temporary <- new$temporary call <- current_env() with_transaction(con, in_transaction, { - tryCatch( + withCallingHandlers( { table <- dplyr::db_write_table(con, table, types = types, @@ -106,7 +106,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) @@ -126,7 +126,7 @@ db_compute.DBIConnection <- function(con, indexes = list(), analyze = TRUE, in_transaction = FALSE) { - table <- as_table_ident(table) + table <- as_table_name(table, con) new <- db_table_temporary(con, table, temporary) table <- new$table temporary <- new$temporary @@ -178,7 +178,7 @@ db_write_table.DBIConnection <- function(con, temporary = TRUE, ..., overwrite = FALSE) { - table <- as_table_ident(table) + table <- as_table_name(table, con) check_character(types, allow_null = TRUE) check_named(types) check_bool(temporary) @@ -187,7 +187,7 @@ db_write_table.DBIConnection <- function(con, tryCatch( dbWriteTable( con, - name = table_ident_to_id(table), + name = SQL(unclass(table)), value = values, field.types = types, temporary = temporary, diff --git a/R/db-sql.R b/R/db-sql.R index c5787f6e8..8ff4bf3a4 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,8 @@ 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 = "_") - - name <- name %||% paste0(c(table_name, columns), collapse = "_") + table <- as_table_name(table, con, error_call = call) + name <- name %||% paste0(c(table, columns), collapse = "_") glue_sql2( con, "CREATE ", if (unique) "UNIQUE ", "INDEX {.name name}", @@ -201,23 +199,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 +221,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_name(name, con) glue_sql2( con, @@ -245,7 +240,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 +252,11 @@ 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, "{.name name}") + } else { # must be a table_name + name <- sql_escape_ident(con, name) + setNames(from, name) } - - set_table_ident_alias(from, name) } #' @export @@ -286,14 +281,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 +765,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(sql) check_character(insert_cols) check_character(by) check_character(returning_cols, allow_null = TRUE) @@ -790,8 +785,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_name(table, con) + from <- as_table_source(from, con) method <- method %||% "where_not_exists" arg_match(method, "where_not_exists", error_arg = "method") @@ -838,8 +833,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 +849,8 @@ sql_query_append.DBIConnection <- function(con, insert_cols, ..., returning_cols = NULL) { - table <- as_table_ident(table) - from <- as_from(from) + table <- as_table_name(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 +875,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 +894,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_name(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 +923,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 +944,8 @@ sql_query_upsert.DBIConnection <- function(con, ..., returning_cols = NULL, method = NULL) { - table <- as_table_ident(table) - from <- as_from(from) + table <- as_table_name(table, con) + from <- as_table_source(from, con) method <- method %||% "cte_update" arg_match(method, "cte_update", error_arg = "method") @@ -998,8 +993,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 +1009,8 @@ sql_query_delete.DBIConnection <- function(con, by, ..., returning_cols = NULL) { - table <- as_table_ident(table) - from <- as_from(from) + table <- as_table_name(table) + from <- as_table_source(from, con) parts <- rows_prep(con, table, from, by, lvl = 1) clauses <- list2( @@ -1143,15 +1138,14 @@ db_save_query.DBIConnection <- function(con, temporary = TRUE, ..., overwrite = FALSE) { - sql <- sql_query_save(con, sql, name, temporary = temporary, ...) - tryCatch( + name <- as_table_name(name, con) + sql <- sql_query_save(con, sql(sql), name, temporary = temporary, ...) + withCallingHandlers( { 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(unclass(name))) if (found) { - DBI::dbRemoveTable(con, name_id) + DBI::dbRemoveTable(con, SQL(unclass(name))) } } DBI::dbExecute(con, sql, immediate = TRUE) diff --git a/R/lazy-ops.R b/R/lazy-ops.R index 14199a42c..cd503fb0c 100644 --- a/R/lazy-ops.R +++ b/R/lazy-ops.R @@ -32,7 +32,7 @@ lazy_base_query <- function(x, vars, class = character(), ...) { } lazy_query_local <- function(df, name) { - name <- as_table_ident(name) + name <- as_table_name(I(name)) lazy_base_query(df, names(df), class = "local", name = name) } @@ -41,7 +41,7 @@ lazy_query_remote <- function(x, vars) { } base_query <- function(from) { - from <- as_from(from) + check_table_source(from) structure( list(from = from), class = c("base_query", "query") diff --git a/R/query-join.R b/R/query-join.R index afe2840ff..38c82228e 100644 --- a/R/query-join.R +++ b/R/query-join.R @@ -337,7 +337,7 @@ sql_table_prefix <- function(con, var, table = NULL) { var <- sql_escape_ident(con, var) if (!is.null(table)) { - table <- as_table_ident(table) + table <- as_table_name(table, con) table <- escape(table, collapse = NULL, con = con) sql(paste0(table, ".", var)) } else { @@ -348,8 +348,7 @@ sql_table_prefix <- function(con, var, table = NULL) { sql_star <- function(con, table = NULL) { var <- sql("*") if (!is.null(table)) { - table <- as_table_ident(table) - table <- escape(table, collapse = NULL, con = con) + stopifnot(is_table_name(table)) sql(paste0(table, ".", var)) } else { var diff --git a/R/remote.R b/R/remote.R index 3366e2663..477aff1d6 100644 --- a/R/remote.R +++ b/R/remote.R @@ -33,11 +33,11 @@ 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)) { + NULL + } else { + table } - - table_ident_name(table) } #' @export diff --git a/R/table-ident.R b/R/table-ident.R index 50bb029c7..00efb6ce8 100644 --- a/R/table-ident.R +++ b/R/table-ident.R @@ -231,14 +231,6 @@ set_table_ident_alias <- function(x, alias) { 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) diff --git a/R/table-name.R b/R/table-name.R new file mode 100644 index 000000000..492b7e830 --- /dev/null +++ b/R/table-name.R @@ -0,0 +1,109 @@ + +# 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_name(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)") + } +} + +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_name(x) || + is.ident(x) || + is(x, "Id") || + is_catalog(x) || + is_schema(x) || + is.character(x) +} + +as_table_name <- function(x, + con, + error_arg = caller_arg(x), + error_call = caller_env()) { + if (is_table_name(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 string without escaping using {.fn I} instead" + ) + ) + table_name(unclass(x)) + } else if (inherits(x, "ident_q")) { + table_name(paste0(x, collapse = ".")) + } else if (is.ident(x)) { + make_table_name(unclass(x), con) + } else if (is(x, "Id")) { + table_name(DBI::dbQuoteLiteral(con, x)) + } else if (inherits(x, "dbplyr_catalog")) { + make_table_name(c(unclass(x$catalog), unclass(x$schema), unclass(x$table)), con) + } else if (inherits(x, "dbplyr_schema")) { + make_table_name(c(unclass(x$schema), unclass(x$table)), con) + } else if (inherits(x, "AsIs")) { + check_string(unclass(x), allow_empty = FALSE, arg = error_arg, call = error_call) + table_name(unclass(x)) + } else if (is.character(x)) { + make_table_name(x, con) + } else { + cli::cli_abort( + "{.arg {error_arg}} uses specification for table name", + error_call = error_call + ) + } +} + +make_table_name <- function(x, con) { + needs_quote <- !vapply(x, function(x) inherits(x, "AsIs"), logical(1)) + x[needs_quote] <- sql_escape_ident(con, x[needs_quote]) + + table_name(paste0(x, collapse = ".")) +} + +table_name <- function(x) { + structure(x, class = "dbplyr_table_name") +} + +#' @export +print.dbplyr_table_name <- function(x) { + cat(" ", style_kw(x), "\n", sep = "") +} + +is_table_name <- function(x) { + inherits(x, "dbplyr_table_name") +} + +#' @export +escape.dbplyr_table_name <- function(x, parens = FALSE, collapse = ", ", con = NULL) { + + alias <- names2(x) + x <- unname(x) + + if (db_supports_table_alias_with_as(con)) { + as_sql <- style_kw(" AS ") + } else { + as_sql <- " " + } + + # TODO: Why is this getting double escaped? + # alias_esc <- sql_escape_ident(con, alias) + + out <- ifelse(alias == "" | alias == x, x, paste0(x, as_sql, alias)) + sql_vector(out, parens, collapse, con = con) +} diff --git a/R/tbl-sql.R b/R/tbl-sql.R index 2e4f6a140..a6a320553 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) { check_dots_used() 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/verb-compute.R b/R/verb-compute.R index 1303e82b8..16822014f 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_name(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..09f714898 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_name(name, dest$con) if (inherits(df, "tbl_sql") && same_src(df$src, dest)) { out <- compute(df, diff --git a/tests/testthat/_snaps/backend-mssql.new.md b/tests/testthat/_snaps/backend-mssql.new.md new file mode 100644 index 000000000..4d7810cce --- /dev/null +++ b/tests/testthat/_snaps/backend-mssql.new.md @@ -0,0 +1,383 @@ +# custom aggregators translated correctly + + Code + test_translate_sql(quantile(x, 0.5, na.rm = TRUE), window = FALSE) + Condition + Error in `quantile()`: + ! Translation of `quantile()` in `summarise()` is not supported for SQL Server. + i Use a combination of `distinct()` and `mutate()` for the same result: + `mutate( = quantile(x, 0.5, na.rm = TRUE)) %>% distinct()` + Code + test_translate_sql(median(x, na.rm = TRUE), window = FALSE) + Condition + Error in `median()`: + ! Translation of `median()` in `summarise()` is not supported for SQL Server. + i Use a combination of `distinct()` and `mutate()` for the same result: + `mutate( = median(x, na.rm = TRUE)) %>% distinct()` + +# custom lubridate functions translated correctly + + Code + test_translate_sql(month(x, label = TRUE, abbr = TRUE)) + Condition + Error in `month()`: + ! `abbr = TRUE` isn't supported in SQL Server translation. + i It must be FALSE instead. + +# convert between bit and boolean as needed + + Code + mf %>% filter(is.na(x)) + Output + + SELECT df.* + FROM df AS `df` + WHERE ((`x` IS NULL)) + +--- + + Code + mf %>% filter(!is.na(x)) + Output + + SELECT df.* + FROM df AS `df` + WHERE (NOT((`x` IS NULL))) + +--- + + Code + mf %>% filter(x == 1L || x == 2L) + Output + + SELECT df.* + FROM df AS `df` + WHERE (`x` = 1 OR `x` = 2) + +--- + + Code + mf %>% mutate(z = ifelse(x == 1L, 1L, 2L)) + Output + + SELECT df.*, IIF(`x` = 1, 1, 2) AS `z` + FROM df AS `df` + +--- + + Code + mf %>% mutate(z = case_when(x == 1L ~ 1L)) + Output + + SELECT df.*, CASE WHEN (`x` = 1) THEN 1 END AS `z` + FROM df AS `df` + +--- + + Code + mf %>% mutate(z = !is.na(x)) + Output + + SELECT df.*, ~CAST(IIF((`x` IS NULL), 1, 0) AS BIT) AS `z` + FROM df AS `df` + +--- + + Code + mf %>% mutate(x = x == 1L) + Output + + SELECT CAST(IIF(`x` = 1, 1, 0) AS BIT) AS `x` + FROM df AS `df` + +--- + + Code + mf %>% mutate(x = x == 1L || x == 2L) + Output + + SELECT CAST(IIF(`x` = 1 OR `x` = 2, 1, 0) AS BIT) AS `x` + FROM df AS `df` + +--- + + Code + mf %>% mutate(x = x == 1L || x == 2L || x == 3L) + Output + + SELECT CAST(IIF(`x` = 1 OR `x` = 2 OR `x` = 3, 1, 0) AS BIT) AS `x` + FROM df AS `df` + +--- + + Code + mf %>% mutate(x = !(x == 1L || x == 2L || x == 3L)) + Output + + SELECT ~CAST(IIF((`x` = 1 OR `x` = 2 OR `x` = 3), 1, 0) AS BIT) AS `x` + FROM df AS `df` + +# handles ORDER BY in subqueries + + Code + sql_query_select(simulate_mssql(), ident("x"), ident("y"), order_by = "z", + subquery = TRUE) + Condition + Warning: + ORDER BY is ignored in subqueries without LIMIT + i Do you need to move arrange() later in the pipeline or use window_order() instead? + Output + SELECT `x` + FROM `y` + +# custom limit translation + + Code + sql_query_select(simulate_mssql(), ident("x"), ident("y"), order_by = ident("z"), + limit = 10) + Output + SELECT TOP 10 `x` + FROM `y` + ORDER BY `z` + +# custom escapes translated correctly + + Code + mf %>% filter(x == a) + Output + + SELECT df.* + FROM df AS `df` + WHERE (`x` = 0x616263) + +--- + + Code + mf %>% filter(x %in% L) + Output + + SELECT df.* + FROM df AS `df` + WHERE (`x` IN (0x616263, 0x0102)) + +--- + + Code + qry + Output + + SELECT df.* + FROM df AS `df` + WHERE (`x` IN (0x616263, 0x0102)) + +# logical escaping to 0/1 for both filter() and mutate() + + Code + mf %>% filter(x == TRUE) + Output + + SELECT df.* + FROM df AS `df` + WHERE (`x` = 1) + +--- + + Code + mf %>% mutate(x = TRUE) + Output + + SELECT 1 AS `x` + FROM df AS `df` + +# generates custom sql + + Code + sql_table_analyze(con, in_schema("schema", "tbl")) + Output + UPDATE STATISTICS `schema`.`tbl` + +--- + + Code + sql_query_save(con, sql("SELECT * FROM foo"), in_schema("schema", "tbl")) + Output + SELECT * INTO `schema`.`tbl` FROM ( + SELECT * FROM foo + ) AS temp + +--- + + Code + sql_query_save(con, sql("SELECT * FROM foo"), in_schema("schema", "tbl"), + temporary = FALSE) + Output + SELECT * INTO `schema`.`tbl` FROM ( + SELECT * FROM foo + ) AS temp + +--- + + Code + lf %>% slice_sample(n = 1) + Output + + SELECT `x` + FROM ( + SELECT df.*, ROW_NUMBER() OVER (ORDER BY RAND()) AS `col01` + FROM df AS `df` + ) AS `q01` + WHERE (`col01` <= 1) + +--- + + Code + copy_inline(con, tibble(x = 1:2, y = letters[1:2])) %>% remote_query() + Output + SELECT + TRY_CAST(TRY_CAST(`x` AS NUMERIC) AS INT) AS `x`, + TRY_CAST(`y` AS VARCHAR(MAX)) AS `y` + FROM ( VALUES (1, 'a'), (2, 'b')) AS drvd(`x`, `y`) + +--- + + Code + copy_inline(con, trees) %>% remote_query() + Output + SELECT + TRY_CAST(`Girth` AS FLOAT) AS `Girth`, + TRY_CAST(`Height` AS FLOAT) AS `Height`, + TRY_CAST(`Volume` AS FLOAT) AS `Volume` + FROM ( + VALUES + (8.3, 70.0, 10.3), + (8.6, 65.0, 10.3), + (8.8, 63.0, 10.2), + (10.5, 72.0, 16.4), + (10.7, 81.0, 18.8), + (10.8, 83.0, 19.7), + (11.0, 66.0, 15.6), + (11.0, 75.0, 18.2), + (11.1, 80.0, 22.6), + (11.2, 75.0, 19.9), + (11.3, 79.0, 24.2), + (11.4, 76.0, 21.0), + (11.4, 76.0, 21.4), + (11.7, 69.0, 21.3), + (12.0, 75.0, 19.1), + (12.9, 74.0, 22.2), + (12.9, 85.0, 33.8), + (13.3, 86.0, 27.4), + (13.7, 71.0, 25.7), + (13.8, 64.0, 24.9), + (14.0, 78.0, 34.5), + (14.2, 80.0, 31.7), + (14.5, 74.0, 36.3), + (16.0, 72.0, 38.3), + (16.3, 77.0, 42.6), + (17.3, 81.0, 55.4), + (17.5, 82.0, 55.7), + (17.9, 80.0, 58.3), + (18.0, 80.0, 51.5), + (18.0, 80.0, 51.0), + (20.6, 87.0, 77.0) + ) AS drvd(`Girth`, `Height`, `Volume`) + +# `sql_query_append()` is correct + + Code + sql_query_append(con = con, table = ident("df_x"), from = sql_render(df_y, con, + lvl = 1), insert_cols = colnames(df_y), returning_cols = c("a", b2 = "b")) + Output + INSERT INTO `df_x` (`a`, `b`, `c`, `d`) + OUTPUT `INSERTED`.`a`, `INSERTED`.`b` AS `b2` + SELECT * + FROM ( + SELECT `a`, `b`, `c` + 1.0 AS `c`, `d` + FROM df_y AS `df_y` + ) AS `...y` + +# `sql_query_update_from()` is correct + + Code + sql_query_update_from(con = con, table = ident("df_x"), from = sql_render(df_y, + con, lvl = 1), by = c("a", "b"), update_values = sql(c = "COALESCE(`df_x`.`c`, `...y`.`c`)", + d = "`...y`.`d`"), returning_cols = c("a", b2 = "b")) + Output + UPDATE `df_x` + SET `c` = COALESCE(`df_x`.`c`, `...y`.`c`), `d` = `...y`.`d` + OUTPUT `INSERTED`.`a`, `INSERTED`.`b` AS `b2` + FROM `df_x` + INNER JOIN ( + SELECT `a`, `b`, `c` + 1.0 AS `c`, `d` + FROM df_y AS `df_y` + ) AS `...y` + ON `...y`.`a` = `df_x`.`a` AND `...y`.`b` = `df_x`.`b` + +# `sql_query_delete()` is correct + + Code + sql_query_delete(con = simulate_mssql(), table = ident("df_x"), from = sql_render( + df_y, simulate_mssql(), lvl = 2), by = c("a", "b"), returning_cols = c("a", + b2 = "b")) + Output + DELETE FROM `df_x` + OUTPUT `DELETED`.`a`, `DELETED`.`b` AS `b2` + WHERE EXISTS ( + SELECT 1 FROM ( + SELECT `a`, `b`, `c` + 1.0 AS `c`, `d` + FROM df_y AS `df_y` + ) AS `...y` + WHERE (`...y`.`a` = `df_x`.`a`) AND (`...y`.`b` = `df_x`.`b`) + ) + +# `sql_query_upsert()` is correct + + Code + sql_query_upsert(con = con, table = ident("df_x"), from = sql_render(df_y, con, + lvl = 1), by = c("a", "b"), update_cols = c("c", "d"), returning_cols = c("a", + b2 = "b")) + Output + MERGE INTO `df_x` + USING ( + SELECT `a`, `b`, `c` + 1.0 AS `c`, `d` + FROM df_y AS `df_y` + ) AS `...y` + ON `...y`.`a` = `df_x`.`a` AND `...y`.`b` = `df_x`.`b` + WHEN MATCHED THEN + UPDATE SET `c` = `...y`.`c`, `d` = `...y`.`d` + WHEN NOT MATCHED THEN + INSERT (`a`, `b`, `c`, `d`) + VALUES (`...y`.`a`, `...y`.`b`, `...y`.`c`, `...y`.`d`) + OUTPUT `INSERTED`.`a`, `INSERTED`.`b` AS `b2` + ; + +# row_number() with and without group_by() and arrange(): unordered defaults to Ordering by NULL (per empty_order) + + Code + mf %>% mutate(rown = row_number()) + Output + + SELECT df.*, ROW_NUMBER() OVER (ORDER BY (SELECT NULL)) AS `rown` + FROM df AS `df` + +--- + + Code + mf %>% group_by(y) %>% mutate(rown = row_number()) + Output + + SELECT + df.*, + ROW_NUMBER() OVER (PARTITION BY `y` ORDER BY (SELECT NULL)) AS `rown` + FROM df AS `df` + +--- + + Code + mf %>% arrange(y) %>% mutate(rown = row_number()) + Output + + SELECT df.*, ROW_NUMBER() OVER (ORDER BY `y`) AS `rown` + FROM df AS `df` + ORDER BY `y` + diff --git a/tests/testthat/_snaps/db-io.md b/tests/testthat/_snaps/db-io.md index 82af628a4..cfa33ee80 100644 --- a/tests/testthat/_snaps/db-io.md +++ b/tests/testthat/_snaps/db-io.md @@ -34,7 +34,7 @@ Output Error in `db_save_query()`: - ! Can't save query to table tmp. + ! Can't save query to table `tmp`. Caused by error: ! dummy DBI error diff --git a/tests/testthat/_snaps/db-sql.new.md b/tests/testthat/_snaps/db-sql.new.md new file mode 100644 index 000000000..b6d257d2f --- /dev/null +++ b/tests/testthat/_snaps/db-sql.new.md @@ -0,0 +1,5 @@ +# 2nd edition uses sql methods + + Code + expect_error(dbplyr_analyze(con), "db_method") + diff --git a/tests/testthat/test-tbl-sql.R b/tests/testthat/test-tbl-sql.R index 774674081..2d6f17c59 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)") From d03a206b57edf2e890762e5540c0fc527f264e46 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Wed, 8 Nov 2023 07:21:11 -0600 Subject: [PATCH 02/51] Fixing alias escaping --- R/db-sql.R | 9 ++++++--- R/lazy-ops.R | 5 ----- R/table-name.R | 8 ++------ R/tbl-lazy.R | 4 +++- tests/testthat/_snaps/backend-.md | 10 ++++------ 5 files changed, 15 insertions(+), 21 deletions(-) diff --git a/R/db-sql.R b/R/db-sql.R index edf782c0a..bd56db25c 100644 --- a/R/db-sql.R +++ b/R/db-sql.R @@ -174,7 +174,8 @@ sql_table_index.DBIConnection <- function(con, ..., call = caller_env()) { table <- as_table_name(table, con, error_call = call) - name <- name %||% paste0(c(table, columns), collapse = "_") + hash <- substr(hash(table), 1, 6) + name <- name %||% paste0(c("dbplyr", hash, columns), collapse = "_") glue_sql2( con, "CREATE ", if (unique) "UNIQUE ", "INDEX {.name name}", @@ -254,8 +255,10 @@ sql_query_wrap.DBIConnection <- function(con, from, name = NULL, ..., lvl = 0) { name <- name %||% unique_subquery_name() glue_sql2(con, "{from}", as_sql, "{.name name}") } else { # must be a table_name - name <- sql_escape_ident(con, name) - setNames(from, name) + if (!is.null(name)) { + names(from) <- as_table_name(name, con) + } + from } } diff --git a/R/lazy-ops.R b/R/lazy-ops.R index cd503fb0c..865c42d6c 100644 --- a/R/lazy-ops.R +++ b/R/lazy-ops.R @@ -31,11 +31,6 @@ lazy_base_query <- function(x, vars, class = character(), ...) { ) } -lazy_query_local <- function(df, name) { - name <- as_table_name(I(name)) - lazy_base_query(df, names(df), class = "local", name = name) -} - lazy_query_remote <- function(x, vars) { lazy_base_query(x, vars, class = "remote") } diff --git a/R/table-name.R b/R/table-name.R index 492b7e830..a8bae7db4 100644 --- a/R/table-name.R +++ b/R/table-name.R @@ -63,7 +63,7 @@ as_table_name <- function(x, make_table_name(x, con) } else { cli::cli_abort( - "{.arg {error_arg}} uses specification for table name", + "{.arg {error_arg}} uses unknown specification for table name", error_call = error_call ) } @@ -91,8 +91,7 @@ is_table_name <- function(x) { #' @export escape.dbplyr_table_name <- function(x, parens = FALSE, collapse = ", ", con = NULL) { - - alias <- names2(x) + alias <- names2(x) # assume alias is already escaped x <- unname(x) if (db_supports_table_alias_with_as(con)) { @@ -101,9 +100,6 @@ escape.dbplyr_table_name <- function(x, parens = FALSE, collapse = ", ", con = N as_sql <- " " } - # TODO: Why is this getting double escaped? - # alias_esc <- sql_escape_ident(con, alias) - out <- ifelse(alias == "" | alias == x, x, paste0(x, as_sql, alias)) sql_vector(out, parens, collapse, con = con) } diff --git a/R/tbl-lazy.R b/R/tbl-lazy.R index a2f188b8d..da9f717d3 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_name(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/tests/testthat/_snaps/backend-.md b/tests/testthat/_snaps/backend-.md index e01806f34..35b7fcca1 100644 --- a/tests/testthat/_snaps/backend-.md +++ b/tests/testthat/_snaps/backend-.md @@ -68,16 +68,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` --- @@ -91,14 +89,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 `dbplyr_119f50_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 `dbplyr_119f50_c` ON `schema`.`tbl` (`c`) --- From 054ee2b96526d15a44a1d91537edc165192918f0 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Wed, 8 Nov 2023 07:22:16 -0600 Subject: [PATCH 03/51] Fix argument name --- R/db-sql.R | 2 +- tests/testthat/_snaps/backend-mssql.new.md | 383 --------------------- 2 files changed, 1 insertion(+), 384 deletions(-) delete mode 100644 tests/testthat/_snaps/backend-mssql.new.md diff --git a/R/db-sql.R b/R/db-sql.R index bd56db25c..75bb64708 100644 --- a/R/db-sql.R +++ b/R/db-sql.R @@ -769,7 +769,7 @@ sql_query_insert <- function(con, returning_cols = NULL, method = NULL) { check_table_id(table) - check_table_source(sql) + check_table_source(from) check_character(insert_cols) check_character(by) check_character(returning_cols, allow_null = TRUE) diff --git a/tests/testthat/_snaps/backend-mssql.new.md b/tests/testthat/_snaps/backend-mssql.new.md deleted file mode 100644 index 4d7810cce..000000000 --- a/tests/testthat/_snaps/backend-mssql.new.md +++ /dev/null @@ -1,383 +0,0 @@ -# custom aggregators translated correctly - - Code - test_translate_sql(quantile(x, 0.5, na.rm = TRUE), window = FALSE) - Condition - Error in `quantile()`: - ! Translation of `quantile()` in `summarise()` is not supported for SQL Server. - i Use a combination of `distinct()` and `mutate()` for the same result: - `mutate( = quantile(x, 0.5, na.rm = TRUE)) %>% distinct()` - Code - test_translate_sql(median(x, na.rm = TRUE), window = FALSE) - Condition - Error in `median()`: - ! Translation of `median()` in `summarise()` is not supported for SQL Server. - i Use a combination of `distinct()` and `mutate()` for the same result: - `mutate( = median(x, na.rm = TRUE)) %>% distinct()` - -# custom lubridate functions translated correctly - - Code - test_translate_sql(month(x, label = TRUE, abbr = TRUE)) - Condition - Error in `month()`: - ! `abbr = TRUE` isn't supported in SQL Server translation. - i It must be FALSE instead. - -# convert between bit and boolean as needed - - Code - mf %>% filter(is.na(x)) - Output - - SELECT df.* - FROM df AS `df` - WHERE ((`x` IS NULL)) - ---- - - Code - mf %>% filter(!is.na(x)) - Output - - SELECT df.* - FROM df AS `df` - WHERE (NOT((`x` IS NULL))) - ---- - - Code - mf %>% filter(x == 1L || x == 2L) - Output - - SELECT df.* - FROM df AS `df` - WHERE (`x` = 1 OR `x` = 2) - ---- - - Code - mf %>% mutate(z = ifelse(x == 1L, 1L, 2L)) - Output - - SELECT df.*, IIF(`x` = 1, 1, 2) AS `z` - FROM df AS `df` - ---- - - Code - mf %>% mutate(z = case_when(x == 1L ~ 1L)) - Output - - SELECT df.*, CASE WHEN (`x` = 1) THEN 1 END AS `z` - FROM df AS `df` - ---- - - Code - mf %>% mutate(z = !is.na(x)) - Output - - SELECT df.*, ~CAST(IIF((`x` IS NULL), 1, 0) AS BIT) AS `z` - FROM df AS `df` - ---- - - Code - mf %>% mutate(x = x == 1L) - Output - - SELECT CAST(IIF(`x` = 1, 1, 0) AS BIT) AS `x` - FROM df AS `df` - ---- - - Code - mf %>% mutate(x = x == 1L || x == 2L) - Output - - SELECT CAST(IIF(`x` = 1 OR `x` = 2, 1, 0) AS BIT) AS `x` - FROM df AS `df` - ---- - - Code - mf %>% mutate(x = x == 1L || x == 2L || x == 3L) - Output - - SELECT CAST(IIF(`x` = 1 OR `x` = 2 OR `x` = 3, 1, 0) AS BIT) AS `x` - FROM df AS `df` - ---- - - Code - mf %>% mutate(x = !(x == 1L || x == 2L || x == 3L)) - Output - - SELECT ~CAST(IIF((`x` = 1 OR `x` = 2 OR `x` = 3), 1, 0) AS BIT) AS `x` - FROM df AS `df` - -# handles ORDER BY in subqueries - - Code - sql_query_select(simulate_mssql(), ident("x"), ident("y"), order_by = "z", - subquery = TRUE) - Condition - Warning: - ORDER BY is ignored in subqueries without LIMIT - i Do you need to move arrange() later in the pipeline or use window_order() instead? - Output - SELECT `x` - FROM `y` - -# custom limit translation - - Code - sql_query_select(simulate_mssql(), ident("x"), ident("y"), order_by = ident("z"), - limit = 10) - Output - SELECT TOP 10 `x` - FROM `y` - ORDER BY `z` - -# custom escapes translated correctly - - Code - mf %>% filter(x == a) - Output - - SELECT df.* - FROM df AS `df` - WHERE (`x` = 0x616263) - ---- - - Code - mf %>% filter(x %in% L) - Output - - SELECT df.* - FROM df AS `df` - WHERE (`x` IN (0x616263, 0x0102)) - ---- - - Code - qry - Output - - SELECT df.* - FROM df AS `df` - WHERE (`x` IN (0x616263, 0x0102)) - -# logical escaping to 0/1 for both filter() and mutate() - - Code - mf %>% filter(x == TRUE) - Output - - SELECT df.* - FROM df AS `df` - WHERE (`x` = 1) - ---- - - Code - mf %>% mutate(x = TRUE) - Output - - SELECT 1 AS `x` - FROM df AS `df` - -# generates custom sql - - Code - sql_table_analyze(con, in_schema("schema", "tbl")) - Output - UPDATE STATISTICS `schema`.`tbl` - ---- - - Code - sql_query_save(con, sql("SELECT * FROM foo"), in_schema("schema", "tbl")) - Output - SELECT * INTO `schema`.`tbl` FROM ( - SELECT * FROM foo - ) AS temp - ---- - - Code - sql_query_save(con, sql("SELECT * FROM foo"), in_schema("schema", "tbl"), - temporary = FALSE) - Output - SELECT * INTO `schema`.`tbl` FROM ( - SELECT * FROM foo - ) AS temp - ---- - - Code - lf %>% slice_sample(n = 1) - Output - - SELECT `x` - FROM ( - SELECT df.*, ROW_NUMBER() OVER (ORDER BY RAND()) AS `col01` - FROM df AS `df` - ) AS `q01` - WHERE (`col01` <= 1) - ---- - - Code - copy_inline(con, tibble(x = 1:2, y = letters[1:2])) %>% remote_query() - Output - SELECT - TRY_CAST(TRY_CAST(`x` AS NUMERIC) AS INT) AS `x`, - TRY_CAST(`y` AS VARCHAR(MAX)) AS `y` - FROM ( VALUES (1, 'a'), (2, 'b')) AS drvd(`x`, `y`) - ---- - - Code - copy_inline(con, trees) %>% remote_query() - Output - SELECT - TRY_CAST(`Girth` AS FLOAT) AS `Girth`, - TRY_CAST(`Height` AS FLOAT) AS `Height`, - TRY_CAST(`Volume` AS FLOAT) AS `Volume` - FROM ( - VALUES - (8.3, 70.0, 10.3), - (8.6, 65.0, 10.3), - (8.8, 63.0, 10.2), - (10.5, 72.0, 16.4), - (10.7, 81.0, 18.8), - (10.8, 83.0, 19.7), - (11.0, 66.0, 15.6), - (11.0, 75.0, 18.2), - (11.1, 80.0, 22.6), - (11.2, 75.0, 19.9), - (11.3, 79.0, 24.2), - (11.4, 76.0, 21.0), - (11.4, 76.0, 21.4), - (11.7, 69.0, 21.3), - (12.0, 75.0, 19.1), - (12.9, 74.0, 22.2), - (12.9, 85.0, 33.8), - (13.3, 86.0, 27.4), - (13.7, 71.0, 25.7), - (13.8, 64.0, 24.9), - (14.0, 78.0, 34.5), - (14.2, 80.0, 31.7), - (14.5, 74.0, 36.3), - (16.0, 72.0, 38.3), - (16.3, 77.0, 42.6), - (17.3, 81.0, 55.4), - (17.5, 82.0, 55.7), - (17.9, 80.0, 58.3), - (18.0, 80.0, 51.5), - (18.0, 80.0, 51.0), - (20.6, 87.0, 77.0) - ) AS drvd(`Girth`, `Height`, `Volume`) - -# `sql_query_append()` is correct - - Code - sql_query_append(con = con, table = ident("df_x"), from = sql_render(df_y, con, - lvl = 1), insert_cols = colnames(df_y), returning_cols = c("a", b2 = "b")) - Output - INSERT INTO `df_x` (`a`, `b`, `c`, `d`) - OUTPUT `INSERTED`.`a`, `INSERTED`.`b` AS `b2` - SELECT * - FROM ( - SELECT `a`, `b`, `c` + 1.0 AS `c`, `d` - FROM df_y AS `df_y` - ) AS `...y` - -# `sql_query_update_from()` is correct - - Code - sql_query_update_from(con = con, table = ident("df_x"), from = sql_render(df_y, - con, lvl = 1), by = c("a", "b"), update_values = sql(c = "COALESCE(`df_x`.`c`, `...y`.`c`)", - d = "`...y`.`d`"), returning_cols = c("a", b2 = "b")) - Output - UPDATE `df_x` - SET `c` = COALESCE(`df_x`.`c`, `...y`.`c`), `d` = `...y`.`d` - OUTPUT `INSERTED`.`a`, `INSERTED`.`b` AS `b2` - FROM `df_x` - INNER JOIN ( - SELECT `a`, `b`, `c` + 1.0 AS `c`, `d` - FROM df_y AS `df_y` - ) AS `...y` - ON `...y`.`a` = `df_x`.`a` AND `...y`.`b` = `df_x`.`b` - -# `sql_query_delete()` is correct - - Code - sql_query_delete(con = simulate_mssql(), table = ident("df_x"), from = sql_render( - df_y, simulate_mssql(), lvl = 2), by = c("a", "b"), returning_cols = c("a", - b2 = "b")) - Output - DELETE FROM `df_x` - OUTPUT `DELETED`.`a`, `DELETED`.`b` AS `b2` - WHERE EXISTS ( - SELECT 1 FROM ( - SELECT `a`, `b`, `c` + 1.0 AS `c`, `d` - FROM df_y AS `df_y` - ) AS `...y` - WHERE (`...y`.`a` = `df_x`.`a`) AND (`...y`.`b` = `df_x`.`b`) - ) - -# `sql_query_upsert()` is correct - - Code - sql_query_upsert(con = con, table = ident("df_x"), from = sql_render(df_y, con, - lvl = 1), by = c("a", "b"), update_cols = c("c", "d"), returning_cols = c("a", - b2 = "b")) - Output - MERGE INTO `df_x` - USING ( - SELECT `a`, `b`, `c` + 1.0 AS `c`, `d` - FROM df_y AS `df_y` - ) AS `...y` - ON `...y`.`a` = `df_x`.`a` AND `...y`.`b` = `df_x`.`b` - WHEN MATCHED THEN - UPDATE SET `c` = `...y`.`c`, `d` = `...y`.`d` - WHEN NOT MATCHED THEN - INSERT (`a`, `b`, `c`, `d`) - VALUES (`...y`.`a`, `...y`.`b`, `...y`.`c`, `...y`.`d`) - OUTPUT `INSERTED`.`a`, `INSERTED`.`b` AS `b2` - ; - -# row_number() with and without group_by() and arrange(): unordered defaults to Ordering by NULL (per empty_order) - - Code - mf %>% mutate(rown = row_number()) - Output - - SELECT df.*, ROW_NUMBER() OVER (ORDER BY (SELECT NULL)) AS `rown` - FROM df AS `df` - ---- - - Code - mf %>% group_by(y) %>% mutate(rown = row_number()) - Output - - SELECT - df.*, - ROW_NUMBER() OVER (PARTITION BY `y` ORDER BY (SELECT NULL)) AS `rown` - FROM df AS `df` - ---- - - Code - mf %>% arrange(y) %>% mutate(rown = row_number()) - Output - - SELECT df.*, ROW_NUMBER() OVER (ORDER BY `y`) AS `rown` - FROM df AS `df` - ORDER BY `y` - From 892b23e80f4498be3e30510fd746be3d512e76ab Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Wed, 8 Nov 2023 07:24:33 -0600 Subject: [PATCH 04/51] Correct escaping in sql_star() --- R/query-join.R | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/R/query-join.R b/R/query-join.R index 38c82228e..0b7568500 100644 --- a/R/query-join.R +++ b/R/query-join.R @@ -348,7 +348,7 @@ sql_table_prefix <- function(con, var, table = NULL) { sql_star <- function(con, table = NULL) { var <- sql("*") if (!is.null(table)) { - stopifnot(is_table_name(table)) + table <- as_table_name(table, con) sql(paste0(table, ".", var)) } else { var From f7a65bc7d162eff9f2f3dbef3c0118750ed0a5ad Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Wed, 8 Nov 2023 07:53:25 -0600 Subject: [PATCH 05/51] Extract table name from id string --- R/db-sql.R | 4 ++-- R/table-name.R | 17 +++++++++++++++++ R/verb-joins.R | 4 ++-- tests/testthat/_snaps/backend-.md | 4 ++-- 4 files changed, 23 insertions(+), 6 deletions(-) diff --git a/R/db-sql.R b/R/db-sql.R index 75bb64708..b6751044a 100644 --- a/R/db-sql.R +++ b/R/db-sql.R @@ -174,8 +174,8 @@ sql_table_index.DBIConnection <- function(con, ..., call = caller_env()) { table <- as_table_name(table, con, error_call = call) - hash <- substr(hash(table), 1, 6) - name <- name %||% paste0(c("dbplyr", hash, columns), collapse = "_") + table <- db_table_name_extract(con, table) + name <- name %||% paste0(c(table, columns), collapse = "_") glue_sql2( con, "CREATE ", if (unique) "UNIQUE ", "INDEX {.name name}", diff --git a/R/table-name.R b/R/table-name.R index a8bae7db4..434974ca1 100644 --- a/R/table-name.R +++ b/R/table-name.R @@ -89,6 +89,23 @@ is_table_name <- function(x) { inherits(x, "dbplyr_table_name") } +# TODO: make this generic +db_parse_table_name <- function(con, x) { + quote_char <- substr(as_table_name("", con = con), 1, 1) + scan( + text = x, + what = character(), + quote = quote_char, + quiet = TRUE, + na.strings = character(), + sep = "." + ) +} +db_table_name_extract <- function(con, x) { + out <- db_parse_table_name(con, x) + out[[length(out)]] +} + #' @export escape.dbplyr_table_name <- function(x, parens = FALSE, collapse = ", ", con = NULL) { alias <- names2(x) # assume alias is already escaped diff --git a/R/verb-joins.R b/R/verb-joins.R index 20ef52aa0..73d55ddb2 100644 --- a/R/verb-joins.R +++ b/R/verb-joins.R @@ -821,8 +821,8 @@ 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") + paste0(db_table_name_extract(con, names[1]), "_LHS"), + paste0(db_table_name_extract(con, names[2]), "_RHS") ) return(out) } diff --git a/tests/testthat/_snaps/backend-.md b/tests/testthat/_snaps/backend-.md index 35b7fcca1..c9bb64654 100644 --- a/tests/testthat/_snaps/backend-.md +++ b/tests/testthat/_snaps/backend-.md @@ -89,14 +89,14 @@ Code sql_table_index(con, in_schema("schema", "tbl"), c("a", "b")) Output - CREATE INDEX `dbplyr_119f50_a_b` ON `schema`.`tbl` (`a`, `b`) + CREATE INDEX `tbl_a_b` ON `tbl` (`a`, `b`) --- Code sql_table_index(con, in_schema("schema", "tbl"), "c", unique = TRUE) Output - CREATE UNIQUE INDEX `dbplyr_119f50_c` ON `schema`.`tbl` (`c`) + CREATE UNIQUE INDEX `tbl_c` ON `tbl` (`c`) --- From afa58e9f73936157c606d01689452e12394a337c Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Wed, 8 Nov 2023 08:39:26 -0600 Subject: [PATCH 06/51] More getting stuff to work --- R/db-sql.R | 3 ++- R/lazy-join-query.R | 11 ++++++----- R/query-join.R | 3 +++ R/remote.R | 7 ++++++- R/table-name.R | 19 ++++++++++++------- R/verb-joins.R | 5 +++-- 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/test-lazy-select-query.R | 4 +++- tests/testthat/test-query-select.R | 2 +- tests/testthat/test-remote.R | 20 +++++++++----------- 14 files changed, 57 insertions(+), 52 deletions(-) diff --git a/R/db-sql.R b/R/db-sql.R index b6751044a..a5e86b0ab 100644 --- a/R/db-sql.R +++ b/R/db-sql.R @@ -256,7 +256,8 @@ sql_query_wrap.DBIConnection <- function(con, from, name = NULL, ..., lvl = 0) { glue_sql2(con, "{from}", as_sql, "{.name name}") } else { # must be a table_name if (!is.null(name)) { - names(from) <- as_table_name(name, con) + table <- db_table_name_extract(con, name) + names(from) <- as_table_name(table, con) } from } diff --git a/R/lazy-join-query.R b/R/lazy-join-query.R index 97f9de32e..5ff798cd0 100644 --- a/R/lazy-join-query.R +++ b/R/lazy-join-query.R @@ -153,7 +153,7 @@ 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( @@ -171,7 +171,8 @@ sql_build.lazy_multi_join_query <- function(op, con, ..., sql_options = NULL) { op$joins$by <- purrr::map2( op$joins$by, seq_along(op$joins$by), function(by, i) { - by$x_as <- table_names_out[op$joins$by_x_table_id[[i]]] + # FIXME: why is unique now needed? + by$x_as <- table_names_out[unique(op$joins$by_x_table_id[[i]])] by$y_as <- table_names_out[i + 1L] by } @@ -185,7 +186,7 @@ sql_build.lazy_multi_join_query <- function(op, con, ..., sql_options = NULL) { ) } -generate_join_table_names <- function(table_names) { +generate_join_table_names <- function(table_names, con) { table_name_length_max <- max(nchar(table_names$name)) if (length(table_names$name) != 2) { @@ -194,7 +195,7 @@ generate_join_table_names <- function(table_names) { table_names$name[may_repair_name] <- table_names_repaired[may_repair_name] table_names_prepared <- table_names$name } else{ - table_names_prepared <- join_two_table_alias(table_names$name, table_names$from) + table_names_prepared <- join_two_table_alias(table_names$name, table_names$from, con) } # avoid database aliases exceeding the database-specific maximum length @@ -217,7 +218,7 @@ generate_join_table_names <- function(table_names) { #' @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) diff --git a/R/query-join.R b/R/query-join.R index 0b7568500..f0275c975 100644 --- a/R/query-join.R +++ b/R/query-join.R @@ -349,6 +349,9 @@ sql_star <- function(con, table = NULL) { var <- sql("*") if (!is.null(table)) { table <- as_table_name(table, con) + table <- db_table_name_extract(con, table) + table <- as_table_name(table, con) + sql(paste0(table, ".", var)) } else { var diff --git a/R/remote.R b/R/remote.R index 477aff1d6..3fb129e50 100644 --- a/R/remote.R +++ b/R/remote.R @@ -36,7 +36,12 @@ remote_name <- function(x, null_if_local = TRUE) { if (is.sql(table)) { NULL } else { - table + con <- remote_con(x) + if (is.null(con)) { + table + } else { + db_table_name_extract(con, table) + } } } diff --git a/R/table-name.R b/R/table-name.R index 434974ca1..5fd1bbe34 100644 --- a/R/table-name.R +++ b/R/table-name.R @@ -1,3 +1,6 @@ +# table source = table id or sql +# table id = interface to outside world; many ways to specify +# table name = escaped string # 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()) { @@ -45,20 +48,20 @@ as_table_name <- function(x, i = "If you want to use a literal string without escaping using {.fn I} instead" ) ) - table_name(unclass(x)) + new_table_name(unclass(x)) } else if (inherits(x, "ident_q")) { - table_name(paste0(x, collapse = ".")) + new_table_name(paste0(x, collapse = ".")) } else if (is.ident(x)) { make_table_name(unclass(x), con) } else if (is(x, "Id")) { - table_name(DBI::dbQuoteLiteral(con, x)) + make_table_name(x@name, con) } else if (inherits(x, "dbplyr_catalog")) { make_table_name(c(unclass(x$catalog), unclass(x$schema), unclass(x$table)), con) } else if (inherits(x, "dbplyr_schema")) { make_table_name(c(unclass(x$schema), unclass(x$table)), con) } else if (inherits(x, "AsIs")) { check_string(unclass(x), allow_empty = FALSE, arg = error_arg, call = error_call) - table_name(unclass(x)) + new_table_name(unclass(x)) } else if (is.character(x)) { make_table_name(x, con) } else { @@ -73,10 +76,10 @@ make_table_name <- function(x, con) { needs_quote <- !vapply(x, function(x) inherits(x, "AsIs"), logical(1)) x[needs_quote] <- sql_escape_ident(con, x[needs_quote]) - table_name(paste0(x, collapse = ".")) + new_table_name(paste0(x, collapse = ".")) } -table_name <- function(x) { +new_table_name <- function(x) { structure(x, class = "dbplyr_table_name") } @@ -111,12 +114,14 @@ escape.dbplyr_table_name <- function(x, parens = FALSE, collapse = ", ", con = N alias <- names2(x) # assume alias is already escaped x <- unname(x) + table_name <- as_table_name(db_table_name_extract(con, x), con) + if (db_supports_table_alias_with_as(con)) { as_sql <- style_kw(" AS ") } else { as_sql <- " " } - out <- ifelse(alias == "" | alias == x, x, paste0(x, as_sql, alias)) + out <- ifelse(alias == "" | alias == table_name, x, paste0(x, as_sql, alias)) sql_vector(out, parens, collapse, con = con) } diff --git a/R/verb-joins.R b/R/verb-joins.R index 73d55ddb2..0b15c9ddc 100644 --- a/R/verb-joins.R +++ b/R/verb-joins.R @@ -687,7 +687,8 @@ add_semi_join <- function(x, 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) + c(x_alias$from, y_alias$from), + remote_con(x) ) lazy_semi_join_query( @@ -805,7 +806,7 @@ join_vars <- function(x_names, y_names, type, by, suffix = c(".x", ".y"), call = ) } -join_two_table_alias <- function(names, from) { +join_two_table_alias <- function(names, from, con) { check_character(names) check_character(from) vctrs::vec_assert(names, size = 2L) diff --git a/tests/testthat/_snaps/lazy-select-query.md b/tests/testthat/_snaps/lazy-select-query.md index d83918dff..0e6bc1496 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..7a462c335 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..4edc839a8 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..75343445e 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..b2334a570 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/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-select.R b/tests/testthat/test-query-select.R index afd868b6c..0affdcde7 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(new_table_name("`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..9bb0577b0 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")) + new_table_name("`refxiudlph`") ) # produces name after unarranging expect_equal( mf %>% arrange(x) %>% arrange() %>% remote_table(), - as_table_ident(ident("refxiudlph")) + new_table_name("`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), new_table_name("`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), new_table_name("`df`")) + expect_equal(lf %>% group_by(x) %>% remote_table(null_if_local = FALSE), new_table_name("`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_name(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", { From f370e12ea1131ecbd58910a2f03ca0ce681d555c Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Tue, 28 Nov 2023 07:48:32 -0600 Subject: [PATCH 07/51] Restore missing con argument --- R/db-sql.R | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/R/db-sql.R b/R/db-sql.R index a5e86b0ab..ab06a30a6 100644 --- a/R/db-sql.R +++ b/R/db-sql.R @@ -1013,7 +1013,7 @@ sql_query_delete.DBIConnection <- function(con, by, ..., returning_cols = NULL) { - table <- as_table_name(table) + table <- as_table_name(table, con) from <- as_table_source(from, con) parts <- rows_prep(con, table, from, by, lvl = 1) From 6ca54e31a5eea7c35631b1b7d3ba594863b8e643 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Tue, 28 Nov 2023 09:01:01 -0600 Subject: [PATCH 08/51] Making progress on joins --- NAMESPACE | 2 ++ R/lazy-join-query.R | 11 +++++---- R/query-join.R | 10 ++++++--- R/table-name.R | 17 +++++++++++--- R/verb-joins.R | 25 ++++++++++++--------- tests/testthat/test-verb-joins.R | 38 ++++++++++++++++++-------------- 6 files changed, 67 insertions(+), 36 deletions(-) diff --git a/NAMESPACE b/NAMESPACE index 9bba386d3..8d4084cf4 100644 --- a/NAMESPACE +++ b/NAMESPACE @@ -1,6 +1,8 @@ # Generated by roxygen2: do not edit by hand S3method("$",tbl_lazy) +S3method("[",dbplyr_table_name) +S3method("[[",dbplyr_table_name) S3method(add_count,tbl_lazy) S3method(anti_join,tbl_lazy) S3method(arrange,tbl_lazy) diff --git a/R/lazy-join-query.R b/R/lazy-join-query.R index 5ff798cd0..e00500621 100644 --- a/R/lazy-join-query.R +++ b/R/lazy-join-query.R @@ -154,6 +154,7 @@ 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, 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( @@ -172,8 +173,8 @@ sql_build.lazy_multi_join_query <- function(op, con, ..., sql_options = NULL) { op$joins$by, seq_along(op$joins$by), function(by, i) { # FIXME: why is unique now needed? - by$x_as <- table_names_out[unique(op$joins$by_x_table_id[[i]])] - by$y_as <- table_names_out[i + 1L] + by$x_as <- new_table_name(table_names_out[unique(op$joins$by_x_table_id[[i]])]) + by$y_as <- new_table_name(table_names_out[i + 1L]) by } ) @@ -199,7 +200,9 @@ generate_join_table_names <- function(table_names, con) { } # avoid database aliases exceeding the database-specific maximum length - abbreviate( + + # TODO: unescape then escape + new_table_name(abbreviate( table_names_prepared, # 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 @@ -213,7 +216,7 @@ generate_join_table_names <- function(table_names, con) { # don't over anchor to the start of the string, # since we opt to add qualifiers (...1, _{R/L}HS, etc.) to end of table name method = "both.sides" - ) + )) } #' @export diff --git a/R/query-join.R b/R/query-join.R index f0275c975..837270e84 100644 --- a/R/query-join.R +++ b/R/query-join.R @@ -182,7 +182,7 @@ 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_names <- new_table_name(names(table_vars)) # FIXME vectorise `sql_table_prefix()` (need to update `ident()` and friends for this...) out <- rep_named(vars$name, list()) @@ -250,7 +250,7 @@ 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)) + table_names <- new_table_name(c(x_as, y_as)) if (type == "full") { duplicated_vars <- intersect(tolower(vars$all_x), tolower(vars$all_y)) @@ -331,6 +331,7 @@ sql_join_tbls <- function(con, by, na_matches) { } sql_table_prefix <- function(con, var, table = NULL) { + if (!is_bare_character(var)) { cli_abort("{.arg var} must be a bare character.", .internal = TRUE) } @@ -338,7 +339,9 @@ sql_table_prefix <- function(con, var, table = NULL) { if (!is.null(table)) { table <- as_table_name(table, con) - table <- escape(table, collapse = NULL, con = con) + table <- db_table_name_extract(con, table) + table <- as_table_name(table, con) + sql(paste0(table, ".", var)) } else { var @@ -346,6 +349,7 @@ sql_table_prefix <- function(con, var, table = NULL) { } sql_star <- function(con, table = NULL) { + # TODO: sql_table_prefix(con, "*", table) ? var <- sql("*") if (!is.null(table)) { table <- as_table_name(table, con) diff --git a/R/table-name.R b/R/table-name.R index 5fd1bbe34..e538e3329 100644 --- a/R/table-name.R +++ b/R/table-name.R @@ -85,7 +85,16 @@ new_table_name <- function(x) { #' @export print.dbplyr_table_name <- function(x) { - cat(" ", style_kw(x), "\n", sep = "") + cat(" ", paste0(style_kw(x), collapse = ", "), "\n", sep = "") +} + +#' @export +`[.dbplyr_table_name` <- function(x, ...) { + new_table_name(NextMethod()) +} +#' @export +`[[.dbplyr_table_name` <- function(x, ...) { + new_table_name(NextMethod()) } is_table_name <- function(x) { @@ -105,8 +114,10 @@ db_parse_table_name <- function(con, x) { ) } db_table_name_extract <- function(con, x) { - out <- db_parse_table_name(con, x) - out[[length(out)]] + vapply(x, FUN.VALUE = character(1), function(x) { + out <- db_parse_table_name(con, x) + out[[length(out)]] + }) } #' @export diff --git a/R/verb-joins.R b/R/verb-joins.R index 0b15c9ddc..15c8b2f45 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,15 +681,17 @@ 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), + aliases <- join_two_table_alias( + new_table_name(c(x_alias$name, y_alias$name)), c(x_alias$from, y_alias$from), remote_con(x) ) + by$x_as <- aliases[1] + by$y_as <- aliases[2] lazy_semi_join_query( x_lq, @@ -738,7 +740,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) @@ -746,18 +748,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_name(x_as, con), + y = if (!is.null(y_as)) as_table_name(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 = new_table_name(""), from = "") } } @@ -822,8 +827,8 @@ join_two_table_alias <- function(names, from, con) { tables_have_same_name <- from[1] == "name" && from[2] == "name" && identical(names[1], names[2]) if (tables_have_same_name) { out <- c( - paste0(db_table_name_extract(con, names[1]), "_LHS"), - paste0(db_table_name_extract(con, names[2]), "_RHS") + as_table_name(paste0(db_table_name_extract(con, names[1]), "_LHS"), con), + as_table_name(paste0(db_table_name_extract(con, names[2]), "_RHS"), con) ) return(out) } diff --git a/tests/testthat/test-verb-joins.R b/tests/testthat/test-verb-joins.R index 7bd93e7c9..c33a7958a 100644 --- a/tests/testthat/test-verb-joins.R +++ b/tests/testthat/test-verb-joins.R @@ -289,56 +289,56 @@ test_that("join uses correct table alias", { # self joins table_names <- sql_build(left_join(x, x, by = "a"))$table_names - expect_equal(table_names, c("x_LHS", "x_RHS")) + expect_equal(table_names, new_table_name(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")) + expect_equal(table_names, new_table_name(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")) + expect_equal(table_names, new_table_name(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")) + expect_equal(table_names, new_table_name(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")) + expect_equal(table_names, new_table_name(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")) + expect_equal(table_names, new_table_name(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")) + expect_equal(table_names, new_table_name(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")) + expect_equal(table_names, new_table_name(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")) + expect_equal(table_names, new_table_name(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")) + expect_equal(table_names, new_table_name(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")) + expect_equal(table_names, new_table_name(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")) + expect_equal(table_names, new_table_name(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, new_table_name(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, new_table_name(c("`x...1`", "`x...2`", "`z`"))) }) test_that("select() before join is inlined", { @@ -623,7 +623,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 = new_table_name(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 +725,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 = new_table_name(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") %>% From 3365fe8a43efc751ad69653eb99415f80b28d40c Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Tue, 28 Nov 2023 13:56:41 -0600 Subject: [PATCH 09/51] Add c method --- NAMESPACE | 1 + R/table-name.R | 6 ++++++ 2 files changed, 7 insertions(+) diff --git a/NAMESPACE b/NAMESPACE index 8d4084cf4..44b88d1e0 100644 --- a/NAMESPACE +++ b/NAMESPACE @@ -23,6 +23,7 @@ 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_name) S3method(c,ident) S3method(c,sql) S3method(collapse,tbl_sql) diff --git a/R/table-name.R b/R/table-name.R index e538e3329..bc7739349 100644 --- a/R/table-name.R +++ b/R/table-name.R @@ -97,6 +97,12 @@ print.dbplyr_table_name <- function(x) { new_table_name(NextMethod()) } +#' @export +`c.dbplyr_table_name` <- function(x, ...) { + new_table_name(NextMethod()) +} + + is_table_name <- function(x) { inherits(x, "dbplyr_table_name") } From c520833e7f76db32659255d3dbe5ed9616b4ebeb Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Tue, 28 Nov 2023 13:57:09 -0600 Subject: [PATCH 10/51] x_as and y_as are now already escaped --- R/lazy-join-query.R | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/R/lazy-join-query.R b/R/lazy-join-query.R index e00500621..6b9966df9 100644 --- a/R/lazy-join-query.R +++ b/R/lazy-join-query.R @@ -264,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) From 9c5ac01f1a7256e62ac7cb481ab1778743d34bb7 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Tue, 28 Nov 2023 14:00:47 -0600 Subject: [PATCH 11/51] Escape LHS & RHS names --- R/table-name.R | 2 +- R/verb-joins.R | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/R/table-name.R b/R/table-name.R index bc7739349..7feba7538 100644 --- a/R/table-name.R +++ b/R/table-name.R @@ -76,7 +76,7 @@ make_table_name <- function(x, con) { needs_quote <- !vapply(x, function(x) inherits(x, "AsIs"), logical(1)) x[needs_quote] <- sql_escape_ident(con, x[needs_quote]) - new_table_name(paste0(x, collapse = ".")) + new_table_name(x) } new_table_name <- function(x) { diff --git a/R/verb-joins.R b/R/verb-joins.R index 15c8b2f45..1c0214c08 100644 --- a/R/verb-joins.R +++ b/R/verb-joins.R @@ -817,7 +817,7 @@ join_two_table_alias <- function(names, from, con) { vctrs::vec_assert(names, size = 2L) out <- names - out[from == ""] <- c("LHS", "RHS")[from == ""] + out[from == ""] <- as_table_name(c("LHS", "RHS"), con)[from == ""] if (!identical(out[1], out[2])) { return(out) From ffd27e8d22b058f499fd808fc075004641bbd302 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Tue, 28 Nov 2023 14:20:10 -0600 Subject: [PATCH 12/51] Generate aliases based on table names --- R/lazy-join-query.R | 20 ++++++++++---------- R/table-name.R | 2 ++ R/verb-joins.R | 26 +++++++++++--------------- 3 files changed, 23 insertions(+), 25 deletions(-) diff --git a/R/lazy-join-query.R b/R/lazy-join-query.R index 6b9966df9..cfc37f110 100644 --- a/R/lazy-join-query.R +++ b/R/lazy-join-query.R @@ -188,22 +188,20 @@ sql_build.lazy_multi_join_query <- function(op, con, ..., sql_options = NULL) { } generate_join_table_names <- function(table_names, con) { - table_name_length_max <- max(nchar(table_names$name)) + names <- db_table_name_extract(con, table_names$name) + 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, con) + names <- join_two_table_alias(names, table_names$from) } # avoid database aliases exceeding the database-specific maximum length - - # TODO: unescape then escape - new_table_name(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." @@ -216,7 +214,9 @@ generate_join_table_names <- function(table_names, con) { # don't over anchor to the start of the string, # since we opt to add qualifiers (...1, _{R/L}HS, etc.) to end of table name method = "both.sides" - )) + ) + + as_table_name(abbr_names, con) } #' @export diff --git a/R/table-name.R b/R/table-name.R index 7feba7538..fa95cc43d 100644 --- a/R/table-name.R +++ b/R/table-name.R @@ -121,6 +121,8 @@ db_parse_table_name <- function(con, x) { } db_table_name_extract <- function(con, x) { vapply(x, FUN.VALUE = character(1), function(x) { + if (x == "") return("") + out <- db_parse_table_name(con, x) out[[length(out)]] }) diff --git a/R/verb-joins.R b/R/verb-joins.R index 1c0214c08..b0e3a269f 100644 --- a/R/verb-joins.R +++ b/R/verb-joins.R @@ -683,15 +683,13 @@ add_semi_join <- function(x, # the table alias can only be determined after `select()` was inlined 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) - aliases <- join_two_table_alias( - new_table_name(c(x_alias$name, y_alias$name)), - c(x_alias$from, y_alias$from), - remote_con(x) + aliases <- vctrs::vec_rbind( + make_table_names(join_alias$x, x_lq), + make_table_names(join_alias$y, y_lq) ) - by$x_as <- aliases[1] - by$y_as <- aliases[2] + 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, @@ -811,13 +809,15 @@ join_vars <- function(x_names, y_names, type, by, suffix = c(".x", ".y"), call = ) } -join_two_table_alias <- function(names, from, con) { +# 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) vctrs::vec_assert(names, size = 2L) out <- names - out[from == ""] <- as_table_name(c("LHS", "RHS"), con)[from == ""] + out[from == ""] <- c("LHS", "RHS")[from == ""] if (!identical(out[1], out[2])) { return(out) @@ -826,11 +826,7 @@ join_two_table_alias <- function(names, from, con) { tables_have_same_name <- from[1] == "name" && from[2] == "name" && identical(names[1], names[2]) if (tables_have_same_name) { - out <- c( - as_table_name(paste0(db_table_name_extract(con, names[1]), "_LHS"), con), - as_table_name(paste0(db_table_name_extract(con, names[2]), "_RHS"), con) - ) - return(out) + return(paste0(names, c("_LHS", "_RHS"))) } out_repaired <- vctrs::vec_as_names(out, repair = "unique", quiet = TRUE) From 381a7edb0984d8df48c8f1efe894716452a80120 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Tue, 28 Nov 2023 14:30:38 -0600 Subject: [PATCH 13/51] Misc fixes --- R/remote.R | 2 +- R/table-name.R | 7 +++++-- tests/testthat/test-tbl-lazy.R | 2 +- tests/testthat/test-tbl-sql.R | 2 +- 4 files changed, 8 insertions(+), 5 deletions(-) diff --git a/R/remote.R b/R/remote.R index 3fb129e50..ef176c9f9 100644 --- a/R/remote.R +++ b/R/remote.R @@ -33,7 +33,7 @@ remote_name <- function(x, null_if_local = TRUE) { table <- remote_table(x, null_if_local = null_if_local) - if (is.sql(table)) { + if (is.sql(table) || is.null(table)) { NULL } else { con <- remote_con(x) diff --git a/R/table-name.R b/R/table-name.R index fa95cc43d..fc2fe1a2b 100644 --- a/R/table-name.R +++ b/R/table-name.R @@ -63,7 +63,7 @@ as_table_name <- function(x, check_string(unclass(x), allow_empty = FALSE, arg = error_arg, call = error_call) new_table_name(unclass(x)) } else if (is.character(x)) { - make_table_name(x, con) + make_table_name(x, con, collapse = FALSE) } else { cli::cli_abort( "{.arg {error_arg}} uses unknown specification for table name", @@ -72,9 +72,12 @@ as_table_name <- function(x, } } -make_table_name <- function(x, con) { +make_table_name <- function(x, con, collapse = TRUE) { needs_quote <- !vapply(x, function(x) inherits(x, "AsIs"), logical(1)) x[needs_quote] <- sql_escape_ident(con, x[needs_quote]) + if (collapse) { + x <- paste0(x, collapse = ".") + } new_table_name(x) } diff --git a/tests/testthat/test-tbl-lazy.R b/tests/testthat/test-tbl-lazy.R index f239d063b..ebc8d2738 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(new_table_name("`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 2d6f17c59..5177faaff 100644 --- a/tests/testthat/test-tbl-sql.R +++ b/tests/testthat/test-tbl-sql.R @@ -73,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)) From d8b66f350445389605c7e4fb426b4048b1558251 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Tue, 28 Nov 2023 16:29:01 -0600 Subject: [PATCH 14/51] Ensure unique_query_name() generates esacped name --- R/backend-teradata.R | 2 +- R/db-sql.R | 4 +-- R/lazy-ops.R | 2 +- R/lazy-select-query.R | 2 +- R/lazy-set-op-query.R | 1 + R/query-join.R | 8 +++--- R/query-select.R | 6 ++--- R/query-set-op.R | 8 +++--- R/sql-build.R | 17 ++++++------ R/utils.R | 4 +-- R/verb-copy-to.R | 4 +-- tests/testthat/_snaps/verb-joins.md | 8 +++--- tests/testthat/test-verb-compute.R | 3 ++- tests/testthat/test-verb-joins.R | 42 +++++++++++++++-------------- 14 files changed, 58 insertions(+), 53 deletions(-) diff --git a/R/backend-teradata.R b/R/backend-teradata.R index fe2007ee4..425a2348b 100644 --- a/R/backend-teradata.R +++ b/R/backend-teradata.R @@ -65,7 +65,7 @@ sql_query_select.Teradata <- function(con, lvl = lvl + 1 ) - alias <- unique_subquery_name() + alias <- unique_subquery_name(con) from <- sql_query_wrap(con, unlimited_query, name = alias) select_outer <- sql_star(con, alias) out <- sql_select_clauses(con, diff --git a/R/db-sql.R b/R/db-sql.R index ab06a30a6..d99a43f4b 100644 --- a/R/db-sql.R +++ b/R/db-sql.R @@ -252,7 +252,7 @@ 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() + name <- name %||% unique_subquery_name(con) glue_sql2(con, "{from}", as_sql, "{.name name}") } else { # must be a table_name if (!is.null(name)) { @@ -1169,7 +1169,7 @@ dbplyr_sql_subquery <- function(con, ...) { #' @importFrom dplyr sql_subquery sql_subquery.DBIConnection <- function(con, from, - name = unique_subquery_name(), + name = unique_subquery_name(con), ..., lvl = 0) { sql_query_wrap(con, from = from, name = name, ..., lvl = lvl) diff --git a/R/lazy-ops.R b/R/lazy-ops.R index 865c42d6c..786383b1c 100644 --- a/R/lazy-ops.R +++ b/R/lazy-ops.R @@ -89,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/lazy-select-query.R b/R/lazy-select-query.R index 84e81c9f0..d07f36da2 100644 --- a/R/lazy-select-query.R +++ b/R/lazy-select-query.R @@ -161,7 +161,7 @@ sql_build.lazy_select_query <- function(op, con, ..., sql_options = NULL) { inform(op$message_summarise) } - alias <- remote_name(op$x, null_if_local = FALSE) %||% unique_subquery_name() + alias <- remote_name(op$x, null_if_local = FALSE) %||% unique_subquery_name(con) from <- sql_build(op$x, con, sql_options = sql_options) select_sql_list <- get_select_sql( select = op$select, diff --git a/R/lazy-set-op-query.R b/R/lazy-set-op-query.R index 72323a803..97ba6b1dd 100644 --- a/R/lazy-set-op-query.R +++ b/R/lazy-set-op-query.R @@ -79,6 +79,7 @@ op_vars.lazy_union_query <- function(op) { #' @export sql_build.lazy_union_query <- function(op, con, ..., sql_options = NULL) { # add_union() ensures that both have same variables + unions <- list( table = purrr::map( op$unions$table, diff --git a/R/query-join.R b/R/query-join.R index 837270e84..5767f0a36 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 <- 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/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..1f2566717 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 <- 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/sql-build.R b/R/sql-build.R index eb74f78f7..fea30a15e 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 <- new_table_name(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 <- 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/utils.R b/R/utils.R index 8fc7856cf..4872dd5f3 100644 --- a/R/utils.R +++ b/R/utils.R @@ -26,11 +26,11 @@ unique_table_name <- function() { options(dbplyr_table_name = i) sprintf("dbplyr_%03i", i) } -unique_subquery_name <- function() { +unique_subquery_name <- function(con) { # Needs to use option so can reset at the start of each query i <- getOption("dbplyr_subquery_name", 0) + 1 options(dbplyr_subquery_name = i) - sprintf("q%02i", i) + as_table_name(sprintf("q%02i", i), con) } unique_column_name <- function() { # Needs to use option so can reset at the start of each query diff --git a/R/verb-copy-to.R b/R/verb-copy-to.R index 09f714898..02072bc64 100644 --- a/R/verb-copy-to.R +++ b/R/verb-copy-to.R @@ -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/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-verb-compute.R b/tests/testthat/test-verb-compute.R index d6b96b3b5..363cd5766 100644 --- a/tests/testthat/test-verb-compute.R +++ b/tests/testthat/test-verb-compute.R @@ -83,7 +83,8 @@ 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() + name <- set_names(unique_subquery_name(con), unique_subquery_name(con)) expect_equal( memdb_frame(x = 1:10) %>% compute() %>% diff --git a/tests/testthat/test-verb-joins.R b/tests/testthat/test-verb-joins.R index c33a7958a..6c258f162 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 = new_table_name(c("foo.df", "foo2.df")), + from = c("name", "name") + )) }) test_that("alias truncates long table names at database limit", { @@ -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)) @@ -124,11 +125,11 @@ test_that("alias truncates long table names at database limit", { # joins correctly work self_join2 <- left_join(mf1, mf1, by = c("x", "y")) - - expect_equal( - self_join2 %>% collect(), - tibble(x = 1:3, y = "a") - ) +# +# expect_equal( +# self_join2 %>% collect(), +# tibble(x = 1:3, y = "a") +# ) expect_snapshot( self_join2 %>% remote_query() @@ -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)) @@ -153,10 +155,10 @@ test_that("alias truncates long table names at database limit", { self_join3 <- left_join(mf1, mf1, by = c("x", "y")) %>% inner_join(mf2, by = "x") - expect_equal( - self_join3 %>% collect(), - tibble(x = 2:3, y.x = "a", y.y = "b") - ) + # expect_equal( + # self_join3 %>% collect(), + # tibble(x = 2:3, y.x = "a", y.y = "b") + # ) expect_snapshot( self_join3 %>% remote_query() ) From 2b2c3d276219ca091a25d03d51f1e36522dc08c4 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Tue, 28 Nov 2023 16:29:47 -0600 Subject: [PATCH 15/51] Escaping now happens earlier --- tests/testthat/test-verb-set-ops.R | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/testthat/test-verb-set-ops.R b/tests/testthat/test-verb-set-ops.R index 8fbd20e50..17909a199 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") From 8ec767041124ac80aa1e0aa0d9cc38dfaeaef9d3 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Tue, 28 Nov 2023 16:41:14 -0600 Subject: [PATCH 16/51] Update postgres setup information --- vignettes/setup/_postgres.Rmd | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) 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" +) ``` From 68c799c4e04bf16f7ca428afba06397c4429ea6b Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Tue, 28 Nov 2023 16:42:33 -0600 Subject: [PATCH 17/51] Remove old ident code --- DESCRIPTION | 1 - NAMESPACE | 10 -- R/table-ident.R | 254 --------------------------- tests/testthat/_snaps/table-ident.md | 109 ------------ tests/testthat/test-table-ident.R | 144 --------------- 5 files changed, 518 deletions(-) delete mode 100644 R/table-ident.R delete mode 100644 tests/testthat/_snaps/table-ident.md delete mode 100644 tests/testthat/test-table-ident.R diff --git a/DESCRIPTION b/DESCRIPTION index ca586111a..b9fc3683c 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -126,7 +126,6 @@ Collate: 'sql-expr.R' 'src-sql.R' 'src_dbi.R' - 'table-ident.R' 'table-name.R' 'tbl-lazy.R' 'tbl-sql.R' diff --git a/NAMESPACE b/NAMESPACE index 44b88d1e0..5f21cf0a8 100644 --- a/NAMESPACE +++ b/NAMESPACE @@ -14,14 +14,6 @@ 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_name) S3method(c,ident) @@ -122,7 +114,6 @@ S3method(escape,character) S3method(escape,data.frame) S3method(escape,dbplyr_catalog) S3method(escape,dbplyr_schema) -S3method(escape,dbplyr_table_ident) S3method(escape,dbplyr_table_name) S3method(escape,double) S3method(escape,factor) @@ -145,7 +136,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) diff --git a/R/table-ident.R b/R/table-ident.R deleted file mode 100644 index 00efb6ce8..000000000 --- a/R/table-ident.R +++ /dev/null @@ -1,254 +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) -} - -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/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/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`" - ) - ) -}) From 6889d45145ad5321a68d02b38620a73631dc7a1c Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Tue, 28 Nov 2023 17:12:55 -0600 Subject: [PATCH 18/51] Start testing table_name functions --- R/table-name.R | 6 +++-- tests/testthat/_snaps/table-name.md | 24 ++++++++++++++++++ tests/testthat/test-table-name.R | 39 +++++++++++++++++++++++++++++ 3 files changed, 67 insertions(+), 2 deletions(-) create mode 100644 tests/testthat/_snaps/table-name.md create mode 100644 tests/testthat/test-table-name.R diff --git a/R/table-name.R b/R/table-name.R index fc2fe1a2b..3c2984d57 100644 --- a/R/table-name.R +++ b/R/table-name.R @@ -39,13 +39,15 @@ as_table_name <- function(x, con, error_arg = caller_arg(x), error_call = caller_env()) { + check_con(con) + if (is_table_name(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 string without escaping using {.fn I} instead" + "{.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." ) ) new_table_name(unclass(x)) diff --git a/tests/testthat/_snaps/table-name.md b/tests/testthat/_snaps/table-name.md new file mode 100644 index 000000000..d0c2820db --- /dev/null +++ b/tests/testthat/_snaps/table-name.md @@ -0,0 +1,24 @@ +# as_table_name validates its inputs + + Code + as_table_name("x") + Condition + Error in `as_table_name()`: + ! argument "con" is missing, with no default + Code + as_table_name(1, con) + Condition + Error in `as_table_name()`: + ! `1` uses unknown specification for table name + +# as_table_name warns when using sql + + Code + as_table_name(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/test-table-name.R b/tests/testthat/test-table-name.R new file mode 100644 index 000000000..220384cd6 --- /dev/null +++ b/tests/testthat/test-table-name.R @@ -0,0 +1,39 @@ + +# as_table_name ----------------------------------------------------------- + +test_that("can coerce all user facing inputs", { + con <- simulate_dbi() + + expect_equal(new_table_name("`x`"), new_table_name("`x`")) + expect_equal(as_table_name("x", con), new_table_name("`x`")) + expect_equal(as_table_name(I("x"), con), new_table_name("x")) + expect_equal(as_table_name(ident("x"), con), new_table_name("`x`")) + expect_equal(as_table_name(ident_q("x"), con), new_table_name("x")) + + id <- DBI::Id(schema = "foo", table = "bar") + expect_equal(as_table_name(id, con), new_table_name("`foo`.`bar`")) + names(id@name) <- NULL + expect_equal(as_table_name(id, con), new_table_name("`foo`.`bar`")) + + expect_equal( + as_table_name(in_schema("foo", "bar"), con), + new_table_name("`foo`.`bar`") + ) + expect_equal( + as_table_name(in_catalog("foo", "bar", "baz"), con), + new_table_name("`foo`.`bar`.`baz`") + ) +}) + +test_that("as_table_name validates its inputs", { + con <- simulate_dbi() + expect_snapshot(error = TRUE, { + as_table_name("x") + as_table_name(1, con) + }) +}) + +test_that("as_table_name warns when using sql", { + con <- simulate_dbi() + expect_snapshot(as_table_name(sql("x"), con)) +}) From 90bc9a1cd222ae1fea5995809295e884007aef9b Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Tue, 28 Nov 2023 17:14:49 -0600 Subject: [PATCH 19/51] Rename constructor --- R/lazy-join-query.R | 4 ++-- R/query-join.R | 4 ++-- R/sql-build.R | 2 +- R/table-name.R | 16 +++++++------- R/verb-joins.R | 2 +- tests/testthat/test-query-select.R | 2 +- tests/testthat/test-remote.R | 10 ++++----- tests/testthat/test-table-name.R | 18 ++++++++-------- tests/testthat/test-tbl-lazy.R | 2 +- tests/testthat/test-verb-joins.R | 34 +++++++++++++++--------------- 10 files changed, 47 insertions(+), 47 deletions(-) diff --git a/R/lazy-join-query.R b/R/lazy-join-query.R index cfc37f110..1ae9bfa0a 100644 --- a/R/lazy-join-query.R +++ b/R/lazy-join-query.R @@ -173,8 +173,8 @@ sql_build.lazy_multi_join_query <- function(op, con, ..., sql_options = NULL) { op$joins$by, seq_along(op$joins$by), function(by, i) { # FIXME: why is unique now needed? - by$x_as <- new_table_name(table_names_out[unique(op$joins$by_x_table_id[[i]])]) - by$y_as <- new_table_name(table_names_out[i + 1L]) + by$x_as <- table_name(table_names_out[unique(op$joins$by_x_table_id[[i]])]) + by$y_as <- table_name(table_names_out[i + 1L]) by } ) diff --git a/R/query-join.R b/R/query-join.R index 5767f0a36..9b44223f5 100644 --- a/R/query-join.R +++ b/R/query-join.R @@ -182,7 +182,7 @@ 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 <- new_table_name(names(table_vars)) + table_names <- table_name(names(table_vars)) # FIXME vectorise `sql_table_prefix()` (need to update `ident()` and friends for this...) out <- rep_named(vars$name, list()) @@ -250,7 +250,7 @@ sql_rf_join_vars <- function(con, use_star, qualify_all_columns) { type <- arg_match0(type, c("right", "full")) - table_names <- new_table_name(c(x_as, y_as)) + table_names <- table_name(c(x_as, y_as)) if (type == "full") { duplicated_vars <- intersect(tolower(vars$all_x), tolower(vars$all_y)) diff --git a/R/sql-build.R b/R/sql-build.R index fea30a15e..ccc662475 100644 --- a/R/sql-build.R +++ b/R/sql-build.R @@ -144,7 +144,7 @@ cte_render <- function(query_list, con) { ctes <- purrr::imap( query_list[-n], function(query, name) { - name <- new_table_name(name) + name <- table_name(name) glue_sql2(con, "{.name name} {.kw 'AS'} (\n{query}\n)") } ) diff --git a/R/table-name.R b/R/table-name.R index 3c2984d57..352095499 100644 --- a/R/table-name.R +++ b/R/table-name.R @@ -50,9 +50,9 @@ as_table_name <- function(x, i = "If you want to use a literal (unquoted) identifier use {.fn I} instead." ) ) - new_table_name(unclass(x)) + table_name(unclass(x)) } else if (inherits(x, "ident_q")) { - new_table_name(paste0(x, collapse = ".")) + table_name(paste0(x, collapse = ".")) } else if (is.ident(x)) { make_table_name(unclass(x), con) } else if (is(x, "Id")) { @@ -63,7 +63,7 @@ as_table_name <- function(x, make_table_name(c(unclass(x$schema), unclass(x$table)), con) } else if (inherits(x, "AsIs")) { check_string(unclass(x), allow_empty = FALSE, arg = error_arg, call = error_call) - new_table_name(unclass(x)) + table_name(unclass(x)) } else if (is.character(x)) { make_table_name(x, con, collapse = FALSE) } else { @@ -81,10 +81,10 @@ make_table_name <- function(x, con, collapse = TRUE) { x <- paste0(x, collapse = ".") } - new_table_name(x) + table_name(x) } -new_table_name <- function(x) { +table_name <- function(x) { structure(x, class = "dbplyr_table_name") } @@ -95,16 +95,16 @@ print.dbplyr_table_name <- function(x) { #' @export `[.dbplyr_table_name` <- function(x, ...) { - new_table_name(NextMethod()) + table_name(NextMethod()) } #' @export `[[.dbplyr_table_name` <- function(x, ...) { - new_table_name(NextMethod()) + table_name(NextMethod()) } #' @export `c.dbplyr_table_name` <- function(x, ...) { - new_table_name(NextMethod()) + table_name(NextMethod()) } diff --git a/R/verb-joins.R b/R/verb-joins.R index b0e3a269f..1e4522c42 100644 --- a/R/verb-joins.R +++ b/R/verb-joins.R @@ -760,7 +760,7 @@ make_table_names <- function(as, lq) { } else if (!is.null(name)) { tibble(name = name, from = "name") } else { - tibble(name = new_table_name(""), from = "") + tibble(name = table_name(""), from = "") } } diff --git a/tests/testthat/test-query-select.R b/tests/testthat/test-query-select.R index 0affdcde7..4bfc20e69 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(new_table_name("`df`"))) + expect_equal(qry$from, base_query(table_name("`df`"))) }) test_that("group by then limit is collapsed", { diff --git a/tests/testthat/test-remote.R b/tests/testthat/test-remote.R index 9bb0577b0..03a94b409 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(), - new_table_name("`refxiudlph`") + table_name("`refxiudlph`") ) # produces name after unarranging expect_equal( mf %>% arrange(x) %>% arrange() %>% remote_table(), - new_table_name("`refxiudlph`") + table_name("`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), new_table_name("`refxiudlph`")) + expect_equal(remote_table(mf), table_name("`refxiudlph`")) expect_null(mf %>% filter(x == 3) %>% remote_table()) expect_null(mf %>% distinct(x) %>% remote_table()) @@ -34,8 +34,8 @@ 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), new_table_name("`df`")) - expect_equal(lf %>% group_by(x) %>% remote_table(null_if_local = FALSE), new_table_name("`df`")) + expect_equal(lf %>% remote_table(null_if_local = FALSE), table_name("`df`")) + expect_equal(lf %>% group_by(x) %>% remote_table(null_if_local = FALSE), table_name("`df`")) }) test_that("remote_name and remote_table can handle different table identifiers", { diff --git a/tests/testthat/test-table-name.R b/tests/testthat/test-table-name.R index 220384cd6..8b08dfa54 100644 --- a/tests/testthat/test-table-name.R +++ b/tests/testthat/test-table-name.R @@ -4,24 +4,24 @@ test_that("can coerce all user facing inputs", { con <- simulate_dbi() - expect_equal(new_table_name("`x`"), new_table_name("`x`")) - expect_equal(as_table_name("x", con), new_table_name("`x`")) - expect_equal(as_table_name(I("x"), con), new_table_name("x")) - expect_equal(as_table_name(ident("x"), con), new_table_name("`x`")) - expect_equal(as_table_name(ident_q("x"), con), new_table_name("x")) + expect_equal(table_name("`x`"), table_name("`x`")) + expect_equal(as_table_name("x", con), table_name("`x`")) + expect_equal(as_table_name(I("x"), con), table_name("x")) + expect_equal(as_table_name(ident("x"), con), table_name("`x`")) + expect_equal(as_table_name(ident_q("x"), con), table_name("x")) id <- DBI::Id(schema = "foo", table = "bar") - expect_equal(as_table_name(id, con), new_table_name("`foo`.`bar`")) + expect_equal(as_table_name(id, con), table_name("`foo`.`bar`")) names(id@name) <- NULL - expect_equal(as_table_name(id, con), new_table_name("`foo`.`bar`")) + expect_equal(as_table_name(id, con), table_name("`foo`.`bar`")) expect_equal( as_table_name(in_schema("foo", "bar"), con), - new_table_name("`foo`.`bar`") + table_name("`foo`.`bar`") ) expect_equal( as_table_name(in_catalog("foo", "bar", "baz"), con), - new_table_name("`foo`.`bar`.`baz`") + table_name("`foo`.`bar`.`baz`") ) }) diff --git a/tests/testthat/test-tbl-lazy.R b/tests/testthat/test-tbl-lazy.R index ebc8d2738..eafa6d3af 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(new_table_name("`df`"))) + expect_equal(out, base_query(table_name("`df`"))) }) test_that("names() inform that they aren't meant to be used", { diff --git a/tests/testthat/test-verb-joins.R b/tests/testthat/test-verb-joins.R index 6c258f162..bf303ebcf 100644 --- a/tests/testthat/test-verb-joins.R +++ b/tests/testthat/test-verb-joins.R @@ -92,7 +92,7 @@ test_that("join works with in_schema", { ) out <- left_join(df4, df5, by = "x") expect_equal(out$lazy_query$table_names, tibble( - name = new_table_name(c("foo.df", "foo2.df")), + name = table_name(c("foo.df", "foo2.df")), from = c("name", "name") )) }) @@ -291,56 +291,56 @@ test_that("join uses correct table alias", { # self joins table_names <- sql_build(left_join(x, x, by = "a"))$table_names - expect_equal(table_names, new_table_name(c("`x_LHS`", "`x_RHS`"))) + expect_equal(table_names, table_name(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, new_table_name(c("`my_x`", "`x`"))) + expect_equal(table_names, table_name(c("`my_x`", "`x`"))) table_names <- sql_build(left_join(x, x, by = "a", y_as = "my_y"))$table_names - expect_equal(table_names, new_table_name(c("`x`", "`my_y`"))) + expect_equal(table_names, table_name(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, new_table_name(c("`my_x`", "`my_y`"))) + expect_equal(table_names, table_name(c("`my_x`", "`my_y`"))) # x-y joins table_names <- sql_build(left_join(x, y, by = "a"))$table_names - expect_equal(table_names, new_table_name(c("`x`", "`y`"))) + expect_equal(table_names, table_name(c("`x`", "`y`"))) table_names <- sql_build(left_join(x, y, by = "a", x_as = "my_x"))$table_names - expect_equal(table_names, new_table_name(c("`my_x`", "`y`"))) + expect_equal(table_names, table_name(c("`my_x`", "`y`"))) table_names <- sql_build(left_join(x, y, by = "a", y_as = "my_y"))$table_names - expect_equal(table_names, new_table_name(c("`x`", "`my_y`"))) + expect_equal(table_names, table_name(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, new_table_name(c("`my_x`", "`my_y`"))) + expect_equal(table_names, table_name(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, new_table_name(c("`y`", "`y...2`"))) + expect_equal(table_names, table_name(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, new_table_name(c("`LHS...1`", "`LHS`"))) + expect_equal(table_names, table_name(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, new_table_name(c("`LHS`", "`RHS`"))) + expect_equal(table_names, table_name(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, new_table_name(c("`my_x`", "`RHS`"))) + expect_equal(table_names, table_name(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, new_table_name(c("`x`", "`y`", "`z`"))) + expect_equal(out$table_names, table_name(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, new_table_name(c("`x...1`", "`x...2`", "`z`"))) + expect_equal(out$table_names, table_name(c("`x...1`", "`x...2`", "`z`"))) }) test_that("select() before join is inlined", { @@ -626,7 +626,7 @@ test_that("multiple joins create a single query", { lq <- out$lazy_query expect_s3_class(lq, "lazy_multi_join_query") expect_equal(lq$table_names, tibble( - name = new_table_name(c("`df1`", "`df2`", "`df3`")), + name = table_name(c("`df1`", "`df2`", "`df3`")), from = "name" )) expect_equal(lq$vars$name, c("x", "a", "b.x", "b.y")) @@ -729,7 +729,7 @@ test_that("multi joins work with x_as", { expect_s3_class(lq, "lazy_multi_join_query") expect_equal( lq$table_names, - tibble(name = new_table_name(c("`lf1`", "`lf2`", "`lf3`")), from = "as") + tibble(name = table_name(c("`lf1`", "`lf2`", "`lf3`")), from = "as") ) # `x_as` provided twice with the same name -> one query From fa83a18eae385297e805b392edd2f61bd92594b5 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Wed, 29 Nov 2023 07:35:07 -0600 Subject: [PATCH 20/51] Reorganise table-name file --- R/table-name.R | 75 ++++++++++++++++++++++++++++---------------------- 1 file changed, 42 insertions(+), 33 deletions(-) diff --git a/R/table-name.R b/R/table-name.R index 352095499..f682b7a37 100644 --- a/R/table-name.R +++ b/R/table-name.R @@ -2,37 +2,11 @@ # table id = interface to outside world; many ways to specify # table name = escaped string -# 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_name(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)") - } -} - -check_table_id <- function(x, arg = caller_arg(x), call = caller_env()) { - if (!is_table_id(x)) { - stop_input_type(x, "a table identifier") - } -} - +# table_name -------------------------------------------------------------- -is_table_id <- function(x) { - is_table_name(x) || - is.ident(x) || - is(x, "Id") || - is_catalog(x) || - is_schema(x) || - is.character(x) +table_name <- function(x) { + check_character(x) + structure(x, class = "dbplyr_table_name") } as_table_name <- function(x, @@ -84,9 +58,6 @@ make_table_name <- function(x, con, collapse = TRUE) { table_name(x) } -table_name <- function(x) { - structure(x, class = "dbplyr_table_name") -} #' @export print.dbplyr_table_name <- function(x) { @@ -149,3 +120,41 @@ escape.dbplyr_table_name <- function(x, parens = FALSE, collapse = ", ", con = N out <- ifelse(alias == "" | alias == table_name, 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_name(x) || + is.ident(x) || + is(x, "Id") || + is_catalog(x) || + is_schema(x) || + is.character(x) +} + + +# 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_name(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)") + } +} + From 8ae739958bea1f86221a680dfdb33e08388b1ef6 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Wed, 29 Nov 2023 07:35:50 -0600 Subject: [PATCH 21/51] Fix R CMD check issues --- R/table-name.R | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/R/table-name.R b/R/table-name.R index f682b7a37..5ef30e593 100644 --- a/R/table-name.R +++ b/R/table-name.R @@ -29,7 +29,7 @@ as_table_name <- function(x, table_name(paste0(x, collapse = ".")) } else if (is.ident(x)) { make_table_name(unclass(x), con) - } else if (is(x, "Id")) { + } else if (methods::is(x, "Id")) { make_table_name(x@name, con) } else if (inherits(x, "dbplyr_catalog")) { make_table_name(c(unclass(x$catalog), unclass(x$schema), unclass(x$table)), con) @@ -60,7 +60,7 @@ make_table_name <- function(x, con, collapse = TRUE) { #' @export -print.dbplyr_table_name <- function(x) { +print.dbplyr_table_name <- function(x, ...) { cat(" ", paste0(style_kw(x), collapse = ", "), "\n", sep = "") } @@ -132,7 +132,7 @@ check_table_id <- function(x, arg = caller_arg(x), call = caller_env()) { is_table_id <- function(x) { is_table_name(x) || is.ident(x) || - is(x, "Id") || + methods::is(x, "Id") || is_catalog(x) || is_schema(x) || is.character(x) From 0048df6962de38d46725c132ad05ba06b984c3f9 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Wed, 29 Nov 2023 07:40:58 -0600 Subject: [PATCH 22/51] Move as and is functions closer & test together --- R/table-name.R | 77 ++++++++++++++--------------- tests/testthat/_snaps/table-name.md | 5 ++ tests/testthat/test-table-name.R | 46 ++++++++++++----- 3 files changed, 76 insertions(+), 52 deletions(-) diff --git a/R/table-name.R b/R/table-name.R index 5ef30e593..8c9bbb6ee 100644 --- a/R/table-name.R +++ b/R/table-name.R @@ -9,45 +9,6 @@ table_name <- function(x) { structure(x, class = "dbplyr_table_name") } -as_table_name <- function(x, - con, - error_arg = caller_arg(x), - error_call = caller_env()) { - check_con(con) - - if (is_table_name(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_name(unclass(x)) - } else if (inherits(x, "ident_q")) { - table_name(paste0(x, collapse = ".")) - } else if (is.ident(x)) { - make_table_name(unclass(x), con) - } else if (methods::is(x, "Id")) { - make_table_name(x@name, con) - } else if (inherits(x, "dbplyr_catalog")) { - make_table_name(c(unclass(x$catalog), unclass(x$schema), unclass(x$table)), con) - } else if (inherits(x, "dbplyr_schema")) { - make_table_name(c(unclass(x$schema), unclass(x$table)), con) - } else if (inherits(x, "AsIs")) { - check_string(unclass(x), allow_empty = FALSE, arg = error_arg, call = error_call) - table_name(unclass(x)) - } else if (is.character(x)) { - make_table_name(x, con, collapse = FALSE) - } else { - cli::cli_abort( - "{.arg {error_arg}} uses unknown specification for table name", - error_call = error_call - ) - } -} - make_table_name <- function(x, con, collapse = TRUE) { needs_quote <- !vapply(x, function(x) inherits(x, "AsIs"), logical(1)) x[needs_quote] <- sql_escape_ident(con, x[needs_quote]) @@ -138,6 +99,44 @@ is_table_id <- function(x) { is.character(x) } +as_table_name <- function(x, + con, + error_arg = caller_arg(x), + error_call = caller_env()) { + check_con(con) + + if (is_table_name(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_name(unclass(x)) + } else if (inherits(x, "ident_q")) { + table_name(paste0(x, collapse = ".")) + } else if (is.ident(x)) { + make_table_name(unclass(x), con) + } else if (methods::is(x, "Id")) { + make_table_name(x@name, con) + } else if (inherits(x, "dbplyr_catalog")) { + make_table_name(c(unclass(x$catalog), unclass(x$schema), unclass(x$table)), con) + } else if (inherits(x, "dbplyr_schema")) { + make_table_name(c(unclass(x$schema), unclass(x$table)), con) + } else if (inherits(x, "AsIs")) { + check_string(unclass(x), allow_empty = FALSE, arg = error_arg, call = error_call) + table_name(unclass(x)) + } else if (is.character(x)) { + make_table_name(x, con, collapse = FALSE) + } else { + cli::cli_abort( + "{.arg {error_arg}} uses unknown specification for table name", + error_call = error_call + ) + } +} # table source ------------------------------------------------------------ diff --git a/tests/testthat/_snaps/table-name.md b/tests/testthat/_snaps/table-name.md index d0c2820db..128a39578 100644 --- a/tests/testthat/_snaps/table-name.md +++ b/tests/testthat/_snaps/table-name.md @@ -10,6 +10,11 @@ Condition Error in `as_table_name()`: ! `1` uses unknown specification for table name + Code + as_table_name(I(1), con) + Condition + Error: + ! `I(1)` must be a single string, not the number 1. # as_table_name warns when using sql diff --git a/tests/testthat/test-table-name.R b/tests/testthat/test-table-name.R index 8b08dfa54..992d8a931 100644 --- a/tests/testthat/test-table-name.R +++ b/tests/testthat/test-table-name.R @@ -4,25 +4,44 @@ test_that("can coerce all user facing inputs", { con <- simulate_dbi() - expect_equal(table_name("`x`"), table_name("`x`")) - expect_equal(as_table_name("x", con), table_name("`x`")) - expect_equal(as_table_name(I("x"), con), table_name("x")) - expect_equal(as_table_name(ident("x"), con), table_name("`x`")) - expect_equal(as_table_name(ident_q("x"), con), table_name("x")) + x_esc <- table_name("`x`") + x_raw <- table_name("x") + + id <- table_name("x") + expect_true(is_table_id(id)) + expect_equal(id, x_raw) + + id <- "x" + expect_true(is_table_id(id)) + expect_equal(as_table_name(id, con), x_esc) + + id <- I("x") + expect_true(is_table_id(id)) + expect_equal(as_table_name(id, con), x_raw) + + id <- ident("x") + expect_true(is_table_id(id)) + expect_equal(as_table_name(id, con), x_esc) + + id <- ident_q("x") + expect_true(is_table_id(id)) + expect_equal(as_table_name(id, con), x_raw) id <- DBI::Id(schema = "foo", table = "bar") + expect_true(is_table_id(id)) expect_equal(as_table_name(id, con), table_name("`foo`.`bar`")) + + # strip names, simulating DBI 1.2.0 names(id@name) <- NULL expect_equal(as_table_name(id, con), table_name("`foo`.`bar`")) - expect_equal( - as_table_name(in_schema("foo", "bar"), con), - table_name("`foo`.`bar`") - ) - expect_equal( - as_table_name(in_catalog("foo", "bar", "baz"), con), - table_name("`foo`.`bar`.`baz`") - ) + id <- in_schema("foo", "bar") + expect_true(is_table_id(id)) + expect_equal(as_table_name(id, con), table_name("`foo`.`bar`")) + + id <- in_catalog("foo", "bar", "baz") + expect_true(is_table_id(id)) + expect_equal(as_table_name(id, con), table_name("`foo`.`bar`.`baz`")) }) test_that("as_table_name validates its inputs", { @@ -30,6 +49,7 @@ test_that("as_table_name validates its inputs", { expect_snapshot(error = TRUE, { as_table_name("x") as_table_name(1, con) + as_table_name(I(1), con) }) }) From 2ef6fba826ddadfe21c036b87bc15aa0bd645cbd Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Wed, 29 Nov 2023 12:22:09 -0600 Subject: [PATCH 23/51] More explaining & re-organising --- R/table-name.R | 33 ++++++++++++++++++++------------- 1 file changed, 20 insertions(+), 13 deletions(-) diff --git a/R/table-name.R b/R/table-name.R index 8c9bbb6ee..dfadfda90 100644 --- a/R/table-name.R +++ b/R/table-name.R @@ -1,6 +1,15 @@ # table source = table id or sql -# table id = interface to outside world; many ways to specify -# table name = escaped string + +# table id = +# * interface to outside world; many ways to specify +# * always refers to exactly one table +# * but all converted to table name ASAP + +# table name +# * escaped string +# * internal (and backend) use only; not user facing +# * can be vector of multiple names +# * object names are always assumed to be table names # table_name -------------------------------------------------------------- @@ -9,17 +18,10 @@ table_name <- function(x) { structure(x, class = "dbplyr_table_name") } -make_table_name <- function(x, con, collapse = TRUE) { - needs_quote <- !vapply(x, function(x) inherits(x, "AsIs"), logical(1)) - x[needs_quote] <- sql_escape_ident(con, x[needs_quote]) - if (collapse) { - x <- paste0(x, collapse = ".") - } - - table_name(x) +is_table_name <- function(x) { + inherits(x, "dbplyr_table_name") } - #' @export print.dbplyr_table_name <- function(x, ...) { cat(" ", paste0(style_kw(x), collapse = ", "), "\n", sep = "") @@ -39,9 +41,14 @@ print.dbplyr_table_name <- function(x, ...) { table_name(NextMethod()) } +make_table_name <- function(x, con, collapse = TRUE) { + needs_quote <- !vapply(x, function(x) inherits(x, "AsIs"), logical(1)) + x[needs_quote] <- sql_escape_ident(con, x[needs_quote]) + if (collapse) { + x <- paste0(x, collapse = ".") + } -is_table_name <- function(x) { - inherits(x, "dbplyr_table_name") + table_name(x) } # TODO: make this generic From 07aee8e1efb67c7f63d3ff771a97f93247f49f62 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Wed, 29 Nov 2023 12:46:23 -0600 Subject: [PATCH 24/51] Make as_table_name() stricter Introducing new as_table_names() for internal use. --- R/lazy-join-query.R | 2 +- R/table-name.R | 7 ++++++- tests/testthat/_snaps/table-name.md | 5 +++++ tests/testthat/test-table-name.R | 1 + 4 files changed, 13 insertions(+), 2 deletions(-) diff --git a/R/lazy-join-query.R b/R/lazy-join-query.R index 1ae9bfa0a..c1aaa85bb 100644 --- a/R/lazy-join-query.R +++ b/R/lazy-join-query.R @@ -216,7 +216,7 @@ generate_join_table_names <- function(table_names, con) { method = "both.sides" ) - as_table_name(abbr_names, con) + as_table_names(abbr_names, con) } #' @export diff --git a/R/table-name.R b/R/table-name.R index dfadfda90..70507ccc9 100644 --- a/R/table-name.R +++ b/R/table-name.R @@ -51,9 +51,13 @@ make_table_name <- function(x, con, collapse = TRUE) { table_name(x) } +as_table_names <- function(x, con) { + make_table_name(x, con, collapse = FALSE) +} + # TODO: make this generic db_parse_table_name <- function(con, x) { - quote_char <- substr(as_table_name("", con = con), 1, 1) + quote_char <- substr( sql_escape_ident(con, ""), 1, 1) scan( text = x, what = character(), @@ -136,6 +140,7 @@ as_table_name <- function(x, check_string(unclass(x), allow_empty = FALSE, arg = error_arg, call = error_call) table_name(unclass(x)) } else if (is.character(x)) { + check_string(x, allow_empty = FALSE, arg = error_arg, call = error_call) make_table_name(x, con, collapse = FALSE) } else { cli::cli_abort( diff --git a/tests/testthat/_snaps/table-name.md b/tests/testthat/_snaps/table-name.md index 128a39578..3cec30664 100644 --- a/tests/testthat/_snaps/table-name.md +++ b/tests/testthat/_snaps/table-name.md @@ -5,6 +5,11 @@ Condition Error in `as_table_name()`: ! argument "con" is missing, with no default + Code + as_table_name(c("x", "y"), con) + Condition + Error: + ! `c("x", "y")` must be a single string, not a character vector. Code as_table_name(1, con) Condition diff --git a/tests/testthat/test-table-name.R b/tests/testthat/test-table-name.R index 992d8a931..00e1aa59e 100644 --- a/tests/testthat/test-table-name.R +++ b/tests/testthat/test-table-name.R @@ -48,6 +48,7 @@ test_that("as_table_name validates its inputs", { con <- simulate_dbi() expect_snapshot(error = TRUE, { as_table_name("x") + as_table_name(c("x", "y"), con) as_table_name(1, con) as_table_name(I(1), con) }) From 90ee786fbe7d30151b5d98201b1b30ac1377c2a2 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Wed, 29 Nov 2023 12:48:45 -0600 Subject: [PATCH 25/51] Test more methods; fix test --- tests/testthat/_snaps/table-name.md | 8 ++++++++ tests/testthat/test-table-name.R | 13 ++++++++++++- 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/tests/testthat/_snaps/table-name.md b/tests/testthat/_snaps/table-name.md index 3cec30664..585edc539 100644 --- a/tests/testthat/_snaps/table-name.md +++ b/tests/testthat/_snaps/table-name.md @@ -1,3 +1,11 @@ +# table_name possess key methods + + Code + name <- table_name(c("x", "y", "z")) + name + Output + x, y, z + # as_table_name validates its inputs Code diff --git a/tests/testthat/test-table-name.R b/tests/testthat/test-table-name.R index 00e1aa59e..9d1968352 100644 --- a/tests/testthat/test-table-name.R +++ b/tests/testthat/test-table-name.R @@ -1,3 +1,14 @@ +test_that("table_name possess key methods", { + expect_snapshot({ + name <- table_name(c("x", "y", "z")) + name + }) + + expect_equal(name[c(1, 3)], table_name(c("x", "z"))) + expect_equal(name[[2]], table_name("y")) + expect_equal(c(name[[1]], name[[2]]), table_name(c("x", "y"))) +}) + # as_table_name ----------------------------------------------------------- @@ -9,7 +20,7 @@ test_that("can coerce all user facing inputs", { id <- table_name("x") expect_true(is_table_id(id)) - expect_equal(id, x_raw) + expect_equal(as_table_name(id, con), x_raw) id <- "x" expect_true(is_table_id(id)) From 0baaf3cb39321b5a4d296bbb76ff9b28aa9f37d5 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Thu, 30 Nov 2023 08:36:55 -0600 Subject: [PATCH 26/51] Update old function --- R/lazy-ops.R | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/R/lazy-ops.R b/R/lazy-ops.R index 786383b1c..1bc797ea5 100644 --- a/R/lazy-ops.R +++ b/R/lazy-ops.R @@ -45,7 +45,7 @@ base_query <- function(from) { #' @export print.lazy_base_remote_query <- function(x, ...) { - if (is_table_ident(x$x)) { + if (is_table_name(x$x)) { cat_line("From: ", format(x$x)) } else { cat_line("From: ") From fbc23aab091b08998e2390a59fc337c4a1ea80cb Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Thu, 30 Nov 2023 08:38:32 -0600 Subject: [PATCH 27/51] Tweak argument checking to hopefully fix problems on older R --- R/table-name.R | 2 +- tests/testthat/_snaps/table-name.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/R/table-name.R b/R/table-name.R index 70507ccc9..c0e211d1a 100644 --- a/R/table-name.R +++ b/R/table-name.R @@ -114,7 +114,7 @@ as_table_name <- function(x, con, error_arg = caller_arg(x), error_call = caller_env()) { - check_con(con) + check_required(con) if (is_table_name(x)) { x diff --git a/tests/testthat/_snaps/table-name.md b/tests/testthat/_snaps/table-name.md index 585edc539..463a2ec30 100644 --- a/tests/testthat/_snaps/table-name.md +++ b/tests/testthat/_snaps/table-name.md @@ -12,7 +12,7 @@ as_table_name("x") Condition Error in `as_table_name()`: - ! argument "con" is missing, with no default + ! `con` is absent but must be supplied. Code as_table_name(c("x", "y"), con) Condition From a749f3c27d091da9e6211ef8834d6c283696aaac Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Thu, 30 Nov 2023 08:53:36 -0600 Subject: [PATCH 28/51] Simplify code; keep question --- R/lazy-join-query.R | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/R/lazy-join-query.R b/R/lazy-join-query.R index c1aaa85bb..71f356130 100644 --- a/R/lazy-join-query.R +++ b/R/lazy-join-query.R @@ -172,9 +172,9 @@ sql_build.lazy_multi_join_query <- function(op, con, ..., sql_options = NULL) { op$joins$by <- purrr::map2( op$joins$by, seq_along(op$joins$by), function(by, i) { - # FIXME: why is unique now needed? - by$x_as <- table_name(table_names_out[unique(op$joins$by_x_table_id[[i]])]) - by$y_as <- table_name(table_names_out[i + 1L]) + # @mgrlich: x_as should be a single value, right? + by$x_as <- table_names_out[unique(op$joins$by_x_table_id[[i]])] + by$y_as <- table_names_out[i + 1L] by } ) From 2252c7957cf6dbe04394a900f8c4ad5d842070b5 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Thu, 30 Nov 2023 08:55:29 -0600 Subject: [PATCH 29/51] WS --- R/lazy-set-op-query.R | 1 - 1 file changed, 1 deletion(-) diff --git a/R/lazy-set-op-query.R b/R/lazy-set-op-query.R index 97ba6b1dd..72323a803 100644 --- a/R/lazy-set-op-query.R +++ b/R/lazy-set-op-query.R @@ -79,7 +79,6 @@ op_vars.lazy_union_query <- function(op) { #' @export sql_build.lazy_union_query <- function(op, con, ..., sql_options = NULL) { # add_union() ensures that both have same variables - unions <- list( table = purrr::map( op$unions$table, From 0ebc70fae3e3044e42c7d21bbf2d12949ebadb74 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Fri, 1 Dec 2023 12:47:52 -0600 Subject: [PATCH 30/51] Update temporary table prefix --- R/backend-hana.R | 6 +----- R/backend-mssql.R | 14 ++----------- R/utils.R | 27 +++++++++++++++++++------- tests/testthat/_snaps/backend-mssql.md | 7 +++++++ tests/testthat/test-backend-mssql.R | 9 +++++++++ 5 files changed, 39 insertions(+), 24 deletions(-) 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 762355620..a50fe7df7 100644 --- a/R/backend-mssql.R +++ b/R/backend-mssql.R @@ -469,23 +469,13 @@ mssql_version <- function(con) { # #' @export `db_table_temporary.Microsoft SQL Server` <- function(con, table, temporary, ...) { - table <- as_table_name(table, con) - - # TODO: FIX ME - - 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/utils.R b/R/utils.R index 4872dd5f3..66e22ca66 100644 --- a/R/utils.R +++ b/R/utils.R @@ -82,13 +82,26 @@ 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( - paste0("Created a temporary table named ", name), - class = c("dbplyr_message_temp_table", "dbplyr_message") - ) - name +add_temporary_prefix <- function(con, table, temporary = TRUE) { + if (!temporary) { + return(table) + } + + table <- as_table_name(table, con) + pieces <- db_parse_table_name(con, table) + 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_name(pieces, con) + } + + table } # nocov end diff --git a/tests/testthat/_snaps/backend-mssql.md b/tests/testthat/_snaps/backend-mssql.md index 69d4c1ece..59be7704e 100644 --- a/tests/testthat/_snaps/backend-mssql.md +++ b/tests/testthat/_snaps/backend-mssql.md @@ -400,3 +400,10 @@ FROM `df` ORDER BY `y` +# add prefix to temporary table + + Code + out <- db_table_temporary(con, I("foo.bar"), temporary = TRUE) + Message + Created a temporary table named #bar + diff --git a/tests/testthat/test-backend-mssql.R b/tests/testthat/test-backend-mssql.R index dfc05c91a..a4c1a07ff 100644 --- a/tests/testthat/test-backend-mssql.R +++ b/tests/testthat/test-backend-mssql.R @@ -347,6 +347,15 @@ test_that("can copy_to() and compute() with temporary tables (#272)", { expect_equal(db2 %>% pull(), 2:4) }) +test_that("add prefix to temporary table", { + con <- simulate_mssql() + expect_snapshot(out <- db_table_temporary(con, I("foo.bar"), temporary = TRUE)) + expect_equal(out, list(table = table_name("`foo`.`#bar`"), temporary = FALSE)) + + expect_silent(out <- db_table_temporary(con, I("foo.#bar"), temporary = TRUE)) + expect_equal(out, list(table = table_name("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()) From 502436394177948bfbfa982292412add71ea0c28 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Fri, 1 Dec 2023 12:50:40 -0600 Subject: [PATCH 31/51] Add table name check --- R/table-name.R | 12 ++++++++++++ tests/testthat/_snaps/table-name.md | 10 ++++++++++ tests/testthat/test-table-name.R | 4 ++++ 3 files changed, 26 insertions(+) diff --git a/R/table-name.R b/R/table-name.R index c0e211d1a..77dca7434 100644 --- a/R/table-name.R +++ b/R/table-name.R @@ -110,6 +110,18 @@ is_table_id <- function(x) { is.character(x) } +check_table_name <- function(x, + error_arg = caller_arg(x), + error_call = caller_env()) { + if (!is_table_name(x)) { + cli::cli_abort( + "{.arg {error_arg}} must be a , not {.obj_type_friendly x}.", + call = error_call, + .internal = TRUE + ) + } +} + as_table_name <- function(x, con, error_arg = caller_arg(x), diff --git a/tests/testthat/_snaps/table-name.md b/tests/testthat/_snaps/table-name.md index 463a2ec30..f703522e4 100644 --- a/tests/testthat/_snaps/table-name.md +++ b/tests/testthat/_snaps/table-name.md @@ -6,6 +6,16 @@ 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_name validates its inputs Code diff --git a/tests/testthat/test-table-name.R b/tests/testthat/test-table-name.R index 9d1968352..36b5e16cc 100644 --- a/tests/testthat/test-table-name.R +++ b/tests/testthat/test-table-name.R @@ -9,6 +9,10 @@ test_that("table_name possess key methods", { expect_equal(c(name[[1]], name[[2]]), table_name(c("x", "y"))) }) +test_that("can check for table name", { + foo <- function(y) check_table_name(y) + expect_snapshot(foo(1), error = TRUE) +}) # as_table_name ----------------------------------------------------------- From 12f682f6f3e48efd95a1d9275a6764029c1da95f Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Fri, 1 Dec 2023 13:33:06 -0600 Subject: [PATCH 32/51] Implement check_table_name() And use where appropriate --- R/backend-mysql.R | 1 - R/backend-postgres.R | 1 - R/backend-spark-sql.R | 1 - R/db-io.R | 2 +- R/db-sql.R | 9 ++++++--- R/db.R | 1 + R/utils.R | 3 ++- tests/testthat/_snaps/backend-.md | 4 ++-- tests/testthat/_snaps/backend-mssql.md | 2 +- tests/testthat/test-backend-mssql.R | 8 ++++++-- 10 files changed, 19 insertions(+), 13 deletions(-) diff --git a/R/backend-mysql.R b/R/backend-mysql.R index 8e7aad7a3..1c9d8a951 100644 --- a/R/backend-mysql.R +++ b/R/backend-mysql.R @@ -56,7 +56,6 @@ db_connection_describe.MySQLConnection <- db_connection_describe.MariaDBConnecti #' @export db_col_types.MariaDBConnection <- function(con, table, call) { - table <- as_table_name(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.R b/R/backend-postgres.R index 929b1114b..a4e3ee401 100644 --- a/R/backend-postgres.R +++ b/R/backend-postgres.R @@ -407,7 +407,6 @@ db_supports_table_alias_with_as.PostgreSQL <- function(con) { #' @export db_col_types.PqConnection <- function(con, table, call) { - table <- as_table_name(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 67911290b..2ac282427 100644 --- a/R/backend-spark-sql.R +++ b/R/backend-spark-sql.R @@ -115,7 +115,6 @@ simulate_spark_sql <- function() simulate_dbi("Spark SQL") cli::cli_abort("Spark SQL only support temporary tables") } - table <- as_table_name(table, con) sql <- glue_sql2( con, "CREATE ", if (overwrite) "OR REPLACE ", diff --git a/R/db-io.R b/R/db-io.R index be69e3465..3786c68fa 100644 --- a/R/db-io.R +++ b/R/db-io.R @@ -178,7 +178,7 @@ db_write_table.DBIConnection <- function(con, temporary = TRUE, ..., overwrite = FALSE) { - table <- as_table_name(table, con) + check_table_name(table) check_character(types, allow_null = TRUE) check_named(types) check_bool(temporary) diff --git a/R/db-sql.R b/R/db-sql.R index d99a43f4b..5c201ab15 100644 --- a/R/db-sql.R +++ b/R/db-sql.R @@ -173,9 +173,12 @@ sql_table_index.DBIConnection <- function(con, unique = FALSE, ..., call = caller_env()) { - table <- as_table_name(table, con, error_call = call) - table <- db_table_name_extract(con, table) - name <- name %||% paste0(c(table, columns), collapse = "_") + table <- as_table_name(table, con) + + if (is.null(name)) { + table_name <- db_table_name_extract(con, table) + name <- name %||% paste0(c(table_name, columns), collapse = "_") + } glue_sql2( con, "CREATE ", if (unique) "UNIQUE ", "INDEX {.name name}", diff --git a/R/db.R b/R/db.R index 8576b5384..cb8b2dc87 100644 --- a/R/db.R +++ b/R/db.R @@ -91,6 +91,7 @@ db_col_types <- function(con, table, call) { return(NULL) } + check_table_name(table) UseMethod("db_col_types") } diff --git a/R/utils.R b/R/utils.R index 66e22ca66..cea2a1b9a 100644 --- a/R/utils.R +++ b/R/utils.R @@ -83,11 +83,12 @@ res_warn_incomplete <- function(res, hint = "n = -1") { } add_temporary_prefix <- function(con, table, temporary = TRUE) { + check_table_name(table) + if (!temporary) { return(table) } - table <- as_table_name(table, con) pieces <- db_parse_table_name(con, table) table_name <- pieces[length(pieces)] diff --git a/tests/testthat/_snaps/backend-.md b/tests/testthat/_snaps/backend-.md index c9bb64654..1fffbe4d9 100644 --- a/tests/testthat/_snaps/backend-.md +++ b/tests/testthat/_snaps/backend-.md @@ -89,14 +89,14 @@ Code sql_table_index(con, in_schema("schema", "tbl"), c("a", "b")) Output - CREATE INDEX `tbl_a_b` ON `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 `tbl_c` ON `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 59be7704e..0abf90281 100644 --- a/tests/testthat/_snaps/backend-mssql.md +++ b/tests/testthat/_snaps/backend-mssql.md @@ -403,7 +403,7 @@ # add prefix to temporary table Code - out <- db_table_temporary(con, I("foo.bar"), temporary = TRUE) + out <- db_table_temporary(con, table_name("foo.bar"), temporary = TRUE) Message Created a temporary table named #bar diff --git a/tests/testthat/test-backend-mssql.R b/tests/testthat/test-backend-mssql.R index a4c1a07ff..57de1ecf5 100644 --- a/tests/testthat/test-backend-mssql.R +++ b/tests/testthat/test-backend-mssql.R @@ -349,10 +349,14 @@ test_that("can copy_to() and compute() with temporary tables (#272)", { test_that("add prefix to temporary table", { con <- simulate_mssql() - expect_snapshot(out <- db_table_temporary(con, I("foo.bar"), temporary = TRUE)) + expect_snapshot( + out <- db_table_temporary(con, table_name("foo.bar"), temporary = TRUE) + ) expect_equal(out, list(table = table_name("`foo`.`#bar`"), temporary = FALSE)) - expect_silent(out <- db_table_temporary(con, I("foo.#bar"), temporary = TRUE)) + expect_silent( + out <- db_table_temporary(con, table_name("foo.#bar"), temporary = TRUE) + ) expect_equal(out, list(table = table_name("foo.#bar"), temporary = FALSE)) }) From bff01305e016402c2acda9088ed1d303907d2cb6 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Fri, 1 Dec 2023 13:40:56 -0600 Subject: [PATCH 33/51] unique_subquery_name() no longer needs con --- R/backend-teradata.R | 2 +- R/db-sql.R | 6 +++--- R/lazy-select-query.R | 2 +- R/query-join.R | 2 +- R/query-set-op.R | 2 +- R/sql-build.R | 2 +- R/table-name.R | 2 +- R/utils.R | 4 ++-- tests/testthat/test-table-name.R | 5 +++++ tests/testthat/test-verb-compute.R | 3 +-- 10 files changed, 17 insertions(+), 13 deletions(-) diff --git a/R/backend-teradata.R b/R/backend-teradata.R index 425a2348b..fe2007ee4 100644 --- a/R/backend-teradata.R +++ b/R/backend-teradata.R @@ -65,7 +65,7 @@ sql_query_select.Teradata <- function(con, lvl = lvl + 1 ) - alias <- unique_subquery_name(con) + alias <- unique_subquery_name() from <- sql_query_wrap(con, unlimited_query, name = alias) select_outer <- sql_star(con, alias) out <- sql_select_clauses(con, diff --git a/R/db-sql.R b/R/db-sql.R index 5c201ab15..4eb03df13 100644 --- a/R/db-sql.R +++ b/R/db-sql.R @@ -255,8 +255,8 @@ 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(con) - glue_sql2(con, "{from}", as_sql, "{.name name}") + name <- name %||% unique_subquery_name() + glue_sql2(con, "{from}", as_sql, "{.tbl name}") } else { # must be a table_name if (!is.null(name)) { table <- db_table_name_extract(con, name) @@ -1172,7 +1172,7 @@ dbplyr_sql_subquery <- function(con, ...) { #' @importFrom dplyr sql_subquery sql_subquery.DBIConnection <- function(con, from, - name = unique_subquery_name(con), + name = unique_subquery_name(), ..., lvl = 0) { sql_query_wrap(con, from = from, name = name, ..., lvl = lvl) diff --git a/R/lazy-select-query.R b/R/lazy-select-query.R index d07f36da2..84e81c9f0 100644 --- a/R/lazy-select-query.R +++ b/R/lazy-select-query.R @@ -161,7 +161,7 @@ sql_build.lazy_select_query <- function(op, con, ..., sql_options = NULL) { inform(op$message_summarise) } - alias <- remote_name(op$x, null_if_local = FALSE) %||% unique_subquery_name(con) + alias <- remote_name(op$x, null_if_local = FALSE) %||% unique_subquery_name() from <- sql_build(op$x, con, sql_options = sql_options) select_sql_list <- get_select_sql( select = op$select, diff --git a/R/query-join.R b/R/query-join.R index 9b44223f5..a51df1341 100644 --- a/R/query-join.R +++ b/R/query-join.R @@ -127,7 +127,7 @@ flatten_query.multi_join_query <- function(qry, query_list, con) { } # TODO reuse query - name <- unique_subquery_name(con) + name <- as_table_name(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/query-set-op.R b/R/query-set-op.R index 1f2566717..5571a03ee 100644 --- a/R/query-set-op.R +++ b/R/query-set-op.R @@ -117,7 +117,7 @@ flatten_query.union_query <- function(qry, query_list, con) { } # TODO reuse query - name <- unique_subquery_name(con) + name <- as_table_name(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/sql-build.R b/R/sql-build.R index ccc662475..cd4d4b0f7 100644 --- a/R/sql-build.R +++ b/R/sql-build.R @@ -169,7 +169,7 @@ querylist_reuse_query <- function(qry, query_list, con) { if (!is.na(id)) { query_list$name <- names(query_list$queries)[[id]] } else { - name <- unique_subquery_name(con) + name <- as_table_name(unique_subquery_name(), con) wrapped_query <- set_names(list(qry), name) query_list$queries <- c(query_list$queries, wrapped_query) query_list$name <- name diff --git a/R/table-name.R b/R/table-name.R index 77dca7434..6c2b0f268 100644 --- a/R/table-name.R +++ b/R/table-name.R @@ -153,7 +153,7 @@ as_table_name <- function(x, table_name(unclass(x)) } else if (is.character(x)) { check_string(x, allow_empty = FALSE, arg = error_arg, call = error_call) - make_table_name(x, con, collapse = FALSE) + make_table_name(unname(x), con, collapse = FALSE) } else { cli::cli_abort( "{.arg {error_arg}} uses unknown specification for table name", diff --git a/R/utils.R b/R/utils.R index cea2a1b9a..b82e3cbf5 100644 --- a/R/utils.R +++ b/R/utils.R @@ -26,11 +26,11 @@ unique_table_name <- function() { options(dbplyr_table_name = i) sprintf("dbplyr_%03i", i) } -unique_subquery_name <- function(con) { +unique_subquery_name <- function() { # Needs to use option so can reset at the start of each query i <- getOption("dbplyr_subquery_name", 0) + 1 options(dbplyr_subquery_name = i) - as_table_name(sprintf("q%02i", i), con) + sprintf("q%02i", i) } unique_column_name <- function() { # Needs to use option so can reset at the start of each query diff --git a/tests/testthat/test-table-name.R b/tests/testthat/test-table-name.R index 36b5e16cc..9cbd513c7 100644 --- a/tests/testthat/test-table-name.R +++ b/tests/testthat/test-table-name.R @@ -59,6 +59,11 @@ test_that("can coerce all user facing inputs", { expect_equal(as_table_name(id, con), table_name("`foo`.`bar`.`baz`")) }) +test_that("strips names", { + con <- simulate_dbi() + expect_equal(as_table_name(c(x = "x"), con), table_name("`x`")) +}) + test_that("as_table_name validates its inputs", { con <- simulate_dbi() expect_snapshot(error = TRUE, { diff --git a/tests/testthat/test-verb-compute.R b/tests/testthat/test-verb-compute.R index 363cd5766..4dc1e2282 100644 --- a/tests/testthat/test-verb-compute.R +++ b/tests/testthat/test-verb-compute.R @@ -84,10 +84,9 @@ test_that("compute keeps window and groups", { test_that("compute can handle named name", { con <- simulate_dbi() - name <- set_names(unique_subquery_name(con), unique_subquery_name(con)) expect_equal( memdb_frame(x = 1:10) %>% - compute() %>% + compute(name = c(x = unique_table_name())) %>% collect(), tibble(x = 1:10) ) From e7bef90df3a06698ddf9a738a51158b569143bcc Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Mon, 18 Dec 2023 13:49:45 -0600 Subject: [PATCH 34/51] Coerce rather than checking --- R/db.R | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/R/db.R b/R/db.R index cb8b2dc87..7b356cc01 100644 --- a/R/db.R +++ b/R/db.R @@ -91,7 +91,7 @@ db_col_types <- function(con, table, call) { return(NULL) } - check_table_name(table) + table <- as_table_name(table, con) UseMethod("db_col_types") } From ac051f7178e33fb921ead97252d0ab640fab900a Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Mon, 18 Dec 2023 16:10:33 -0600 Subject: [PATCH 35/51] RPostgreSQL needs unquoted table name --- R/backend-postgres-old.R | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/R/backend-postgres-old.R b/R/backend-postgres-old.R index 36b406b77..bba28f27c 100644 --- a/R/backend-postgres-old.R +++ b/R/backend-postgres-old.R @@ -27,7 +27,7 @@ db_write_table.PostgreSQLConnection <- function(con, # the bare table name dbWriteTable( con, - name = SQL(unclass(table)), + name = db_parse_table_name(con, table), value = values, field.types = types, ..., From b8e9b623545ee7ae4d70e4b7381729e222813321 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Mon, 18 Dec 2023 16:26:57 -0600 Subject: [PATCH 36/51] Disable windows R3.6 checking for RMariaDB --- .github/workflows/R-CMD-check.yaml | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/.github/workflows/R-CMD-check.yaml b/.github/workflows/R-CMD-check.yaml index ee65ccb57..95e951b2c 100644 --- a/.github/workflows/R-CMD-check.yaml +++ b/.github/workflows/R-CMD-check.yaml @@ -25,8 +25,10 @@ jobs: - {os: macos-latest, r: 'release'} - {os: windows-latest, r: 'release'} + # # some database packages (e.g. RMariaDB) might not work on old R + # versions on windows #1382 # Use 3.6 to trigger usage of RTools35 - - {os: windows-latest, r: '3.6'} + # - {os: windows-latest, r: '3.6'} # use 4.1 to check with rtools40's older compiler - {os: windows-latest, r: '4.1'} From 7ddea50e7fb88e35b3e9dbb345e63220183371d4 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Mon, 18 Dec 2023 16:28:40 -0600 Subject: [PATCH 37/51] Use correct function name --- R/backend-postgres-old.R | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/R/backend-postgres-old.R b/R/backend-postgres-old.R index bba28f27c..e16e3635b 100644 --- a/R/backend-postgres-old.R +++ b/R/backend-postgres-old.R @@ -27,7 +27,7 @@ db_write_table.PostgreSQLConnection <- function(con, # the bare table name dbWriteTable( con, - name = db_parse_table_name(con, table), + name = db_table_name_extract(con, table), value = values, field.types = types, ..., From 86aeea53915f89d4e04326a0061422d6bb614331 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Mon, 18 Dec 2023 17:17:32 -0600 Subject: [PATCH 38/51] Refine table_name_table() interface --- R/backend-postgres-old.R | 3 +-- R/db-sql.R | 4 ++-- R/lazy-join-query.R | 3 ++- R/query-join.R | 21 ++++++------------- R/remote.R | 2 +- R/table-name.R | 35 +++++++++++++++++--------------- R/utils.R | 2 +- tests/testthat/test-verb-joins.R | 8 ++++---- 8 files changed, 36 insertions(+), 42 deletions(-) diff --git a/R/backend-postgres-old.R b/R/backend-postgres-old.R index e16e3635b..885b92517 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_name(table, con) 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 = db_table_name_extract(con, table), + name = table_name_table(table, con), value = values, field.types = types, ..., diff --git a/R/db-sql.R b/R/db-sql.R index 4eb03df13..60b8c9455 100644 --- a/R/db-sql.R +++ b/R/db-sql.R @@ -176,7 +176,7 @@ sql_table_index.DBIConnection <- function(con, table <- as_table_name(table, con) if (is.null(name)) { - table_name <- db_table_name_extract(con, table) + table_name <- table_name_table(table, con) name <- name %||% paste0(c(table_name, columns), collapse = "_") } glue_sql2( @@ -259,7 +259,7 @@ sql_query_wrap.DBIConnection <- function(con, from, name = NULL, ..., lvl = 0) { glue_sql2(con, "{from}", as_sql, "{.tbl name}") } else { # must be a table_name if (!is.null(name)) { - table <- db_table_name_extract(con, name) + table <- table_name_table(name, con) names(from) <- as_table_name(table, con) } from diff --git a/R/lazy-join-query.R b/R/lazy-join-query.R index 71f356130..ffce3a6f7 100644 --- a/R/lazy-join-query.R +++ b/R/lazy-join-query.R @@ -153,6 +153,7 @@ 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, con) tables <- set_names(c(list(op$x), op$joins$table), table_names_out) @@ -188,7 +189,7 @@ sql_build.lazy_multi_join_query <- function(op, con, ..., sql_options = NULL) { } generate_join_table_names <- function(table_names, con) { - names <- db_table_name_extract(con, table_names$name) + names <- table_name_table(table_names$name, con) table_name_length_max <- max(nchar(names)) if (length(table_names$name) != 2) { diff --git a/R/query-join.R b/R/query-join.R index a51df1341..0ef04e280 100644 --- a/R/query-join.R +++ b/R/query-join.R @@ -331,29 +331,20 @@ sql_join_tbls <- function(con, by, na_matches) { } 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_name(table, con) - table <- db_table_name_extract(con, table) - table <- as_table_name(table, con) - - sql(paste0(table, ".", var)) - } else { - var - } + sql_table_name_prefix(con, table, var) } sql_star <- function(con, table = NULL) { - # TODO: sql_table_prefix(con, "*", table) ? - var <- sql("*") + sql_table_name_prefix(con, table, sql("*")) +} + +sql_table_name_prefix <- function(con, table, var) { if (!is.null(table)) { - table <- as_table_name(table, con) - table <- db_table_name_extract(con, table) + table <- table_name_table(table, con) table <- as_table_name(table, con) sql(paste0(table, ".", var)) diff --git a/R/remote.R b/R/remote.R index ef176c9f9..b303ccc04 100644 --- a/R/remote.R +++ b/R/remote.R @@ -40,7 +40,7 @@ remote_name <- function(x, null_if_local = TRUE) { if (is.null(con)) { table } else { - db_table_name_extract(con, table) + table_name_table(table, con) } } } diff --git a/R/table-name.R b/R/table-name.R index 6c2b0f268..372c9ebe1 100644 --- a/R/table-name.R +++ b/R/table-name.R @@ -55,9 +55,20 @@ as_table_names <- function(x, con) { make_table_name(x, con, collapse = FALSE) } -# TODO: make this generic -db_parse_table_name <- function(con, x) { - quote_char <- substr( sql_escape_ident(con, ""), 1, 1) +# Extract just the table name from a full identifier +table_name_table <- function(x, con) { + x <- as_table_name(x, con) + + vapply(x, FUN.VALUE = character(1), function(x) { + if (x == "") return("") + + out <- table_name_components(x, con) + out[[length(out)]] + }) +} +table_name_components <- function(x, con) { + quote_char <- substr(sql_escape_ident(con, ""), 1, 1) + scan( text = x, what = character(), @@ -67,21 +78,13 @@ db_parse_table_name <- function(con, x) { sep = "." ) } -db_table_name_extract <- function(con, x) { - vapply(x, FUN.VALUE = character(1), function(x) { - if (x == "") return("") - - out <- db_parse_table_name(con, x) - out[[length(out)]] - }) -} #' @export escape.dbplyr_table_name <- function(x, parens = FALSE, collapse = ", ", con = NULL) { - alias <- names2(x) # assume alias is already escaped - x <- unname(x) - - table_name <- as_table_name(db_table_name_extract(con, x), con) + # names are always already escaped + alias <- names2(x) + table_name <- as_table_name(table_name_table(x, con), con) + has_alias <- alias == "" | alias == table_name if (db_supports_table_alias_with_as(con)) { as_sql <- style_kw(" AS ") @@ -89,7 +92,7 @@ escape.dbplyr_table_name <- function(x, parens = FALSE, collapse = ", ", con = N as_sql <- " " } - out <- ifelse(alias == "" | alias == table_name, x, paste0(x, as_sql, alias)) + out <- ifelse(has_alias, unname(x), paste0(x, as_sql, alias)) sql_vector(out, parens, collapse, con = con) } diff --git a/R/utils.R b/R/utils.R index b82e3cbf5..945039294 100644 --- a/R/utils.R +++ b/R/utils.R @@ -89,7 +89,7 @@ add_temporary_prefix <- function(con, table, temporary = TRUE) { return(table) } - pieces <- db_parse_table_name(con, table) + pieces <- table_name_components(table, con) table_name <- pieces[length(pieces)] if (substr(table_name, 1, 1) != "#") { diff --git a/tests/testthat/test-verb-joins.R b/tests/testthat/test-verb-joins.R index bf303ebcf..e8e833a88 100644 --- a/tests/testthat/test-verb-joins.R +++ b/tests/testthat/test-verb-joins.R @@ -102,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_name(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_name(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 From af52e5e5318e60f034138a553eeaf55ad93153d4 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Mon, 18 Dec 2023 17:21:06 -0600 Subject: [PATCH 39/51] Teach S4 that dbplyr_table_name inherits from character --- R/db-io.R | 2 +- R/db-sql.R | 4 ++-- R/table-name.R | 5 ++++- 3 files changed, 7 insertions(+), 4 deletions(-) diff --git a/R/db-io.R b/R/db-io.R index 3786c68fa..084520620 100644 --- a/R/db-io.R +++ b/R/db-io.R @@ -187,7 +187,7 @@ db_write_table.DBIConnection <- function(con, withCallingHandlers( dbWriteTable( con, - name = SQL(unclass(table)), + name = SQL(table), value = values, field.types = types, temporary = temporary, diff --git a/R/db-sql.R b/R/db-sql.R index 60b8c9455..536adcc1d 100644 --- a/R/db-sql.R +++ b/R/db-sql.R @@ -1148,9 +1148,9 @@ db_save_query.DBIConnection <- function(con, withCallingHandlers( { if (overwrite) { - found <- DBI::dbExistsTable(con, SQL(unclass(name))) + found <- DBI::dbExistsTable(con, SQL(name)) if (found) { - DBI::dbRemoveTable(con, SQL(unclass(name))) + DBI::dbRemoveTable(con, SQL(name)) } } DBI::dbExecute(con, sql, immediate = TRUE) diff --git a/R/table-name.R b/R/table-name.R index 372c9ebe1..19bb0921f 100644 --- a/R/table-name.R +++ b/R/table-name.R @@ -15,9 +15,12 @@ table_name <- function(x) { check_character(x) - structure(x, class = "dbplyr_table_name") + structure(x, class = c("dbplyr_table_name", "character")) } +# So you can do SQL(table_name("foo")) +setOldClass(c("dbplyr_table_name", "character")) + is_table_name <- function(x) { inherits(x, "dbplyr_table_name") } From fd5eaec9143f52a7c7370502f57c494180f50262 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Mon, 18 Dec 2023 17:29:55 -0600 Subject: [PATCH 40/51] Move coercion back to methods (Since that's where it actually works) --- R/backend-mysql.R | 1 + R/backend-postgres.R | 1 + R/db.R | 1 - 3 files changed, 2 insertions(+), 1 deletion(-) diff --git a/R/backend-mysql.R b/R/backend-mysql.R index 1c9d8a951..8e7aad7a3 100644 --- a/R/backend-mysql.R +++ b/R/backend-mysql.R @@ -56,6 +56,7 @@ db_connection_describe.MySQLConnection <- db_connection_describe.MariaDBConnecti #' @export db_col_types.MariaDBConnection <- function(con, table, call) { + table <- as_table_name(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.R b/R/backend-postgres.R index a4e3ee401..929b1114b 100644 --- a/R/backend-postgres.R +++ b/R/backend-postgres.R @@ -407,6 +407,7 @@ db_supports_table_alias_with_as.PostgreSQL <- function(con) { #' @export db_col_types.PqConnection <- function(con, table, call) { + table <- as_table_name(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/db.R b/R/db.R index 7b356cc01..8576b5384 100644 --- a/R/db.R +++ b/R/db.R @@ -91,7 +91,6 @@ db_col_types <- function(con, table, call) { return(NULL) } - table <- as_table_name(table, con) UseMethod("db_col_types") } From dd2d0c34cce7f4746d26a27a09e0e291c02cc633 Mon Sep 17 00:00:00 2001 From: Maximilian Girlich Date: Mon, 22 Jan 2024 13:43:10 +0000 Subject: [PATCH 41/51] Use dev odbc for testing MS SQL --- DESCRIPTION | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/DESCRIPTION b/DESCRIPTION index 6f09d0388..45543ca7c 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -27,6 +27,7 @@ Imports: lifecycle (>= 1.0.3), magrittr, methods, + odbc (>= 1.4.1), pillar (>= 1.9.0), purrr (>= 1.0.1), R6 (>= 2.2.2), @@ -43,7 +44,6 @@ Suggests: knitr, Lahman, nycflights13, - odbc, RMariaDB (>= 1.2.2), rmarkdown, RPostgres (>= 1.4.5), @@ -159,3 +159,5 @@ Collate: 'verb-uncount.R' 'verb-window.R' 'zzz.R' +Remotes: + r-dbi/odbc@11d48a3 From 2e83a3679b0f74d394dc1201e662219da77ddd96 Mon Sep 17 00:00:00 2001 From: Maximilian Girlich Date: Wed, 31 Jan 2024 07:43:27 +0000 Subject: [PATCH 42/51] Remove remote odbc and require >= 1.4.2 instead --- DESCRIPTION | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/DESCRIPTION b/DESCRIPTION index 45543ca7c..d5683ae51 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -27,7 +27,6 @@ Imports: lifecycle (>= 1.0.3), magrittr, methods, - odbc (>= 1.4.1), pillar (>= 1.9.0), purrr (>= 1.0.1), R6 (>= 2.2.2), @@ -44,6 +43,7 @@ Suggests: knitr, Lahman, nycflights13, + odbc (>= 1.4.2), RMariaDB (>= 1.2.2), rmarkdown, RPostgres (>= 1.4.5), @@ -159,5 +159,3 @@ Collate: 'verb-uncount.R' 'verb-window.R' 'zzz.R' -Remotes: - r-dbi/odbc@11d48a3 From 6c3c2a6507a50abf5a4d5d244997e4f9adc58b9c Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Wed, 14 Feb 2024 16:58:47 -0600 Subject: [PATCH 43/51] Update R/lazy-join-query.R Co-authored-by: Maximilian Girlich --- R/lazy-join-query.R | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/R/lazy-join-query.R b/R/lazy-join-query.R index ffce3a6f7..d9bda0d39 100644 --- a/R/lazy-join-query.R +++ b/R/lazy-join-query.R @@ -174,7 +174,7 @@ sql_build.lazy_multi_join_query <- function(op, con, ..., sql_options = NULL) { op$joins$by, seq_along(op$joins$by), function(by, i) { # @mgrlich: x_as should be a single value, right? - by$x_as <- table_names_out[unique(op$joins$by_x_table_id[[i]])] + by$x_as <- table_names_out[op$joins$by_x_table_id[[i]]] by$y_as <- table_names_out[i + 1L] by } From 091b04fd67d380e1ebde78c7d5d777bd736a1d99 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Wed, 14 Feb 2024 17:18:54 -0600 Subject: [PATCH 44/51] sql_table_name_prefix() improvements * Now always escapes `var` * New name: `sql_qualify_var` * Vectorised once more --- R/lazy-join-query.R | 1 - R/query-join.R | 12 ++++++------ R/simulate.R | 6 +++++- 3 files changed, 11 insertions(+), 8 deletions(-) diff --git a/R/lazy-join-query.R b/R/lazy-join-query.R index d9bda0d39..00c7c89a1 100644 --- a/R/lazy-join-query.R +++ b/R/lazy-join-query.R @@ -173,7 +173,6 @@ sql_build.lazy_multi_join_query <- function(op, con, ..., sql_options = NULL) { op$joins$by <- purrr::map2( op$joins$by, seq_along(op$joins$by), function(by, i) { - # @mgrlich: x_as should be a single value, right? by$x_as <- table_names_out[op$joins$by_x_table_id[[i]]] by$y_as <- table_names_out[i + 1L] by diff --git a/R/query-join.R b/R/query-join.R index 0ef04e280..296040f0b 100644 --- a/R/query-join.R +++ b/R/query-join.R @@ -184,7 +184,6 @@ sql_multi_join_vars <- function(con, vars, table_vars, use_star, qualify_all_col } table_names <- table_name(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)) { @@ -334,18 +333,19 @@ 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) - sql_table_name_prefix(con, table, var) + sql_qualify_var(con, table, var) } sql_star <- function(con, table = NULL) { - sql_table_name_prefix(con, table, sql("*")) + sql_qualify_var(con, table, SQL("*")) } -sql_table_name_prefix <- function(con, table, var) { +sql_qualify_var <- function(con, table, var) { + var <- sql_escape_ident(con, var) + if (!is.null(table)) { table <- table_name_table(table, con) - table <- as_table_name(table, con) + table <- as_table_names(table, con) sql(paste0(table, ".", var)) } else { diff --git a/R/simulate.R b/R/simulate.R index fe3be85c7..d98635633 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 (is(x, "SQL")) { + x + } else { + sql_quote(x, "`") + } } sql_escape_string <- function(con, x) { From b85ca5d3a7159aeef27ce21505115786c131c0a4 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Wed, 14 Feb 2024 17:36:14 -0600 Subject: [PATCH 45/51] Make `c()` safer --- R/query-join.R | 5 ++++- tests/testthat/test-query-join.R | 8 ++++---- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/R/query-join.R b/R/query-join.R index 296040f0b..22739fb61 100644 --- a/R/query-join.R +++ b/R/query-join.R @@ -249,7 +249,10 @@ sql_rf_join_vars <- function(con, use_star, qualify_all_columns) { type <- arg_match0(type, c("right", "full")) - table_names <- table_name(c(x_as, y_as)) + + check_table_name(x_as) + check_table_name(y_as) + table_names <- c(x_as, y_as) if (type == "full") { duplicated_vars <- intersect(tolower(vars$all_x), tolower(vars$all_y)) diff --git a/tests/testthat/test-query-join.R b/tests/testthat/test-query-join.R index 6e347dbde..38d5c3e3e 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_name("left"), + y_as = table_name("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_name("left"), + y_as = table_name("right"), use_star = TRUE, qualify_all_columns = FALSE ), From a434eb0d6b6881091ac19c9b2712a923fa967b83 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Thu, 15 Feb 2024 07:13:39 -0600 Subject: [PATCH 46/51] Handle unquoting within in_schema/in_catalog --- R/table-name.R | 15 +++++++++++---- tests/testthat/test-table-name.R | 10 ++++++++++ 2 files changed, 21 insertions(+), 4 deletions(-) diff --git a/R/table-name.R b/R/table-name.R index 19bb0921f..b7376a40e 100644 --- a/R/table-name.R +++ b/R/table-name.R @@ -15,6 +15,7 @@ table_name <- function(x) { check_character(x) + x <- unname(x) structure(x, class = c("dbplyr_table_name", "character")) } @@ -45,7 +46,9 @@ print.dbplyr_table_name <- function(x, ...) { } make_table_name <- function(x, con, collapse = TRUE) { - needs_quote <- !vapply(x, function(x) inherits(x, "AsIs"), logical(1)) + needs_quote <- !vapply(x, name_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 = ".") @@ -54,6 +57,10 @@ make_table_name <- function(x, con, collapse = TRUE) { table_name(x) } +name_is_escaped <- function(x) { + inherits(x, "AsIs") || is.sql(x) || inherits(x, "ident_q") +} + as_table_names <- function(x, con) { make_table_name(x, con, collapse = FALSE) } @@ -151,15 +158,15 @@ as_table_name <- function(x, } else if (methods::is(x, "Id")) { make_table_name(x@name, con) } else if (inherits(x, "dbplyr_catalog")) { - make_table_name(c(unclass(x$catalog), unclass(x$schema), unclass(x$table)), con) + make_table_name(list(x$catalog, x$schema, x$table), con) } else if (inherits(x, "dbplyr_schema")) { - make_table_name(c(unclass(x$schema), unclass(x$table)), con) + make_table_name(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_name(unclass(x)) } else if (is.character(x)) { check_string(x, allow_empty = FALSE, arg = error_arg, call = error_call) - make_table_name(unname(x), con, collapse = FALSE) + make_table_name(x, con, collapse = FALSE) } else { cli::cli_abort( "{.arg {error_arg}} uses unknown specification for table name", diff --git a/tests/testthat/test-table-name.R b/tests/testthat/test-table-name.R index 9cbd513c7..c09155f71 100644 --- a/tests/testthat/test-table-name.R +++ b/tests/testthat/test-table-name.R @@ -57,11 +57,21 @@ test_that("can coerce all user facing inputs", { id <- in_catalog("foo", "bar", "baz") expect_true(is_table_id(id)) expect_equal(as_table_name(id, con), table_name("`foo`.`bar`.`baz`")) + + id <- in_catalog("foo", sql("bar"), ident_q("baz")) + expect_true(is_table_id(id)) + expect_equal(as_table_name(id, con), table_name("`foo`.bar.baz")) }) test_that("strips names", { con <- simulate_dbi() expect_equal(as_table_name(c(x = "x"), con), table_name("`x`")) + + id <- in_schema(c(x = "a"), "b") + expect_equal(as_table_name(id, con), table_name("`a`.`b`")) + + id <- in_catalog("a", "b", c(x = "c")) + expect_equal(as_table_name(id, con), table_name("`a`.`b`.`c`")) }) test_that("as_table_name validates its inputs", { From 09e78e755266c73079c1963443151ac3aed25bb7 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Thu, 15 Feb 2024 07:18:57 -0600 Subject: [PATCH 47/51] Uncomment accidentally commented tests --- tests/testthat/test-verb-joins.R | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/tests/testthat/test-verb-joins.R b/tests/testthat/test-verb-joins.R index e8e833a88..d55760ded 100644 --- a/tests/testthat/test-verb-joins.R +++ b/tests/testthat/test-verb-joins.R @@ -125,11 +125,11 @@ test_that("alias truncates long table names at database limit", { # joins correctly work self_join2 <- left_join(mf1, mf1, by = c("x", "y")) -# -# expect_equal( -# self_join2 %>% collect(), -# tibble(x = 1:3, y = "a") -# ) + + expect_equal( + self_join2 %>% collect(), + tibble(x = 1:3, y = "a") + ) expect_snapshot( self_join2 %>% remote_query() @@ -155,10 +155,10 @@ test_that("alias truncates long table names at database limit", { self_join3 <- left_join(mf1, mf1, by = c("x", "y")) %>% inner_join(mf2, by = "x") - # expect_equal( - # self_join3 %>% collect(), - # tibble(x = 2:3, y.x = "a", y.y = "b") - # ) + expect_equal( + self_join3 %>% collect(), + tibble(x = 2:3, y.x = "a", y.y = "b") + ) expect_snapshot( self_join3 %>% remote_query() ) From 2cc65f38282c769d08f88050a7bf6c86042dfcfb Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Thu, 15 Feb 2024 07:21:18 -0600 Subject: [PATCH 48/51] Qualify is --- R/simulate.R | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/R/simulate.R b/R/simulate.R index d98635633..04a950c94 100644 --- a/R/simulate.R +++ b/R/simulate.R @@ -32,7 +32,7 @@ sql_escape_ident.DBIConnection <- function(con, x) { } #' @export sql_escape_ident.TestConnection <- function(con, x) { - if (is(x, "SQL")) { + if (methods::is(x, "SQL")) { x } else { sql_quote(x, "`") From c28dd749546e803791901f829e5a3ab6b3a6e500 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Thu, 15 Feb 2024 07:22:58 -0600 Subject: [PATCH 49/51] Add bullet about index name --- NEWS.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/NEWS.md b/NEWS.md index c5a3c5fc5..c89c51281 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,5 +1,8 @@ # dbplyr (development version) +* 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. + * Namespaced dplyr calls now error if the function doesn't exist, or a translation is not available (#1426). From c88c3557e7c4c5f3f297d0f9bc11f2fbd2ae3566 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Thu, 15 Feb 2024 08:02:57 -0600 Subject: [PATCH 50/51] Rename table_name to table_path --- NAMESPACE | 10 +-- R/backend-mysql.R | 2 +- R/backend-postgres-old.R | 2 +- R/backend-postgres.R | 2 +- R/build-sql.R | 2 +- R/db-io.R | 6 +- R/db-sql.R | 24 +++--- R/lazy-join-query.R | 4 +- R/lazy-ops.R | 2 +- R/query-join.R | 18 ++-- R/query-set-op.R | 2 +- R/remote.R | 2 +- R/sql-build.R | 4 +- R/table-name.R | 95 +++++++++++----------- R/tbl-lazy.R | 2 +- R/utils.R | 6 +- R/verb-compute.R | 2 +- R/verb-copy-to.R | 2 +- R/verb-joins.R | 6 +- tests/testthat/_snaps/backend-.md | 4 +- tests/testthat/_snaps/backend-mssql.md | 2 +- tests/testthat/_snaps/lazy-select-query.md | 2 +- tests/testthat/_snaps/query-join.md | 6 +- tests/testthat/_snaps/query-select.md | 2 +- tests/testthat/_snaps/query-semi-join.md | 4 +- tests/testthat/_snaps/query-set-op.md | 6 +- tests/testthat/_snaps/table-name.md | 28 +++---- tests/testthat/test-backend-mssql.R | 8 +- tests/testthat/test-query-join.R | 8 +- tests/testthat/test-query-select.R | 2 +- tests/testthat/test-remote.R | 12 +-- tests/testthat/test-table-name.R | 60 +++++++------- tests/testthat/test-tbl-lazy.R | 2 +- tests/testthat/test-verb-joins.R | 62 +++++++------- 34 files changed, 201 insertions(+), 200 deletions(-) diff --git a/NAMESPACE b/NAMESPACE index d7a2d697a..ce1b1dcf3 100644 --- a/NAMESPACE +++ b/NAMESPACE @@ -1,8 +1,8 @@ # Generated by roxygen2: do not edit by hand S3method("$",tbl_lazy) -S3method("[",dbplyr_table_name) -S3method("[[",dbplyr_table_name) +S3method("[",dbplyr_table_path) +S3method("[[",dbplyr_table_path) S3method(add_count,tbl_lazy) S3method(anti_join,tbl_lazy) S3method(arrange,tbl_lazy) @@ -15,7 +15,7 @@ S3method(as.sql,dbplyr_schema) S3method(as.sql,ident) S3method(as.sql,sql) S3method(auto_copy,tbl_sql) -S3method(c,dbplyr_table_name) +S3method(c,dbplyr_table_path) S3method(c,ident) S3method(c,sql) S3method(collapse,tbl_sql) @@ -117,7 +117,7 @@ S3method(escape,character) S3method(escape,data.frame) S3method(escape,dbplyr_catalog) S3method(escape,dbplyr_schema) -S3method(escape,dbplyr_table_name) +S3method(escape,dbplyr_table_path) S3method(escape,double) S3method(escape,factor) S3method(escape,ident) @@ -175,7 +175,7 @@ S3method(print,base_query) S3method(print,dbplyr_catalog) S3method(print,dbplyr_schema) S3method(print,dbplyr_sql_options) -S3method(print,dbplyr_table_name) +S3method(print,dbplyr_table_path) S3method(print,ident) S3method(print,join_query) S3method(print,lazy_base_local_query) diff --git a/R/backend-mysql.R b/R/backend-mysql.R index 8e7aad7a3..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_name(table, con, 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 885b92517..e79379a3e 100644 --- a/R/backend-postgres-old.R +++ b/R/backend-postgres-old.R @@ -26,7 +26,7 @@ db_write_table.PostgreSQLConnection <- function(con, # the bare table name dbWriteTable( con, - name = table_name_table(table, con), + name = table_name(table, con), value = values, field.types = types, ..., diff --git a/R/backend-postgres.R b/R/backend-postgres.R index b2fe756ce..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_name(table, con, 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/build-sql.R b/R/build-sql.R index 9f709dace..4c3de18b9 100644 --- a/R/build-sql.R +++ b/R/build-sql.R @@ -128,7 +128,7 @@ sql_quote_transformer <- function(connection) { glue_check_collapse(type, should_collapse) if (type == "tbl") { - value <- as_table_name(value, connection) + value <- as_table_path(value, connection) } else if (type == "from") { value <- as_table_source(value, connection) } else if (type == "col") { diff --git a/R/db-io.R b/R/db-io.R index fffcdb991..b22ccd494 100644 --- a/R/db-io.R +++ b/R/db-io.R @@ -65,7 +65,7 @@ db_copy_to.DBIConnection <- function(con, indexes = NULL, analyze = TRUE, in_transaction = TRUE) { - table <- as_table_name(table, con) + table <- as_table_path(table, con) new <- db_table_temporary(con, table, temporary) table <- new$table temporary <- new$temporary @@ -124,7 +124,7 @@ db_compute.DBIConnection <- function(con, indexes = list(), analyze = TRUE, in_transaction = FALSE) { - table <- as_table_name(table, con) + table <- as_table_path(table, con) new <- db_table_temporary(con, table, temporary) table <- new$table temporary <- new$temporary @@ -180,7 +180,7 @@ db_write_table.DBIConnection <- function(con, temporary = TRUE, ..., overwrite = FALSE) { - check_table_name(table) + check_table_path(table) check_character(types, allow_null = TRUE) check_named(types) check_bool(temporary) diff --git a/R/db-sql.R b/R/db-sql.R index 22e66f510..b8f934d46 100644 --- a/R/db-sql.R +++ b/R/db-sql.R @@ -173,10 +173,10 @@ sql_table_index.DBIConnection <- function(con, unique = FALSE, ..., call = caller_env()) { - table <- as_table_name(table, con) + table <- as_table_path(table, con) if (is.null(name)) { - table_name <- table_name_table(table, con) + table_name <- table_name(table, con) name <- name %||% paste0(c(table_name, columns), collapse = "_") } glue_sql2( @@ -225,7 +225,7 @@ sql_query_save <- function(con, sql, name, temporary = TRUE, ...) { } #' @export sql_query_save.DBIConnection <- function(con, sql, name, temporary = TRUE, ...) { - name <- as_table_name(name, con) + name <- as_table_path(name, con) glue_sql2( con, @@ -257,10 +257,10 @@ sql_query_wrap.DBIConnection <- function(con, from, name = NULL, ..., lvl = 0) { # some backends, e.g. Postgres, require an alias for a subquery name <- name %||% unique_subquery_name() glue_sql2(con, "{from}", as_sql, "{.tbl name}") - } else { # must be a table_name + } else { # must be a table_path if (!is.null(name)) { - table <- table_name_table(name, con) - names(from) <- as_table_name(table, con) + table <- table_name(name, con) + names(from) <- as_table_path(table, con) } from } @@ -792,7 +792,7 @@ sql_query_insert.DBIConnection <- function(con, conflict = c("error", "ignore"), returning_cols = NULL, method = NULL) { - table <- as_table_name(table, con) + table <- as_table_path(table, con) from <- as_table_source(from, con) method <- method %||% "where_not_exists" @@ -856,7 +856,7 @@ sql_query_append.DBIConnection <- function(con, insert_cols, ..., returning_cols = NULL) { - table <- as_table_name(table, con) + table <- as_table_path(table, con) from <- as_table_source(from, con) # https://stackoverflow.com/questions/25969/insert-into-values-select-from @@ -901,7 +901,7 @@ sql_query_update_from.DBIConnection <- function(con, update_values, ..., returning_cols = NULL) { - table <- as_table_name(table, con) + 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 @@ -951,7 +951,7 @@ sql_query_upsert.DBIConnection <- function(con, ..., returning_cols = NULL, method = NULL) { - table <- as_table_name(table, con) + table <- as_table_path(table, con) from <- as_table_source(from, con) method <- method %||% "cte_update" @@ -1016,7 +1016,7 @@ sql_query_delete.DBIConnection <- function(con, by, ..., returning_cols = NULL) { - table <- as_table_name(table, con) + table <- as_table_path(table, con) from <- as_table_source(from, con) parts <- rows_prep(con, table, from, by, lvl = 1) @@ -1121,7 +1121,7 @@ db_save_query.DBIConnection <- function(con, temporary = TRUE, ..., overwrite = FALSE) { - name <- as_table_name(name, con) + name <- as_table_path(name, con) sql <- sql_query_save(con, sql, name, temporary = temporary, ...) if (overwrite) { diff --git a/R/lazy-join-query.R b/R/lazy-join-query.R index 00c7c89a1..03209a7bd 100644 --- a/R/lazy-join-query.R +++ b/R/lazy-join-query.R @@ -188,7 +188,7 @@ sql_build.lazy_multi_join_query <- function(op, con, ..., sql_options = NULL) { } generate_join_table_names <- function(table_names, con) { - names <- table_name_table(table_names$name, con) + names <- table_name(table_names$name, con) table_name_length_max <- max(nchar(names)) if (length(table_names$name) != 2) { @@ -216,7 +216,7 @@ generate_join_table_names <- function(table_names, con) { method = "both.sides" ) - as_table_names(abbr_names, con) + as_table_paths(abbr_names, con) } #' @export diff --git a/R/lazy-ops.R b/R/lazy-ops.R index 1bc797ea5..e2fa8635e 100644 --- a/R/lazy-ops.R +++ b/R/lazy-ops.R @@ -45,7 +45,7 @@ base_query <- function(from) { #' @export print.lazy_base_remote_query <- function(x, ...) { - if (is_table_name(x$x)) { + if (is_table_path(x$x)) { cat_line("From: ", format(x$x)) } else { cat_line("From: ") diff --git a/R/query-join.R b/R/query-join.R index 22739fb61..733781148 100644 --- a/R/query-join.R +++ b/R/query-join.R @@ -127,7 +127,7 @@ flatten_query.multi_join_query <- function(qry, query_list, con) { } # TODO reuse query - name <- as_table_name(unique_subquery_name(), con) + 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,11 +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 <- table_name(names(table_vars)) + table_paths <- table_path(names(table_vars)) 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] @@ -194,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,8 +250,8 @@ sql_rf_join_vars <- function(con, qualify_all_columns) { type <- arg_match0(type, c("right", "full")) - check_table_name(x_as) - check_table_name(y_as) + check_table_path(x_as) + check_table_path(y_as) table_names <- c(x_as, y_as) if (type == "full") { @@ -347,8 +347,8 @@ sql_qualify_var <- function(con, table, var) { var <- sql_escape_ident(con, var) if (!is.null(table)) { - table <- table_name_table(table, con) - table <- as_table_names(table, con) + table <- table_name(table, con) + table <- as_table_paths(table, con) sql(paste0(table, ".", var)) } else { diff --git a/R/query-set-op.R b/R/query-set-op.R index 5571a03ee..7758934f5 100644 --- a/R/query-set-op.R +++ b/R/query-set-op.R @@ -117,7 +117,7 @@ flatten_query.union_query <- function(qry, query_list, con) { } # TODO reuse query - name <- as_table_name(unique_subquery_name(), con) + 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 b303ccc04..41a977c8f 100644 --- a/R/remote.R +++ b/R/remote.R @@ -40,7 +40,7 @@ remote_name <- function(x, null_if_local = TRUE) { if (is.null(con)) { table } else { - table_name_table(table, con) + table_name(table, con) } } } diff --git a/R/sql-build.R b/R/sql-build.R index cd4d4b0f7..af37cce0b 100644 --- a/R/sql-build.R +++ b/R/sql-build.R @@ -144,7 +144,7 @@ cte_render <- function(query_list, con) { ctes <- purrr::imap( query_list[-n], function(query, name) { - name <- table_name(name) + name <- table_path(name) glue_sql2(con, "{.name name} {.kw 'AS'} (\n{query}\n)") } ) @@ -169,7 +169,7 @@ querylist_reuse_query <- function(qry, query_list, con) { if (!is.na(id)) { query_list$name <- names(query_list$queries)[[id]] } else { - name <- as_table_name(unique_subquery_name(), con) + 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 diff --git a/R/table-name.R b/R/table-name.R index b7376a40e..d968257ea 100644 --- a/R/table-name.R +++ b/R/table-name.R @@ -5,48 +5,49 @@ # * always refers to exactly one table # * but all converted to table name ASAP -# table name -# * escaped string +# 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 of multiple names -# * object names are always assumed to be table names +# * can be vector containing multiple names +# * object names are always assumed to be table paths -# table_name -------------------------------------------------------------- +# table_path -------------------------------------------------------------- -table_name <- function(x) { +table_path <- function(x) { check_character(x) x <- unname(x) - structure(x, class = c("dbplyr_table_name", "character")) + structure(x, class = c("dbplyr_table_path", "character")) } -# So you can do SQL(table_name("foo")) -setOldClass(c("dbplyr_table_name", "character")) +# So you can do SQL(table_path("foo")) +setOldClass(c("dbplyr_table_path", "character")) -is_table_name <- function(x) { - inherits(x, "dbplyr_table_name") +is_table_path <- function(x) { + inherits(x, "dbplyr_table_path") } #' @export -print.dbplyr_table_name <- function(x, ...) { - cat(" ", paste0(style_kw(x), collapse = ", "), "\n", sep = "") +print.dbplyr_table_path <- function(x, ...) { + cat(" ", paste0(style_kw(x), collapse = ", "), "\n", sep = "") } #' @export -`[.dbplyr_table_name` <- function(x, ...) { - table_name(NextMethod()) +`[.dbplyr_table_path` <- function(x, ...) { + table_path(NextMethod()) } #' @export -`[[.dbplyr_table_name` <- function(x, ...) { - table_name(NextMethod()) +`[[.dbplyr_table_path` <- function(x, ...) { + table_path(NextMethod()) } #' @export -`c.dbplyr_table_name` <- function(x, ...) { - table_name(NextMethod()) +`c.dbplyr_table_path` <- function(x, ...) { + table_path(NextMethod()) } -make_table_name <- function(x, con, collapse = TRUE) { - needs_quote <- !vapply(x, name_is_escaped, logical(1)) +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]) @@ -54,29 +55,29 @@ make_table_name <- function(x, con, collapse = TRUE) { x <- paste0(x, collapse = ".") } - table_name(x) + table_path(x) } -name_is_escaped <- function(x) { +component_is_escaped <- function(x) { inherits(x, "AsIs") || is.sql(x) || inherits(x, "ident_q") } -as_table_names <- function(x, con) { - make_table_name(x, con, collapse = FALSE) +as_table_paths <- function(x, con) { + make_table_path(x, con, collapse = FALSE) } # Extract just the table name from a full identifier -table_name_table <- function(x, con) { - x <- as_table_name(x, con) +table_name <- function(x, con) { + x <- as_table_path(x, con) vapply(x, FUN.VALUE = character(1), function(x) { if (x == "") return("") - out <- table_name_components(x, con) + out <- table_path_components(x, con) out[[length(out)]] }) } -table_name_components <- function(x, con) { +table_path_components <- function(x, con) { quote_char <- substr(sql_escape_ident(con, ""), 1, 1) scan( @@ -90,11 +91,11 @@ table_name_components <- function(x, con) { } #' @export -escape.dbplyr_table_name <- function(x, parens = FALSE, collapse = ", ", con = NULL) { +escape.dbplyr_table_path <- function(x, parens = FALSE, collapse = ", ", con = NULL) { # names are always already escaped alias <- names2(x) - table_name <- as_table_name(table_name_table(x, con), con) - has_alias <- alias == "" | alias == table_name + 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 ") @@ -115,7 +116,7 @@ check_table_id <- function(x, arg = caller_arg(x), call = caller_env()) { } is_table_id <- function(x) { - is_table_name(x) || + is_table_path(x) || is.ident(x) || methods::is(x, "Id") || is_catalog(x) || @@ -123,25 +124,25 @@ is_table_id <- function(x) { is.character(x) } -check_table_name <- function(x, +check_table_path <- function(x, error_arg = caller_arg(x), error_call = caller_env()) { - if (!is_table_name(x)) { + if (!is_table_path(x)) { cli::cli_abort( - "{.arg {error_arg}} must be a , not {.obj_type_friendly x}.", + "{.arg {error_arg}} must be a , not {.obj_type_friendly x}.", call = error_call, .internal = TRUE ) } } -as_table_name <- function(x, +as_table_path <- function(x, con, error_arg = caller_arg(x), error_call = caller_env()) { check_required(con) - if (is_table_name(x)) { + if (is_table_path(x)) { x } else if (is.sql(x)) { cli::cli_warn( @@ -150,23 +151,23 @@ as_table_name <- function(x, i = "If you want to use a literal (unquoted) identifier use {.fn I} instead." ) ) - table_name(unclass(x)) + table_path(unclass(x)) } else if (inherits(x, "ident_q")) { - table_name(paste0(x, collapse = ".")) + table_path(paste0(x, collapse = ".")) } else if (is.ident(x)) { - make_table_name(unclass(x), con) + make_table_path(unclass(x), con) } else if (methods::is(x, "Id")) { - make_table_name(x@name, con) + make_table_path(x@name, con) } else if (inherits(x, "dbplyr_catalog")) { - make_table_name(list(x$catalog, x$schema, x$table), con) + make_table_path(list(x$catalog, x$schema, x$table), con) } else if (inherits(x, "dbplyr_schema")) { - make_table_name(list(x$schema, x$table), con) + 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_name(unclass(x)) + table_path(unclass(x)) } else if (is.character(x)) { check_string(x, allow_empty = FALSE, arg = error_arg, call = error_call) - make_table_name(x, con, collapse = FALSE) + make_table_path(x, con, collapse = FALSE) } else { cli::cli_abort( "{.arg {error_arg}} uses unknown specification for table name", @@ -182,7 +183,7 @@ as_table_source <- function(x, con, ..., error_arg = caller_arg(x), error_call = if (is.sql(x)) { x } else if (is_table_id(x)) { - as_table_name(x, con = con, error_arg = error_arg, error_call = error_call) + 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) } diff --git a/R/tbl-lazy.R b/R/tbl-lazy.R index da9f717d3..8a78d36c2 100644 --- a/R/tbl-lazy.R +++ b/R/tbl-lazy.R @@ -17,7 +17,7 @@ tbl_lazy <- function(df, con = NULL, ..., name = "df") { con <- con %||% sql_current_con() %||% simulate_dbi() subclass <- class(con)[[1]] - name <- as_table_name(name, con) + name <- as_table_path(name, con) dplyr::make_tbl( purrr::compact(c(subclass, "lazy")), diff --git a/R/utils.R b/R/utils.R index 931ebde15..21f50bea6 100644 --- a/R/utils.R +++ b/R/utils.R @@ -83,13 +83,13 @@ res_warn_incomplete <- function(res, hint = "n = -1") { } add_temporary_prefix <- function(con, table, temporary = TRUE) { - check_table_name(table) + check_table_path(table) if (!temporary) { return(table) } - pieces <- table_name_components(table, con) + pieces <- table_path_components(table, con) table_name <- pieces[length(pieces)] if (substr(table_name, 1, 1) != "#") { @@ -99,7 +99,7 @@ add_temporary_prefix <- function(con, table, temporary = TRUE) { class = c("dbplyr_message_temp_table", "dbplyr_message") ) pieces[[length(pieces)]] <- new_name - table <- make_table_name(pieces, con) + table <- make_table_path(pieces, con) } table diff --git a/R/verb-compute.R b/R/verb-compute.R index 5b9e40898..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_name(name, x$src$con) + 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 02072bc64..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_name(name, dest$con) + name <- as_table_path(name, dest$con) if (inherits(df, "tbl_sql") && same_src(df$src, dest)) { out <- compute(df, diff --git a/R/verb-joins.R b/R/verb-joins.R index 1e4522c42..82d72164e 100644 --- a/R/verb-joins.R +++ b/R/verb-joins.R @@ -747,8 +747,8 @@ make_join_aliases <- function(con, x_as, y_as, sql_on, call) { } list( - x = if (!is.null(x_as)) as_table_name(x_as, con), - y = if (!is.null(y_as)) as_table_name(y_as, con) + x = if (!is.null(x_as)) as_table_path(x_as, con), + y = if (!is.null(y_as)) as_table_path(y_as, con) ) } @@ -760,7 +760,7 @@ make_table_names <- function(as, lq) { } else if (!is.null(name)) { tibble(name = name, from = "name") } else { - tibble(name = table_name(""), from = "") + tibble(name = table_path(""), from = "") } } diff --git a/tests/testthat/_snaps/backend-.md b/tests/testthat/_snaps/backend-.md index 1fffbe4d9..bb5a82e48 100644 --- a/tests/testthat/_snaps/backend-.md +++ b/tests/testthat/_snaps/backend-.md @@ -68,14 +68,14 @@ Code sql_query_wrap(con, ident("table")) Output - `table` + `table` --- Code sql_query_wrap(con, in_schema("schema", "tbl")) Output - `schema`.`tbl` + `schema`.`tbl` --- diff --git a/tests/testthat/_snaps/backend-mssql.md b/tests/testthat/_snaps/backend-mssql.md index 98ab650dc..f17f8175c 100644 --- a/tests/testthat/_snaps/backend-mssql.md +++ b/tests/testthat/_snaps/backend-mssql.md @@ -470,7 +470,7 @@ # add prefix to temporary table Code - out <- db_table_temporary(con, table_name("foo.bar"), temporary = TRUE) + out <- db_table_temporary(con, table_path("foo.bar"), temporary = TRUE) Message Created a temporary table named #bar diff --git a/tests/testthat/_snaps/lazy-select-query.md b/tests/testthat/_snaps/lazy-select-query.md index 0e6bc1496..95f1e91c4 100644 --- a/tests/testthat/_snaps/lazy-select-query.md +++ b/tests/testthat/_snaps/lazy-select-query.md @@ -6,7 +6,7 @@ Output From: - `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 7a462c335..631506fbc 100644 --- a/tests/testthat/_snaps/query-join.md +++ b/tests/testthat/_snaps/query-join.md @@ -5,17 +5,17 @@ Output X: - `lf1` + `lf1` Type: left By: x-x Y: - `lf2` + `lf2` Type: left By: x-x Y: - `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 4edc839a8..a60a2ede3 100644 --- a/tests/testthat/_snaps/query-select.md +++ b/tests/testthat/_snaps/query-select.md @@ -5,7 +5,7 @@ Output From: - `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 75343445e..dfab9f2c6 100644 --- a/tests/testthat/_snaps/query-semi-join.md +++ b/tests/testthat/_snaps/query-semi-join.md @@ -11,9 +11,9 @@ Where: "`df_RHS`.`z` = 2.0" X: - `df` + `df` Y: - `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 b2334a570..643388822 100644 --- a/tests/testthat/_snaps/query-set-op.md +++ b/tests/testthat/_snaps/query-set-op.md @@ -5,21 +5,21 @@ Output From: - `lf1` + `lf1` Select: `lf1`.*, NULL UNION From: - `lf2` + `lf2` Select: `x`, NULL, `z` UNION ALL From: - `lf3` + `lf3` Select: `x`, NULL, `z` # generated sql doesn't change unexpectedly diff --git a/tests/testthat/_snaps/table-name.md b/tests/testthat/_snaps/table-name.md index f703522e4..f6e0ba6ae 100644 --- a/tests/testthat/_snaps/table-name.md +++ b/tests/testthat/_snaps/table-name.md @@ -1,10 +1,10 @@ -# table_name possess key methods +# table_path possess key methods Code - name <- table_name(c("x", "y", "z")) + name <- table_path(c("x", "y", "z")) name Output - x, y, z + x, y, z # can check for table name @@ -12,41 +12,41 @@ foo(1) Condition Error in `foo()`: - ! `y` must be a , not a string. + ! `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_name validates its inputs +# as_table_path validates its inputs Code - as_table_name("x") + as_table_path("x") Condition - Error in `as_table_name()`: + Error in `as_table_path()`: ! `con` is absent but must be supplied. Code - as_table_name(c("x", "y"), con) + as_table_path(c("x", "y"), con) Condition Error: ! `c("x", "y")` must be a single string, not a character vector. Code - as_table_name(1, con) + as_table_path(1, con) Condition - Error in `as_table_name()`: + Error in `as_table_path()`: ! `1` uses unknown specification for table name Code - as_table_name(I(1), con) + as_table_path(I(1), con) Condition Error: ! `I(1)` must be a single string, not the number 1. -# as_table_name warns when using sql +# as_table_path warns when using sql Code - as_table_name(sql("x"), con) + 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 + x diff --git a/tests/testthat/test-backend-mssql.R b/tests/testthat/test-backend-mssql.R index dea21af51..dc875f279 100644 --- a/tests/testthat/test-backend-mssql.R +++ b/tests/testthat/test-backend-mssql.R @@ -392,14 +392,14 @@ test_that("can copy_to() and compute() with temporary tables (#438)", { test_that("add prefix to temporary table", { con <- simulate_mssql() expect_snapshot( - out <- db_table_temporary(con, table_name("foo.bar"), temporary = TRUE) + out <- db_table_temporary(con, table_path("foo.bar"), temporary = TRUE) ) - expect_equal(out, list(table = table_name("`foo`.`#bar`"), temporary = FALSE)) + expect_equal(out, list(table = table_path("`foo`.`#bar`"), temporary = FALSE)) expect_silent( - out <- db_table_temporary(con, table_name("foo.#bar"), temporary = TRUE) + out <- db_table_temporary(con, table_path("foo.#bar"), temporary = TRUE) ) - expect_equal(out, list(table = table_name("foo.#bar"), temporary = FALSE)) + expect_equal(out, list(table = table_path("foo.#bar"), temporary = FALSE)) }) test_that("bit conversion works for important cases", { diff --git a/tests/testthat/test-query-join.R b/tests/testthat/test-query-join.R index 38d5c3e3e..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 = table_name("left"), - y_as = table_name("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 = table_name("left"), - y_as = table_name("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 4bfc20e69..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(table_name("`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 03a94b409..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(), - table_name("`refxiudlph`") + table_path("`refxiudlph`") ) # produces name after unarranging expect_equal( mf %>% arrange(x) %>% arrange() %>% remote_table(), - table_name("`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), table_name("`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,12 +34,12 @@ 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), table_name("`df`")) - expect_equal(lf %>% group_by(x) %>% remote_table(null_if_local = FALSE), table_name("`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_name(x, simulate_sqlite())) { + 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), "tbl") diff --git a/tests/testthat/test-table-name.R b/tests/testthat/test-table-name.R index c09155f71..ae1a73868 100644 --- a/tests/testthat/test-table-name.R +++ b/tests/testthat/test-table-name.R @@ -1,90 +1,90 @@ -test_that("table_name possess key methods", { +test_that("table_path possess key methods", { expect_snapshot({ - name <- table_name(c("x", "y", "z")) + name <- table_path(c("x", "y", "z")) name }) - expect_equal(name[c(1, 3)], table_name(c("x", "z"))) - expect_equal(name[[2]], table_name("y")) - expect_equal(c(name[[1]], name[[2]]), table_name(c("x", "y"))) + 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_name(y) + foo <- function(y) check_table_path(y) expect_snapshot(foo(1), error = TRUE) }) -# as_table_name ----------------------------------------------------------- +# as_table_path ----------------------------------------------------------- test_that("can coerce all user facing inputs", { con <- simulate_dbi() - x_esc <- table_name("`x`") - x_raw <- table_name("x") + x_esc <- table_path("`x`") + x_raw <- table_path("x") - id <- table_name("x") + id <- table_path("x") expect_true(is_table_id(id)) - expect_equal(as_table_name(id, con), x_raw) + expect_equal(as_table_path(id, con), x_raw) id <- "x" expect_true(is_table_id(id)) - expect_equal(as_table_name(id, con), x_esc) + expect_equal(as_table_path(id, con), x_esc) id <- I("x") expect_true(is_table_id(id)) - expect_equal(as_table_name(id, con), x_raw) + expect_equal(as_table_path(id, con), x_raw) id <- ident("x") expect_true(is_table_id(id)) - expect_equal(as_table_name(id, con), x_esc) + expect_equal(as_table_path(id, con), x_esc) id <- ident_q("x") expect_true(is_table_id(id)) - expect_equal(as_table_name(id, con), x_raw) + 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_name(id, con), table_name("`foo`.`bar`")) + 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_name(id, con), table_name("`foo`.`bar`")) + 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_name(id, con), table_name("`foo`.`bar`")) + 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_name(id, con), table_name("`foo`.`bar`.`baz`")) + 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_name(id, con), table_name("`foo`.bar.baz")) + expect_equal(as_table_path(id, con), table_path("`foo`.bar.baz")) }) test_that("strips names", { con <- simulate_dbi() - expect_equal(as_table_name(c(x = "x"), con), table_name("`x`")) + expect_equal(as_table_path(c(x = "x"), con), table_path("`x`")) id <- in_schema(c(x = "a"), "b") - expect_equal(as_table_name(id, con), table_name("`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_name(id, con), table_name("`a`.`b`.`c`")) + expect_equal(as_table_path(id, con), table_path("`a`.`b`.`c`")) }) -test_that("as_table_name validates its inputs", { +test_that("as_table_path validates its inputs", { con <- simulate_dbi() expect_snapshot(error = TRUE, { - as_table_name("x") - as_table_name(c("x", "y"), con) - as_table_name(1, con) - as_table_name(I(1), con) + 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_name warns when using sql", { +test_that("as_table_path warns when using sql", { con <- simulate_dbi() - expect_snapshot(as_table_name(sql("x"), con)) + 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 eafa6d3af..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(table_name("`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-verb-joins.R b/tests/testthat/test-verb-joins.R index d55760ded..7a17172ff 100644 --- a/tests/testthat/test-verb-joins.R +++ b/tests/testthat/test-verb-joins.R @@ -92,7 +92,7 @@ test_that("join works with in_schema", { ) out <- left_join(df4, df5, by = "x") expect_equal(out$lazy_query$table_names, tibble( - name = table_name(c("foo.df", "foo2.df")), + name = table_path(c("foo.df", "foo2.df")), from = c("name", "name") )) }) @@ -102,10 +102,10 @@ 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 <- table_name(paste0("a", paste0(0:61 %% 10, collapse = ""))) + nm1 <- table_path(paste0("a", paste0(0:61 %% 10, collapse = ""))) mf1 <- local_db_table(con, tibble(x = 1:3, y = "a"), unclass(nm1)) - nm2 <- table_name(paste0("b", paste0(0:61 %% 10, collapse = ""))) + 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 @@ -290,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, table_name(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, table_name(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, table_name(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, table_name(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, table_name(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, table_name(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, table_name(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, table_name(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, table_name(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, table_name(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, table_name(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, table_name(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, table_name(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, table_name(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", { @@ -626,7 +626,7 @@ test_that("multiple joins create a single query", { lq <- out$lazy_query expect_s3_class(lq, "lazy_multi_join_query") expect_equal(lq$table_names, tibble( - name = table_name(c("`df1`", "`df2`", "`df3`")), + name = table_path(c("`df1`", "`df2`", "`df3`")), from = "name" )) expect_equal(lq$vars$name, c("x", "a", "b.x", "b.y")) @@ -729,7 +729,7 @@ test_that("multi joins work with x_as", { expect_s3_class(lq, "lazy_multi_join_query") expect_equal( lq$table_names, - tibble(name = table_name(c("`lf1`", "`lf2`", "`lf3`")), from = "as") + tibble(name = table_path(c("`lf1`", "`lf2`", "`lf3`")), from = "as") ) # `x_as` provided twice with the same name -> one query From 27a789fa5428dfb5047ac8a4c8aa1779e78146de Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Thu, 15 Feb 2024 08:08:49 -0600 Subject: [PATCH 51/51] Add news bullets --- NEWS.md | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/NEWS.md b/NEWS.md index c89c51281..d51e8f8c5 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,5 +1,24 @@ # 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.