OdbcResult: Ability to explicitely describe parameters

[detule]

Hi @simonpcouch @hadley

This is a draft/RFC of a feature that would allow (power) users to explicitly describe parameter attributes when submitting parametrized queries.

The details are in #518 and this PR would address the issue there. In summary:

• We do a very good job of inferring the parameter metadata, but at the end of the day it's only an informed guess.
• In some cases the user may wish to override those inferred values. Linked issue above is one of those.
• Offer them the ability to do that. This type of feature is not uncommon in other ODBC programming interfaces.

Some notes about the PR:

• Added an ODBC_TYPE helper method that would help with the user not having to guess what integer corresponds to SQL_WVARCHAR, for example.
• The example is in ?OdbcResult.
• I have a few items below as TODOs, including offering the usage of this parameter more broadly.

TODO:

• Rename ODBC_TYPE to SQL_TYPE
• Permeate parameter_description to dbSendQuery, dbGetQuery
• Add unit tests

[simonpcouch]

Seems very much useful for power users! I don't have much to offer in terms of critique on the src/ side, but left some notes on the R interface.

[simonpcouch]

Suggested change

	#' @examples
	#' \dontrun{
	#' library(odbc)
	#' # Writing UNICODE into a VARCHAR
	#' # column with SQL server
	#' DBI::dbRemoveTable(conn, "#tmp")
	#' dbExecute(conn, "CREATE TABLE #tmp (col1 VARCHAR(50) COLLATE Latin1_General_100_CI_AI_SC_UTF8);")
	#' res <- dbSendQuery(conn, "INSERT INTO #tmp SELECT ? COLLATE Latin1_General_100_CI_AI_SC_UTF8")
	#' description <- data.frame(param_index = 1, data_type = ODBC_TYPE("WVARCHAR"),
	#' column_size = 5, decimal_digits = NA_integer_)
	#' dbBind(res, params = list("\u2915"), param_description = description)
	#' dbClearResult(res)
	#' DBI::dbReadTable(conn, "#tmp")
	#' }
	#' @examplesIf FALSE
	#' # Writing UNICODE into a VARCHAR column with SQL server
	#' dbRemoveTable(conn, "#tmp")
	#'
	#' dbExecute(
	#' conn,
	#' "CREATE TABLE #tmp (col1 VARCHAR(50) COLLATE Latin1_General_100_CI_AI_SC_UTF8);"
	#' )
	#'
	#' res <- dbSendQuery(
	#' conn,
	#' "INSERT INTO #tmp SELECT ? COLLATE Latin1_General_100_CI_AI_SC_UTF8"
	#' )
	#'
	#' description <- data.frame(
	#' param_index = 1,
	#' data_type = ODBC_TYPE("WVARCHAR"),
	#' column_size = 5,
	#' decimal_digits = NA_integer_
	#' )
	#'
	#' dbBind(res, params = list("\u2915"), param_description = description)
	#' dbClearResult(res)
	#'
	#' dbReadTable(conn, "#tmp")

Some style edits I'd make:

• examples with \dontrun{} -> examplesIf FALSE so that users don't see ## Not run tags and CRAN definitely won't try to run the examples
• I believe the DBI:: namespacing here may not be needed?
• Line-breaking more liberally via codegrip

[simonpcouch]

Suggested change

	#' @param param_description A data.frame with per-parameter attribute
	#' overrides. Argument is optional; if used it must have columns:
	#' * param_index Index of parameter in query ( beginning with 1 ).
	#' * data_type Integer corresponding to the parameter SQL Data Type.
	#' See \code{\link{ODBC_TYPE}}.
	#' * column_size Size of parameter.
	#' * decimal_digits Either precision or the scale of the parameter
	#' depending on type.
	#' @param param_description A data frame containing per-parameter attribute overrides.
	#' Optional argument that, if provided, must contain the following columns:
	#' \describe{
	#' \item{param_index}{Index of parameter in query (beginning with 1).}
	#' \item{data_type}{Integer corresponding to the parameter SQL Data Type.
	#' See [ODBC_TYPE()].
	#' \item{column_size}{Size of parameter.}
	#' \item{decimal_digits}{Either precision or the scale of the parameter
	#' depending on type.}
	#' }

• Transitions to \describe for data frame columns
• Links to ODBC_TYPE() with roxygen shorthand
• Refers to "data frame" rather than data.frame since tibbles and other data.frame subclasses are fine

[simonpcouch]

Suggested change

	param_description = data.frame(),
	param_description = NULL,

NULL indicates a bit more clearly to me that "this is the default that won't actually be used if you don't supply anything"

[simonpcouch]

Suggested change

	if (nrow(param_description)) {
	if (!is.null(param_description)) {

Aligning with above.

[simonpcouch]

Suggested change

	if (!all(c("param_index", "data_type", "column_size", "decimal_digits")
	%in% colnames(param_description))) {
	cli::cli_abort(
	"param_description data.frame does not have necessary columns."
	)
	}
	check_data_frame(param_description)
	needed_columns <- c("param_index", "data_type", "column_size", "decimal_digits")
	if (!all(needed_columns %in% colnames(param_description))) {
	cli::cli_abort(
	"{.arg param_description} must have columns {.field {needed_columns}}, but
	doesn't have column{?s}
	{.field {.or {needed_columns[needed_columns %in% colnames(param_description)]}}}."
	)
	}

check_data_frame() will first ensure that the thing is a data frame and provide an informative message if not before we do the finer-grain check for needed columns.

[simonpcouch]

I think, just to situate more comfily in this package's namespace, this may be better formatted as:

Suggested change

	ODBC_TYPE <- function(type) {
	odbcType <- function(type) {