diff --git a/DESCRIPTION b/DESCRIPTION index c2bd6fd55f..700f583176 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -118,7 +118,7 @@ VignetteBuilder: Config/autostyle/scope: line_breaks Config/autostyle/strict: true Config/testthat/edition: 3 -Config/testthat/parallel: true +Config/testthat/parallel: false Config/testthat/start-first: zzx-deprecated, flatten, dplyr, filter-dm, draw-dm, bind, rows-dm, learn Encoding: UTF-8 diff --git a/R/db-interface.R b/R/db-interface.R index b928ae3032..414c2180f4 100644 --- a/R/db-interface.R +++ b/R/db-interface.R @@ -5,17 +5,20 @@ #' and a [`dm`] object as its second argument. #' The latter is copied to the former. #' The default is to create temporary tables, set `temporary = FALSE` to create permanent tables. -#' Unless `set_key_constraints` is `FALSE`, primary key constraints are set on all databases, -#' and in addition foreign key constraints are set on MSSQL and Postgres databases. +#' Unless `set_key_constraints` is `FALSE`, primary key, foreign key, and unique constraints +#' are set, and indexes for foreign keys are created, on all databases. #' #' @inheritParams dm_examine_constraints #' #' @param dest An object of class `"src"` or `"DBIConnection"`. #' @param dm A `dm` object. #' @inheritParams rlang::args_dots_empty -#' @param set_key_constraints If `TRUE` will mirror `dm` primary and foreign key constraints on a database -#' and create indexes for foreign key constraints. -#' Set to `FALSE` if your data model currently does not satisfy primary or foreign key constraints. +#' @param set_key_constraints If `TRUE` will mirror +#' the primary, foreign, and unique key constraints +#' and create indexes for foreign key constraints +#' for the primary and foreign keys in the `dm` object. +#' Set to `FALSE` if your data model currently does not satisfy +#' primary or foreign key constraints. #' @param temporary If `TRUE`, only temporary tables will be created. #' These tables will vanish when disconnecting from the database. #' @param schema Name of schema to copy the `dm` to. @@ -93,21 +96,13 @@ copy_dm_to <- function( progress = NA, unique_table_names = NULL, copy_to = NULL) { - # for the time being, we will be focusing on MSSQL - # we want to - # 1. change `dm_get_src_impl(dm)` to `dest` - # 2. copy the tables to `dest` - # 3. implement the key situation within our `dm` on the DB + # if (!is.null(unique_table_names)) { - deprecate_warn( + deprecate_stop( "0.1.4", "dm::copy_dm_to(unique_table_names = )", - details = "Use `table_names = identity` to use unchanged names for temporary tables." + details = "Use `table_names = set_names(names(dm))` to use unchanged names for temporary tables." ) - - if (is.null(table_names) && temporary && !unique_table_names) { - table_names <- identity - } } if (!is.null(copy_to)) { @@ -124,113 +119,116 @@ copy_dm_to <- function( check_suggested("dbplyr", "copy_dm_to") dest <- src_from_src_or_con(dest) - src_names <- src_tbls_impl(dm) - - if (is_db(dest)) { - dest_con <- con_from_src_or_con(dest) - - # in case `table_names` was chosen by the user, check if the input makes sense: - # 1. is there one name per dm-table? - # 2. are there any duplicated table names? - # 3. is it a named character or ident_q vector with the correct names? - if (is.null(table_names)) { - table_names_out <- repair_table_names_for_db(src_names, temporary, dest_con, schema) - # https://github.com/tidyverse/dbplyr/issues/487 - if (is_mssql(dest)) { - temporary <- FALSE - } - } else { - if (!is.null(schema)) abort_one_of_schema_table_names() - if (is_function(table_names) || is_bare_formula(table_names)) { - table_name_fun <- as_function(table_names) - table_names_out <- set_names(table_name_fun(src_names), src_names) - } else { - table_names_out <- table_names - } - check_naming(names(table_names_out), src_names) - if (anyDuplicated(table_names_out)) { - problem <- table_names_out[duplicated(table_names_out)][[1]] - abort_copy_dm_to_table_names_duplicated(problem) - } - - names(table_names_out) <- src_names - } - } else { - # FIXME: Other data sources than local and database possible - deprecate_warn( - "0.1.6", "dm::copy_dm_to(dest = 'must refer to a remote data source')", + if (!is_db(dest)) { + deprecate_stop( + "0.1.6", "dm::copy_dm_to(dest = 'must refer to a DBI connection')", "dm::collect.dm()" ) - table_names_out <- set_names(src_names) } - # FIXME: if same_src(), can use compute() but need to set NOT NULL and other - # constraints + src_names <- src_tbls_impl(dm) + dest_con <- con_from_src_or_con(dest) + + # in case `table_names` was chosen by the user, check if the input makes sense: + # 1. is there one name per dm-table? + # 2. are there any duplicated table names? + # 3. is it a named character or ident_q vector with the correct names? + if (is.null(table_names)) { + table_names_out <- repair_table_names_for_db(src_names, temporary, dest_con, schema) + # https://github.com/tidyverse/dbplyr/issues/487 + if (is_mssql(dest)) { + temporary <- FALSE + } + } else { + if (!is.null(schema)) abort_one_of_schema_table_names() + if (is_function(table_names) || is_bare_formula(table_names)) { + table_name_fun <- as_function(table_names) + table_names_out <- set_names(table_name_fun(src_names), src_names) + } else { + table_names_out <- table_names + } + check_naming(names(table_names_out), src_names) - # Shortcut necessary to avoid copying into .GlobalEnv - if (!is_db(dest)) { - return(dm) + if (anyDuplicated(table_names_out)) { + problem <- table_names_out[duplicated(table_names_out)][[1]] + abort_copy_dm_to_table_names_duplicated(problem) + } + + names(table_names_out) <- src_names } # Must be done here because table types may depend on string length, #2066 dm <- collect(dm, progress = progress) - queries <- build_copy_queries(dest_con, dm, set_key_constraints, temporary, table_names_out) + if (isTRUE(set_key_constraints)) { + dm_for_sql <- dm + } else { + def_no_keys <- dm_get_def(dm) + # FIXME: Add test + def_no_keys$pks[] <- list(new_pk()) + def_no_keys$uks[] <- list(new_uk()) + def_no_keys$fks[] <- list(new_fk()) + # Must keep primary keys + dm_for_sql <- dm_from_def(def_no_keys) + } - ticker_create <- new_ticker( + sql <- dm_sql(dm_for_sql, dest_con, table_names_out, temporary) + + # FIXME: Extract function + # FIXME: Make descriptions part of the dm_sql() output + + pre <- unlist(sql$pre) + load <- unlist(sql$load) + post <- unlist(sql$post) + + ticker_pre <- new_ticker( "creating tables", - n = length(queries$sql_table), + n = length(pre), progress = progress, top_level_fun = "copy_dm_to" ) # create tables - walk(queries$sql_table, ticker_create(~ { + walk(pre, ticker_pre(~ { DBI::dbExecute(dest_con, .x, immediate = TRUE) })) - ticker_populate <- new_ticker( + ticker_load <- new_ticker( "populating tables", - n = length(queries$name), + n = length(load), progress = progress, top_level_fun = "copy_dm_to" ) # populate tables - pwalk( - queries[c("name", "remote_name")], - ticker_populate(~ db_append_table( - con = dest_con, - remote_table = .y, - table = dm[[.x]], - progress = progress, - autoinc = dm_get_all_pks(dm, table = !!.x)$autoincrement - )) - ) + walk(load, ticker_load(~ { + DBI::dbExecute(dest_con, .x, immediate = TRUE) + })) - ticker_index <- new_ticker( + ticker_post <- new_ticker( "creating indexes", - n = sum(lengths(queries$sql_index)), + n = length(post), progress = progress, top_level_fun = "copy_dm_to" ) # create indexes - walk(unlist(queries$sql_index), ticker_index(~ { + walk(post, ticker_post(~ { DBI::dbExecute(dest_con, .x, immediate = TRUE) })) # remote dm is same as source dm with replaced data + # FIXME: Extract function def <- dm_get_def(dm) remote_tables <- map2( table_names_out, map(def$data, colnames), - ~ tbl(dest_con, ..1, vars = ..2) + ~ tbl(dest_con, .x, vars = .y) ) - def$data <- unname(remote_tables[names(dm)]) + def$data <- unname(remote_tables) remote_dm <- dm_from_def(def) invisible(debug_dm_validate(remote_dm)) diff --git a/man/copy_dm_to.Rd b/man/copy_dm_to.Rd index b39894b861..5f57acdab4 100644 --- a/man/copy_dm_to.Rd +++ b/man/copy_dm_to.Rd @@ -24,9 +24,12 @@ copy_dm_to( \item{...}{These dots are for future extensions and must be empty.} -\item{set_key_constraints}{If \code{TRUE} will mirror \code{dm} primary and foreign key constraints on a database -and create indexes for foreign key constraints. -Set to \code{FALSE} if your data model currently does not satisfy primary or foreign key constraints.} +\item{set_key_constraints}{If \code{TRUE} will mirror +the primary, foreign, and unique key constraints +and create indexes for foreign key constraints +for the primary and foreign keys in the \code{dm} object. +Set to \code{FALSE} if your data model currently does not satisfy +primary or foreign key constraints.} \item{table_names}{Desired names for the tables on \code{dest}; the names within the \code{dm} remain unchanged. Can be \code{NULL}, a named character vector, or a vector of \link[DBI:Id]{DBI::Id} objects. @@ -85,8 +88,8 @@ as the input \code{dm}. and a \code{\link{dm}} object as its second argument. The latter is copied to the former. The default is to create temporary tables, set \code{temporary = FALSE} to create permanent tables. -Unless \code{set_key_constraints} is \code{FALSE}, primary key constraints are set on all databases, -and in addition foreign key constraints are set on MSSQL and Postgres databases. +Unless \code{set_key_constraints} is \code{FALSE}, primary key, foreign key, and unique constraints +are set, and indexes for foreign keys are created, on all databases. } \examples{ \dontshow{if (rlang::is_installed(c("RSQLite", "nycflights13", "dbplyr"))) (if (getRversion() >= "3.4") withAutoprint else force)(\{ # examplesIf} diff --git a/tests/testthat/_snaps/db-interface.md b/tests/testthat/_snaps/db-interface.md index 92b35875e0..fc2e5c5671 100644 --- a/tests/testthat/_snaps/db-interface.md +++ b/tests/testthat/_snaps/db-interface.md @@ -1,3 +1,12 @@ +# copy_dm_to() copies data frames from any source + + Code + copy_dm_to(default_local_src(), dm_for_filter()) + Condition + Error: + ! The `dest` argument of `copy_dm_to()` must refer to a DBI connection as of dm 0.1.6. + i Please use `collect.dm()` instead. + # copy_dm_to() rejects overwrite and types arguments Code diff --git a/tests/testthat/_snaps/filter-dm.md b/tests/testthat/_snaps/filter-dm.md index d0fdc7c165..177b237760 100644 --- a/tests/testthat/_snaps/filter-dm.md +++ b/tests/testthat/_snaps/filter-dm.md @@ -5,7 +5,7 @@ Condition Warning: The `table` argument of `dm_filter()` is deprecated as of dm 1.0.0. - `dm_filter()` now takes named filter expressions, the names correspond to the tables to be filtered. You no longer need to call `dm_apply_filters()` to materialize the filters. + i `dm_filter()` now takes named filter expressions, the names correspond to the tables to be filtered. You no longer need to call `dm_apply_filters()` to materialize the filters. Output -- Metadata -------------------------------------------------------------------- Tables: `tf_1`, `tf_2`, `tf_3`, `tf_4`, `tf_5`, `tf_6` @@ -19,10 +19,10 @@ Condition Warning: The `dm` argument of `dm_filter()` is deprecated as of dm 1.0.0. - Please use the `.dm` argument instead. + i Please use the `.dm` argument instead. Warning: The `table` argument of `dm_filter()` is deprecated as of dm 1.0.0. - `dm_filter()` now takes named filter expressions, the names correspond to the tables to be filtered. You no longer need to call `dm_apply_filters()` to materialize the filters. + i `dm_filter()` now takes named filter expressions, the names correspond to the tables to be filtered. You no longer need to call `dm_apply_filters()` to materialize the filters. Output -- Metadata -------------------------------------------------------------------- Tables: `tf_1`, `tf_2`, `tf_3`, `tf_4`, `tf_5`, `tf_6` @@ -36,7 +36,7 @@ Condition Warning: The `table` argument of `dm_filter()` is deprecated as of dm 1.0.0. - `dm_filter()` now takes named filter expressions, the names correspond to the tables to be filtered. You no longer need to call `dm_apply_filters()` to materialize the filters. + i `dm_filter()` now takes named filter expressions, the names correspond to the tables to be filtered. You no longer need to call `dm_apply_filters()` to materialize the filters. Output -- Metadata -------------------------------------------------------------------- Tables: `tf_1`, `tf_2`, `tf_3`, `tf_4`, `tf_5`, `tf_6` @@ -48,7 +48,7 @@ Condition Warning: `dm_apply_filters()` was deprecated in dm 1.0.0. - Calling `dm_apply_filters()` after `dm_filter()` is no longer necessary. + i Calling `dm_apply_filters()` after `dm_filter()` is no longer necessary. Output -- Metadata -------------------------------------------------------------------- Tables: `tf_1`, `tf_2`, `tf_3`, `tf_4`, `tf_5`, `tf_6` @@ -60,7 +60,7 @@ Condition Warning: The `table` argument of `dm_filter()` is deprecated as of dm 1.0.0. - `dm_filter()` now takes named filter expressions, the names correspond to the tables to be filtered. You no longer need to call `dm_apply_filters()` to materialize the filters. + i `dm_filter()` now takes named filter expressions, the names correspond to the tables to be filtered. You no longer need to call `dm_apply_filters()` to materialize the filters. Output # A tibble: 3 x 4 c d e e1 @@ -73,7 +73,7 @@ Condition Warning: `dm_apply_filters_to_tbl()` was deprecated in dm 1.0.0. - Access tables directly after `dm_filter()`. + i Access tables directly after `dm_filter()`. Output # A tibble: 3 x 4 c d e e1 @@ -86,7 +86,7 @@ Condition Warning: The `table` argument of `dm_filter()` is deprecated as of dm 1.0.0. - `dm_filter()` now takes named filter expressions, the names correspond to the tables to be filtered. You no longer need to call `dm_apply_filters()` to materialize the filters. + i `dm_filter()` now takes named filter expressions, the names correspond to the tables to be filtered. You no longer need to call `dm_apply_filters()` to materialize the filters. Output # A tibble: 1 x 3 table filter zoomed @@ -97,10 +97,10 @@ Condition Warning: `dm_get_filters()` was deprecated in dm 1.0.0. - Filter conditions are no longer stored with the dm object. + i Filter conditions are no longer stored with the dm object. Output # A tibble: 0 x 3 - # ... with 3 variables: table , filter , zoomed + # i 3 variables: table , filter , zoomed # data structure diff --git a/tests/testthat/_snaps/learn.md b/tests/testthat/_snaps/learn.md index 2f17067ceb..189efc9da3 100644 --- a/tests/testthat/_snaps/learn.md +++ b/tests/testthat/_snaps/learn.md @@ -264,84 +264,132 @@ }, { "constraint_name": 2, + "table_name": "tf_1", + "column_name": "a", + "ordinal_position": 1 + }, + { + "constraint_name": 3, "table_name": "tf_2", "column_name": "c", "ordinal_position": 1 }, { - "constraint_name": 3, + "constraint_name": 4, + "table_name": "tf_2", + "column_name": "c", + "ordinal_position": 1 + }, + { + "constraint_name": 5, "table_name": "tf_2", "column_name": "d", "ordinal_position": 1 }, { - "constraint_name": 4, + "constraint_name": 6, "table_name": "tf_2", "column_name": "e", "ordinal_position": 1 }, { - "constraint_name": 4, + "constraint_name": 6, "table_name": "tf_2", "column_name": "e1", "ordinal_position": 2 }, { - "constraint_name": 5, + "constraint_name": 7, "table_name": "tf_3", "column_name": "f", "ordinal_position": 1 }, { - "constraint_name": 5, + "constraint_name": 7, "table_name": "tf_3", "column_name": "f1", "ordinal_position": 2 }, { - "constraint_name": 6, + "constraint_name": 8, + "table_name": "tf_3", + "column_name": "f", + "ordinal_position": 1 + }, + { + "constraint_name": 8, + "table_name": "tf_3", + "column_name": "f1", + "ordinal_position": 2 + }, + { + "constraint_name": 9, + "table_name": "tf_3", + "column_name": "g", + "ordinal_position": 1 + }, + { + "constraint_name": 10, "table_name": "tf_4", "column_name": "h", "ordinal_position": 1 }, { - "constraint_name": 7, + "constraint_name": 11, + "table_name": "tf_4", + "column_name": "h", + "ordinal_position": 1 + }, + { + "constraint_name": 12, "table_name": "tf_4", "column_name": "j", "ordinal_position": 1 }, { - "constraint_name": 7, + "constraint_name": 12, "table_name": "tf_4", "column_name": "j1", "ordinal_position": 2 }, { - "constraint_name": 8, + "constraint_name": 13, "table_name": "tf_5", "column_name": "k", "ordinal_position": 1 }, { - "constraint_name": 9, + "constraint_name": 14, + "table_name": "tf_5", + "column_name": "k", + "ordinal_position": 1 + }, + { + "constraint_name": 15, "table_name": "tf_5", "column_name": "l", "ordinal_position": 1 }, { - "constraint_name": 10, + "constraint_name": 16, "table_name": "tf_5", "column_name": "m", "ordinal_position": 1 }, { - "constraint_name": 11, + "constraint_name": 17, "table_name": "tf_6", "column_name": "n", "ordinal_position": 1 }, { - "constraint_name": 12, + "constraint_name": 18, + "table_name": "tf_6", + "column_name": "o", + "ordinal_position": 1 + }, + { + "constraint_name": 19, "table_name": "tf_6", "column_name": "o", "ordinal_position": 1 diff --git a/tests/testthat/test-db-interface.R b/tests/testthat/test-db-interface.R index 3f3608422f..357121c0f9 100644 --- a/tests/testthat/test-db-interface.R +++ b/tests/testthat/test-db-interface.R @@ -20,12 +20,9 @@ test_that("copy_dm_to() copies data frames to databases", { }) test_that("copy_dm_to() copies data frames from any source", { - expect_equivalent_dm( - expect_deprecated_obj( - copy_dm_to(default_local_src(), dm_for_filter()) - ), - dm_for_filter() - ) + expect_snapshot(error = TRUE, { + copy_dm_to(default_local_src(), dm_for_filter()) + }) }) # FIXME: Add test that set_key_constraints = FALSE doesn't set key constraints, diff --git a/tests/testthat/test-filter-dm.R b/tests/testthat/test-filter-dm.R index ded1d4092a..dafda764c8 100644 --- a/tests/testthat/test-filter-dm.R +++ b/tests/testthat/test-filter-dm.R @@ -44,8 +44,6 @@ test_that("dm_filter() legacy API", { test_that("dm_filter() deprecations", { local_options(lifecycle_verbosity = "warning") - skip_if_src_not("db") - expect_snapshot({ dm_filter(dm_for_filter(), tf_1, a > 4) dm_filter(dm = dm_for_filter(), tf_1, a > 4)