From d34dc70fa9a51c4a357fe3958c916e41315e1d8a Mon Sep 17 00:00:00 2001 From: owenjonesuob <21007837+owenjonesuob@users.noreply.github.com> Date: Fri, 3 May 2024 19:26:15 +0100 Subject: [PATCH 1/2] fix: pass naming pattern through to dm_learn_from_db() (#2213) --- R/db-helpers.R | 12 +----------- R/dm_from_con.R | 15 +++++++++++++-- R/learn.R | 6 +++--- 3 files changed, 17 insertions(+), 16 deletions(-) diff --git a/R/db-helpers.R b/R/db-helpers.R index 4ec793a533..76b9cc2d85 100644 --- a/R/db-helpers.R +++ b/R/db-helpers.R @@ -121,7 +121,7 @@ find_name_clashes <- function(old, new) { } #' @autoglobal -get_src_tbl_names <- function(src, schema = NULL, dbname = NULL, names = NULL) { +get_src_tbl_names <- function(src, schema = NULL, dbname = NULL, names_pattern = "{.table}") { if (!is_mssql(src) && !is_postgres(src) && !is_mariadb(src)) { warn_if_arg_not(schema, only_on = c("MSSQL", "Postgres", "MariaDB")) warn_if_arg_not(dbname, only_on = "MSSQL") @@ -155,16 +155,6 @@ get_src_tbl_names <- function(src, schema = NULL, dbname = NULL, names = NULL) { names_table <- get_names_table_mariadb(con) } - # Use smart default for `.names`, if it wasn't provided - if (!is.null(names)) { - names_pattern <- names - } else if (length(schema) == 1) { - names_pattern <- "{.table}" - } else { - names_pattern <- "{.schema}.{.table}" - cli::cli_inform('Using {.code .names = "{names_pattern}"}') - } - names_table <- names_table %>% filter(schema_name %in% !!(if (inherits(schema, "sql")) glue_sql_collapse(schema) else schema)) %>% collect() %>% diff --git a/R/dm_from_con.R b/R/dm_from_con.R index 21a20041a5..5b0eb14824 100644 --- a/R/dm_from_con.R +++ b/R/dm_from_con.R @@ -64,11 +64,22 @@ dm_from_con <- function( src <- src_from_src_or_con(con) + # Use smart default for `.names`, if it wasn't provided + dots <- list2(...) + if (!is.null(.names)) { + names_pattern <- .names + } else if (is.null(dots$schema) || length(dots$schema) == 1) { + names_pattern <- "{.table}" + } else { + names_pattern <- "{.schema}.{.table}" + cli::cli_inform('Using {.code .names = "{names_pattern}"}') + } + if (is.null(learn_keys) || isTRUE(learn_keys)) { # FIXME: Try to make it work everywhere tryCatch( { - dm_learned <- dm_learn_from_db(con, ...) + dm_learned <- dm_learn_from_db(con, ..., names_pattern = names_pattern) if (is_null(learn_keys)) { inform(c( "Keys queried successfully.", @@ -104,7 +115,7 @@ dm_from_con <- function( } if (is_null(table_names)) { - src_tbl_names <- get_src_tbl_names(src, ..., names = .names) + src_tbl_names <- get_src_tbl_names(src, ..., names_pattern = names_pattern) } else { src_tbl_names <- table_names if (is.null(names(src_tbl_names))) { diff --git a/R/learn.R b/R/learn.R index 4deb1cf659..19d00ff9da 100644 --- a/R/learn.R +++ b/R/learn.R @@ -33,7 +33,7 @@ #' iris_dm_learned <- dm_learn_from_db(src_sqlite) #' } #' @autoglobal -dm_learn_from_db <- function(dest, dbname = NA, schema = NULL, name_format = "{table}") { +dm_learn_from_db <- function(dest, dbname = NA, schema = NULL, names_pattern = "{.table}") { # assuming that we will not try to learn from (globally) temporary tables, which do not appear in sys.table con <- con_from_src_or_con(dest) src <- src_from_src_or_con(dest) @@ -51,8 +51,8 @@ dm_learn_from_db <- function(dest, dbname = NA, schema = NULL, name_format = "{t dm_name <- df_info$tables %>% - select(catalog = table_catalog, schema = table_schema, table = table_name) %>% - mutate(name = glue(!!name_format)) %>% + select(catalog = table_catalog, .schema = table_schema, .table = table_name) %>% + mutate(name = glue(!!names_pattern)) %>% pull() %>% unclass() %>% vec_as_names(repair = "unique") From a1119ed18df5532dc5bec25a83e3583514c6302d Mon Sep 17 00:00:00 2001 From: owenjonesuob <21007837+owenjonesuob@users.noreply.github.com> Date: Fri, 3 May 2024 22:26:36 +0100 Subject: [PATCH 2/2] chore: update tests to reflect changed default argument in get_src_tbl_names() --- tests/testthat/test-db-helpers.R | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/tests/testthat/test-db-helpers.R b/tests/testthat/test-db-helpers.R index ee42bc805a..b3c63b3a17 100644 --- a/tests/testthat/test-db-helpers.R +++ b/tests/testthat/test-db-helpers.R @@ -70,15 +70,15 @@ test_that("DB helpers work for MSSQL", { DBI::Id(catalog = "db_helpers_db", schema = "schema_db_helpers_2", table = "test_db_helpers_4") ) expect_identical( - get_src_tbl_names(my_test_src(), schema = c("dbo", "schema_db_helpers"))[["dbo.test_db_helpers_2"]], + get_src_tbl_names(my_test_src(), schema = c("dbo", "schema_db_helpers"), names_pattern = "{.schema}.{.table}")[["dbo.test_db_helpers_2"]], DBI::Id(schema = "dbo", table = "test_db_helpers_2") ) expect_identical( - get_src_tbl_names(my_test_src(), schema = c("dbo", "schema_db_helpers"))[["schema_db_helpers.test_db_helpers_2"]], + get_src_tbl_names(my_test_src(), schema = c("dbo", "schema_db_helpers"), names_pattern = "{.schema}.{.table}")[["schema_db_helpers.test_db_helpers_2"]], DBI::Id(schema = "schema_db_helpers", table = "test_db_helpers_2") ) expect_warning( - out <- get_src_tbl_names(my_test_src(), schema = c("dbo", "schema_db_helpers"), names = "{.table}")["test_db_helpers_2"], + out <- get_src_tbl_names(my_test_src(), schema = c("dbo", "schema_db_helpers"), names_pattern = "{.table}")["test_db_helpers_2"], 'Local name test_db_helpers_2 will refer to <"dbo"."test_db_helpers_2">, rather than to <"schema_db_helpers"."test_db_helpers_2">', fixed = TRUE ) @@ -90,7 +90,7 @@ test_that("DB helpers work for MSSQL", { )) ) expect_warning( - out <- get_src_tbl_names(my_test_src(), schema = c("schema_db_helpers", "dbo"), names = "{.table}")["test_db_helpers_2"], + out <- get_src_tbl_names(my_test_src(), schema = c("schema_db_helpers", "dbo"), names_pattern = "{.table}")["test_db_helpers_2"], 'Local name test_db_helpers_2 will refer to <"schema_db_helpers"."test_db_helpers_2">, rather than to <"dbo"."test_db_helpers_2">', fixed = TRUE ) @@ -145,15 +145,15 @@ test_that("DB helpers work for Postgres", { DBI::Id(schema = "schema_db_helpers", table = "test_db_helpers_2") ) expect_identical( - get_src_tbl_names(my_test_src(), schema = c("public", "schema_db_helpers"))["public.test_db_helpers_2"][[1]], + get_src_tbl_names(my_test_src(), schema = c("public", "schema_db_helpers"), names_pattern = "{.schema}.{.table}")["public.test_db_helpers_2"][[1]], DBI::Id(schema = "public", table = "test_db_helpers_2") ) expect_identical( - get_src_tbl_names(my_test_src(), schema = c("public", "schema_db_helpers"))["schema_db_helpers.test_db_helpers_2"][[1]], + get_src_tbl_names(my_test_src(), schema = c("public", "schema_db_helpers"), names_pattern = "{.schema}.{.table}")["schema_db_helpers.test_db_helpers_2"][[1]], DBI::Id(schema = "schema_db_helpers", table = "test_db_helpers_2") ) expect_warning( - out <- get_src_tbl_names(my_test_src(), schema = c("public", "schema_db_helpers"), names = "{.table}")["test_db_helpers_2"], + out <- get_src_tbl_names(my_test_src(), schema = c("public", "schema_db_helpers"), names_pattern = "{.table}")["test_db_helpers_2"], 'Local name test_db_helpers_2 will refer to <"public"."test_db_helpers_2">, rather than to <"schema_db_helpers"."test_db_helpers_2">', fixed = TRUE ) @@ -165,7 +165,7 @@ test_that("DB helpers work for Postgres", { )) ) expect_warning( - out <- get_src_tbl_names(my_test_src(), schema = c("schema_db_helpers", "public"), names = "{.table}")["test_db_helpers_2"], + out <- get_src_tbl_names(my_test_src(), schema = c("schema_db_helpers", "public"), names_pattern = "{.table}")["test_db_helpers_2"], 'Local name test_db_helpers_2 will refer to <"schema_db_helpers"."test_db_helpers_2">, rather than to <"public"."test_db_helpers_2">', fixed = TRUE )