Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SQL Server: Index local temp tables #600

Merged
merged 5 commits into from
Sep 16, 2023
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions NAMESPACE
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ S3method(odbcListColumns,OdbcConnection)
S3method(odbcListObjectTypes,default)
S3method(odbcListObjects,OdbcConnection)
S3method(odbcPreviewObject,OdbcConnection)
export(isTempTable)
export(odbc)
export(odbcConnectionActions)
export(odbcConnectionColumns)
Expand Down
5 changes: 5 additions & 0 deletions NEWS.md
Original file line number Diff line number Diff line change
@@ -1,4 +1,9 @@
# odbc (development version)
* SQL Server: Improved handling for local temp tables in dbWrite, dbAppendTable,
dbExistTable (@detule, #600)
* Teradata: Improved handling for temp tables (@detule, #589, 590)
* Oracle: Fix regression when falling back to odbcConnectionColumns to
describe column data types (@detule, #587)

# odbc 1.3.5

Expand Down
5 changes: 3 additions & 2 deletions R/Table.R
Original file line number Diff line number Diff line change
Expand Up @@ -208,7 +208,8 @@ createFields <- function(con, fields, field.types, row.names) {
setMethod(
"dbExistsTable", c("OdbcConnection", "Id"),
function(conn, name, ...) {
name@name[["table"]] %in% odbcConnectionTables(conn,
dbExistsTable(
conn,
name = id_field(name, "table"),
catalog_name = id_field(name, "catalog"),
schema_name = id_field(name, "schema")
Expand All @@ -231,6 +232,6 @@ setMethod(
"dbExistsTable", c("OdbcConnection", "character"),
function(conn, name, ...) {
stopifnot(length(name) == 1)
df <- odbcConnectionTables(conn, name = name)
df <- odbcConnectionTables(conn, name = name, ...)
NROW(df) > 0
})
130 changes: 121 additions & 9 deletions R/db.R
Original file line number Diff line number Diff line change
@@ -1,3 +1,45 @@

#' Helper method used to determine if a table identifier is that
#' of a temporary table.
#'
#' Currently implemented only for select back-ends where
#' we have a use for it (SQL Server, for example). Generic, in case
#' we develop a broader use case.
#' @param conn OdbcConnection
#' @param name Table name
#' @param ... additional parameters to methods
#' @rdname isTempTable
#' @export
setGeneric(
"isTempTable",
valueClass = "logical",
function(conn, name, ...) {
standardGeneric("isTempTable")
}
)

#' @rdname isTempTable
setMethod(
"isTempTable",
c("OdbcConnection", "Id"),
function(conn, name, ...) {
isTempTable(conn,
name = id_field(name, "table"),
catalog_name = id_field(name, "catalog"),
schema_name = id_field(name, "schema"),
...)
}
)

#' @rdname isTempTable
setMethod(
"isTempTable",
c("OdbcConnection", "SQL"),
function(conn, name, ...) {
isTempTable(conn, dbUnquoteIdentifier(conn, name)[[1]], ...)
}
)

# Oracle --------------------------------------------------------------------

# Simple class prototype to avoid messages about unknown classes from setMethod
Expand Down Expand Up @@ -203,19 +245,89 @@ setMethod("sqlCreateTable", "DB2/AIX64",

# Microsoft SQL Server ---------------------------------------------------------

# Simple class prototype to avoid messages about unknown classes from setMethod
#' Simple class prototype to avoid messages about unknown classes from setMethod
#' @rdname SQLServer
#' @usage NULL
setClass("Microsoft SQL Server", where = class_cache)

# For SQL Server, conn@quote will return the quotation mark, however
# both quotation marks as well as square bracket are used interchangeably for
# delimited identifiers. See:
# https://learn.microsoft.com/en-us/sql/relational-databases/databases/database-identifiers?view=sql-server-ver16
# Therefore strip the brackets first, and then call the DBI method that strips
# the quotation marks.
# TODO: the generic implementation in DBI should take a quote char as
# parameter.
#' SQL Server specific implementation.
#'
#' For SQL Server, `conn@quote` will return the quotation mark, however
#' both quotation marks as well as square bracket are used interchangeably for
#' delimited identifiers. See:
#' \url{https://learn.microsoft.com/en-us/sql/relational-databases/databases/database-identifiers?view=sql-server-ver16}
#' Therefore strip the brackets first, and then call the DBI method that strips
#' the quotation marks.
#' TODO: the generic implementation in DBI should take a quote char as
#' parameter.
#'
#' @rdname SQLServer
#' @docType methods
#' @inheritParams DBI::dbUnquoteIdentifier
#' @usage NULL
setMethod("dbUnquoteIdentifier", c("Microsoft SQL Server", "SQL"),
function(conn, x, ...) {
x <- gsub("(\\[)([^\\.]+?)(\\])", "\\2", x)
callNextMethod( conn, x, ... )
})

#' SQL Server specific implementation.
#'
#' Local temp tables are stored as
#' \code{ [tempdb].[dbo].[#name]________(padding using underscores)[numeric identifier] }
#'
#' True if:
#' - If catalog_name is supplied it must equal "temdb" or "%" ( wildcard )
#' - Name must start with "#" followd by a non-"#" character
Comment on lines +276 to +281
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest just writing all these method docs in a bulleted list in the SQLServer docs. That makes it a bit easier to see how the final rendered doc will look.

And I think it's fine to leave the @usage as is, so the read can more easily see what methods are customised.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Hadley.

I took a look at the markdown in SQLServer.md and fwict it already looks like a bulleted list:

  \details{
  True if:
  \itemize{
  \item If catalog_name is supplied it must equal "temdb" or "\%" ( wildcard )
  \item Name must start with "#" followd by a non-"#" character
  }

Let me know if you had something else in mind.

RE: usage, I spend some time struggling with this:

❯ checking for code/documentation mismatches ... WARNING
  Functions or methods with usage in documentation object 'Microsoft SQL Server-class' but not in code:
    ‘\S4method{isTempTable}{Microsoft SQL Server,character}’

My first thought was that it has something to do with the class name having white-space characters but now i am not so sure.

The class and method are tagged like so:

  \name{Microsoft SQL Server-class}                                                                                                                                                                                                         
  \alias{Microsoft SQL Server-class}
  \alias{isTempTable,Microsoft SQL Server,character-method}

while the usage section:

\usage{
  \S4method{isTempTable}{`Microsoft SQL Server`,character}(conn, name, catalog_name = NULL, schema_name = NULL, ...)                                                                     
}

Let me know if you have any ideas.

Thanks again!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you manually change the alias to the following, does the problem go away?

  \alias{isTempTable,`Microsoft SQL Server`,character-method}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good suggestion; alas doesn't work.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Weird! Definitely not worth looking into further.

#' @rdname SQLServer
#' @usage NULL
setMethod("isTempTable", c("Microsoft SQL Server", "character"),
function(conn, name, catalog_name = NULL, schema_name = NULL, ...) {
if ( !is.null(catalog_name) &&
catalog_name != "%" &&
length(catalog_name ) > 0 &&
catalog_name != "tempdb" ) {
return(FALSE)
}

if ( !grepl("^[#][^#]", name ) ) {
return(FALSE)
}
return(TRUE)
})

#' SQL server specific dbExistsTable implementation that accounts for
#' local temp tables.
#'
#' If we can identify that the name is that of a local temp table
#' then adjust the identifier and query appropriately.
#'
#' Note, the implementation here is such that it assumes the metadata attribute is
#' set such that catalog functions accept wildcard entries.
#'
#' Driver note. OEM driver will return correctly for
#' name, \code{catalog_name = "tempdb"} in some circumstances. For exmaple
#' if the name has no underscores to beginwith. FreeTDS, will not index
#' the table correctly unless name is adjusted ( allowed trailing wildcards to
#' accomodate trailing underscores and postfix ).
#'
#' Therefore, in all cases query for \code{name___%}.
#' @rdname SQLServer
#' @docType methods
#' @aliases dbExistsTable
#' @inherit DBI::dbExistsTable
#' @usage NULL
setMethod(
"dbExistsTable", c("Microsoft SQL Server", "character"),
function(conn, name, ...) {
stopifnot(length(name) == 1)
if ( isTempTable( conn, name, ... ) )
{
name <- paste0(name, "\\_\\_\\_%");
df <- odbcConnectionTables(conn, name, catalog_name = "tempdb", schema_name = "dbo")
}
else {
df <- odbcConnectionTables(conn, name = name, ...)
}
NROW(df) > 0
})
131 changes: 131 additions & 0 deletions man/SQLServer.Rd

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

27 changes: 27 additions & 0 deletions man/isTempTable.Rd

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

28 changes: 28 additions & 0 deletions tests/testthat/test-SQLServer.R
Original file line number Diff line number Diff line change
Expand Up @@ -234,4 +234,32 @@ test_that("SQLServer", {
expect_equal(nrow(dbFetch(rs, n = 0)), 0)
expect_equal(nrow(dbFetch(rs, n = 10)), 2)
})

test_that("isTempTable tests", {
con <- DBItest:::connect(DBItest:::get_default_context())
expect_true( isTempTable(con, "#myTmp"))
expect_true( isTempTable(con, "#myTmp", catalog_name = "tempdb" ))
expect_true( isTempTable(con, "#myTmp", catalog_name = "%" ))
expect_true( isTempTable(con, "#myTmp", catalog_name = NULL ))
expect_true( !isTempTable(con, "##myTmp"))
expect_true( !isTempTable(con, "#myTmp", catalog_name = "abc" ))
})

test_that("dbExistsTable accounts for local temp tables", {
con <- DBItest:::connect(DBItest:::get_default_context())
tbl_name <- "#myTemp"
tbl_name2 <- "##myTemp"
tbl_name3 <- "#myTemp2"
DBI::dbExecute(con, paste0("CREATE TABLE ", tbl_name, " (
id int not null,
primary key (id) )"), immediate = TRUE)
expect_true( dbExistsTable( con, tbl_name) )
expect_true( dbExistsTable( con, tbl_name, catalog_name = "tempdb") )
# Fail because not recognized as temp table ( catalog not tempdb )
expect_true( !dbExistsTable( con, tbl_name, catalog_name = "abc") )
# Fail because not recognized as temp table ( second char "#" )
expect_true( !dbExistsTable( con, tbl_name2, catalog_name = "tempdb" ) )
# Fail because table not actually present
expect_true( !dbExistsTable( con, tbl_name3, catalog_name = "tempdb" ) )
})
})