Add DuckDB functionality: Changes (99739ac) from SAE environment

[stephhazlitt]

Changes from @ateucher from the DIP SRE.

[BonnieJRobert]

Suggested change

	person("Bonnie", "Robert", , "bonnie.robbie@gov.bc.ca", role = "aut"),
	person("Bonnie", "Ashcroft", , "bonnie.ashcroft@gov.bc.ca", role = "aut"),

[BonnieJRobert]

an example here would be helpful - using one of the built-in datasets, perhaps?

[BonnieJRobert]

Suggested change

	#' @export
	#' @export
	#' @examples
	#' \dontrun{
	#' con <- DBI::dbConnect(duckdb::duckdb())
	#' p <- "./inst/extdata/starwars-dict.csv"
	#' spec <- dput(sniff_csv_duckdb(p)$schema)
	#' csv_to_duckdb_query(con, p, "starwars", spec)
	#' }

[BonnieJRobert]

Maybe not necessary given the users and use cases of dipr, but I wonder about guarding against SQL injection. In this case via the tbl_name parameter:

if (grepl(";", tbl_name) stop(glue::glue("possible SQL injection introduced via tablename: {tbl_name}"))

[BonnieJRobert]

This logic is confusing; code comments would be helpful.

[BonnieJRobert]

I'm tempted to extract the code that constructs the various *_opts_parsed variables into individual functions. Consider refactoring into smaller, focused functions like:
format_varchar_scalar_opts <- function(opts) { ... }
format_varchar_array_opts <- function(opts) { ... } etc.

[BonnieJRobert]

When the final SQL CREATE or REPLACE TABLE statement is executed via DBI::dbExecute(), column names with spaces (like First Name AS first_name) will cause SQL syntax errors. The dipr functions are behaving as expected, but it could be a "nice to have" to warn against this. Here's an example:

> print(strSQL)
CREATE OR REPLACE TABLE starwars AS SELECT Name Abbr AS name_abbr, Name AS name FROM read_csv( './inst/extdata/starwars-dict.csv', types = { 'Name Abbr': 'VARCHAR', 'Name': 'VARCHAR' })
> DBI::dbExecute(con,strSQL)

This will cause dbSendQuery() to error.

[BonnieJRobert]

I focused my review on the code in the csv-duckdb.R file and ran a few toy examples. The code runs as expected with no serious errors that I could reproduce. However I highlighted a couple areas of concern. Additionally, the master branch should be renamed to main and there is a stale branch from several years back with a few updates to the README.