From aec6ab8676e91672c7d39a20ca328a129d330516 Mon Sep 17 00:00:00 2001 From: eauleaf Date: Thu, 30 Nov 2023 16:02:41 -0800 Subject: [PATCH 1/6] Produce the DB indexing parameters in SQL order. Might help with issue #426: "dbplyr 2.4.0- breaking changes #426" And my issue, which is similar to #214: "Name and schema in DBI::Id #214" --- R/00-Id.R | 47 +++++++++++++++++++++++++++++++++---- tests/testthat/test-00-Id.R | 18 +++++++++++--- 2 files changed, 57 insertions(+), 8 deletions(-) diff --git a/R/00-Id.R b/R/00-Id.R index b196513ce..0e3b5b7a9 100644 --- a/R/00-Id.R +++ b/R/00-Id.R @@ -21,15 +21,16 @@ setClass("Id", slots = list(name = "character")) #' #' Objects of this class are also returned from [dbListObjects()]. #' -#' @param ... Components of the hierarchy, e.g. `schema`, `table`, or `cluster`, +#' @param ... Components of the hierarchy, e.g. `cluster`, #' `catalog`, `schema`, `table`, depending on the database backend. For more #' on these concepts, see #' @export #' @examples #' # Identifies a table in a specific schema: #' Id("dbo", "Customer") -#' # You can name the components if you want, but it's not needed -#' Id(schema = "dbo", table = "Customer") +#' # You can name the components to ensure a correct output order, +#' # but names are not required, e.g: +#' Id(table = "Customer", schema = "dbo") #' #' # Create a SQL expression for an identifier: #' dbQuoteIdentifier(ANSI(), Id("nycflights13", "flights")) @@ -39,12 +40,14 @@ setClass("Id", slots = list(name = "character")) #' dbWriteTable(con, Id("myschema", "mytable"), data.frame(a = 1)) #' } Id <- function(...) { - components <- c(...) + + components <- orderIdParams(...) + if (!is.character(components)) { stop("All elements of `...` must be strings.", call. = FALSE) } - new("Id", name = c(...)) + methods::new("Id", name = components) } #' @export @@ -56,3 +59,37 @@ toString.Id <- function(x, ...) { dbQuoteIdentifier_DBIConnection_Id <- function(conn, x, ...) { SQL(paste0(dbQuoteIdentifier(conn, x@name), collapse = ".")) } + + +#' Order DBI::Id() user inputs +#' +#' Create the specific order: +#' catalog > cluster > schema > table +#' +#' @param ... table param and unnamed `Id` components +#' @param database optional database `Id` component +#' @param catalog optional catalog `Id` component +#' @param cluster optional cluster `Id` component +#' @param schema optional schema `Id` component +#' @param table optional table `Id` component +#' +#' @return vector of user's inputs, with names if specified, in correct SQL order +#' +#' @examples +#' DBI:::orderIdParams(table = "flights", schema = "nycflights13", 'unnamed_id') +#' DBI:::orderIdParams("nycflights13", "flights") +#' +orderIdParams <- function(..., database = NULL, + catalog = NULL, cluster = NULL, + schema = NULL, table = NULL){ + + list( + database = database, + cluster = cluster, + catalog = catalog, + schema = schema, + ..., + table = table + ) |> unlist() + +} diff --git a/tests/testthat/test-00-Id.R b/tests/testthat/test-00-Id.R index 1c408f360..adadc107d 100644 --- a/tests/testthat/test-00-Id.R +++ b/tests/testthat/test-00-Id.R @@ -1,14 +1,26 @@ test_that("Id() requires a character vector", { - expect_snapshot(Id(1), error = TRUE) + expect_error(Id(1)) }) test_that("has a decent print method", { - expect_snapshot(Id("a", "b")) + expect_equal( + utils::capture.output(Id(schema = "a", table = "b")), + " \"a\".\"b\"" + ) }) test_that("each element is quoted individually", { expect_equal( - DBI::dbQuoteIdentifier(ANSI(), Id("a", "b.c")), + dbQuoteIdentifier(ANSI(), Id("a", "b.c")), SQL('"a"."b.c"') ) }) + +test_that("Id organizes standard named elements", { + expect_equal( + dbQuoteIdentifier(ANSI(), Id("unnamed", + table = "last", schema = "3rd", + cluster = '1st', catalog = "2nd")), + SQL('"1st"."2nd"."3rd"."unnamed"."last"') + ) +}) From be917bdf2d7ce0dd5fdd6743b239971d1663b06e Mon Sep 17 00:00:00 2001 From: eauleaf Date: Thu, 30 Nov 2023 16:20:18 -0800 Subject: [PATCH 2/6] simplify doc changes --- R/00-Id.R | 4 +--- tests/testthat/test-00-Id.R | 2 +- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/R/00-Id.R b/R/00-Id.R index 0e3b5b7a9..ed650586f 100644 --- a/R/00-Id.R +++ b/R/00-Id.R @@ -28,7 +28,7 @@ setClass("Id", slots = list(name = "character")) #' @examples #' # Identifies a table in a specific schema: #' Id("dbo", "Customer") -#' # You can name the components to ensure a correct output order, +#' # You can name the components to order `Id()` output, #' # but names are not required, e.g: #' Id(table = "Customer", schema = "dbo") #' @@ -40,9 +40,7 @@ setClass("Id", slots = list(name = "character")) #' dbWriteTable(con, Id("myschema", "mytable"), data.frame(a = 1)) #' } Id <- function(...) { - components <- orderIdParams(...) - if (!is.character(components)) { stop("All elements of `...` must be strings.", call. = FALSE) } diff --git a/tests/testthat/test-00-Id.R b/tests/testthat/test-00-Id.R index adadc107d..07efe132d 100644 --- a/tests/testthat/test-00-Id.R +++ b/tests/testthat/test-00-Id.R @@ -16,7 +16,7 @@ test_that("each element is quoted individually", { ) }) -test_that("Id organizes standard named elements", { +test_that("Id organizes the standard named elements", { expect_equal( dbQuoteIdentifier(ANSI(), Id("unnamed", table = "last", schema = "3rd", From ecde7d983df55a5b8f539d336e398bbd7c1f18c9 Mon Sep 17 00:00:00 2001 From: eauleaf Date: Thu, 30 Nov 2023 16:57:09 -0800 Subject: [PATCH 3/6] simplified code --- R/00-Id.R | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/R/00-Id.R b/R/00-Id.R index ed650586f..59a742114 100644 --- a/R/00-Id.R +++ b/R/00-Id.R @@ -80,14 +80,13 @@ dbQuoteIdentifier_DBIConnection_Id <- function(conn, x, ...) { orderIdParams <- function(..., database = NULL, catalog = NULL, cluster = NULL, schema = NULL, table = NULL){ - - list( + c( database = database, cluster = cluster, catalog = catalog, schema = schema, ..., table = table - ) |> unlist() + ) } From 7501ce5cf573a5909aa768fdbcf2b7caf11e56e8 Mon Sep 17 00:00:00 2001 From: eauleaf Date: Tue, 5 Dec 2023 16:12:46 -0800 Subject: [PATCH 4/6] updated per krlmlr and hadley comments --- R/00-Id.R | 24 ++---------------------- tests/testthat/test-00-Id.R | 11 ++++++++++- 2 files changed, 12 insertions(+), 23 deletions(-) diff --git a/R/00-Id.R b/R/00-Id.R index 59a742114..4fbe80019 100644 --- a/R/00-Id.R +++ b/R/00-Id.R @@ -59,34 +59,14 @@ dbQuoteIdentifier_DBIConnection_Id <- function(conn, x, ...) { } -#' Order DBI::Id() user inputs -#' -#' Create the specific order: -#' catalog > cluster > schema > table -#' -#' @param ... table param and unnamed `Id` components -#' @param database optional database `Id` component -#' @param catalog optional catalog `Id` component -#' @param cluster optional cluster `Id` component -#' @param schema optional schema `Id` component -#' @param table optional table `Id` component -#' -#' @return vector of user's inputs, with names if specified, in correct SQL order -#' -#' @examples -#' DBI:::orderIdParams(table = "flights", schema = "nycflights13", 'unnamed_id') -#' DBI:::orderIdParams("nycflights13", "flights") -#' orderIdParams <- function(..., database = NULL, catalog = NULL, cluster = NULL, schema = NULL, table = NULL){ - c( - database = database, + c(database = database, cluster = cluster, catalog = catalog, schema = schema, ..., - table = table - ) + table = table) } diff --git a/tests/testthat/test-00-Id.R b/tests/testthat/test-00-Id.R index 07efe132d..20909d4c9 100644 --- a/tests/testthat/test-00-Id.R +++ b/tests/testthat/test-00-Id.R @@ -1,5 +1,5 @@ test_that("Id() requires a character vector", { - expect_error(Id(1)) + expect_snapshot(Id(1), error = TRUE) }) test_that("has a decent print method", { @@ -24,3 +24,12 @@ test_that("Id organizes the standard named elements", { SQL('"1st"."2nd"."3rd"."unnamed"."last"') ) }) + +test_that("Id organizes mingled named and unnamed elements; ignores NULL", { + expect_equal( + dbQuoteIdentifier(ANSI(), Id( + table = "4", some_ref = '2', "3", catalog = "1", cluster = NULL)), + SQL('"1"."2"."3"."4"') + ) +}) + From fb08391e87ab614fbd5f433a160f4ceaecbadbcf Mon Sep 17 00:00:00 2001 From: eauleaf Date: Tue, 5 Dec 2023 16:41:23 -0800 Subject: [PATCH 5/6] to minimize changes --- R/00-Id.R | 7 +++---- tests/testthat/test-00-Id.R | 7 ++----- 2 files changed, 5 insertions(+), 9 deletions(-) diff --git a/R/00-Id.R b/R/00-Id.R index 4fbe80019..6866bdbad 100644 --- a/R/00-Id.R +++ b/R/00-Id.R @@ -22,14 +22,13 @@ setClass("Id", slots = list(name = "character")) #' Objects of this class are also returned from [dbListObjects()]. #' #' @param ... Components of the hierarchy, e.g. `cluster`, -#' `catalog`, `schema`, `table`, depending on the database backend. For more +#' `catalog`, `schema`, or `table`, depending on the database backend. For more #' on these concepts, see #' @export #' @examples #' # Identifies a table in a specific schema: #' Id("dbo", "Customer") -#' # You can name the components to order `Id()` output, -#' # but names are not required, e.g: +#' # You can name the components if you want, but it's not needed #' Id(table = "Customer", schema = "dbo") #' #' # Create a SQL expression for an identifier: @@ -45,7 +44,7 @@ Id <- function(...) { stop("All elements of `...` must be strings.", call. = FALSE) } - methods::new("Id", name = components) + new("Id", name = components) } #' @export diff --git a/tests/testthat/test-00-Id.R b/tests/testthat/test-00-Id.R index 20909d4c9..3587899ee 100644 --- a/tests/testthat/test-00-Id.R +++ b/tests/testthat/test-00-Id.R @@ -3,15 +3,12 @@ test_that("Id() requires a character vector", { }) test_that("has a decent print method", { - expect_equal( - utils::capture.output(Id(schema = "a", table = "b")), - " \"a\".\"b\"" - ) + expect_snapshot(Id("a", "b")) }) test_that("each element is quoted individually", { expect_equal( - dbQuoteIdentifier(ANSI(), Id("a", "b.c")), + DBI::dbQuoteIdentifier(ANSI(), Id("a", "b.c")), SQL('"a"."b.c"') ) }) From 5b91bce02fc8cdcec54f0e52a8a8c47c72f67dd3 Mon Sep 17 00:00:00 2001 From: eauleaf <81445509+eauleaf@users.noreply.github.com> Date: Wed, 6 Dec 2023 09:58:36 -0800 Subject: [PATCH 6/6] Update R/00-Id.R Co-authored-by: Hadley Wickham --- R/00-Id.R | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/R/00-Id.R b/R/00-Id.R index 6866bdbad..6d75661ad 100644 --- a/R/00-Id.R +++ b/R/00-Id.R @@ -66,6 +66,6 @@ orderIdParams <- function(..., database = NULL, catalog = catalog, schema = schema, ..., - table = table) - + table = table + ) }