Fix DBI binary standard writes (#246)

Co-authored-by: Zac Davies <zachary.davies+data@databricks.com>

Author: zacdav-db

NEWS.md

@@ -1,5 +1,6 @@
 # brickster (development version)
 
+-   Fixed `dbWriteTable()` and `dbAppendTable()` standard-path writes for binary columns, which now use Databricks `BINARY` types and `X'...'` literals when no staging volume is configured (#245)
 -   Fixed `git_source()` erroring when `type` was left at its default
 -   Fixed Unity Catalog volume file requests so `db_volume_*` paths containing spaces are encoded correctly (#231)
 -   `db_cluster_events()` now forwards the `event_types` argument to the API, which was previously ignored

R/databricks-dbi.R

@@ -1061,6 +1061,17 @@ db_clean_table_name <- function(name) {
   gsub('^\"|\"$', '', name)
 }
 
+# Check whether an R column represents Databricks binary values.
+db_is_binary_column <- function(x) {
+  if (inherits(x, "blob") || is.raw(x)) {
+    return(TRUE)
+  }
+
+  is.list(x) &&
+    purrr::every(x, \(value) is.raw(value) || is.null(value)) &&
+    purrr::some(x, is.raw)
+}
+
 #' Map R data types to Databricks SQL types
 #' @param dbObj A DatabricksConnection object
 #' @param obj R object(s) to get SQL types for
@@ -1072,6 +1083,10 @@ setMethod("dbDataType", "DatabricksConnection", function(dbObj, obj, ...) {
   purrr::map_chr(
     obj,
     function(x) {
+      if (db_is_binary_column(x)) {
+        return("BINARY")
+      }
+
       switch(
         class(x)[1],
         logical = "BOOLEAN",
@@ -1584,31 +1599,51 @@ db_create_table_from_data <- function(
 #' @keywords internal
 db_generate_typed_values_sql <- function(conn, data) {
   # Convert each row to SQL values with proper typing
-  row_values <- apply(data, 1, function(row) {
-    values <- purrr::map2_chr(row, names(data), function(val, col_name) {
-      col_data <- data[[col_name]]
-
-      if (is.na(val)) {
-        "NULL"
-      } else if (is.logical(col_data)) {
-        if (as.logical(val)) "TRUE" else "FALSE"
-      } else if (is.numeric(col_data)) {
-        # Don't quote numeric values to preserve type
-        as.character(val)
-      } else if (is.character(col_data)) {
-        # Quote string values and escape single quotes
-        db_escape_string_literal(conn, val)
-      } else {
-        # Default to quoted string for other types
-        db_escape_string_literal(conn, as.character(val))
-      }
+  row_values <- purrr::pmap_chr(data, function(...) {
+    row <- list(...)
+    values <- purrr::imap_chr(row, function(val, col_name) {
+      db_format_typed_value_sql(conn, val, data[[col_name]])
     })
     paste0("(", paste(values, collapse = ", "), ")")
   })
 
   paste(row_values, collapse = ", ")
 }
 
+# Format a single R value for inline SQL VALUES.
+db_format_typed_value_sql <- function(conn, val, col_data) {
+  if (db_is_missing_sql_value(val)) {
+    "NULL"
+  } else if (db_is_binary_column(col_data)) {
+    db_binary_literal(val)
+  } else if (is.logical(col_data)) {
+    if (as.logical(val)) "TRUE" else "FALSE"
+  } else if (is.numeric(col_data)) {
+    # Don't quote numeric values to preserve type
+    as.character(val)
+  } else if (is.character(col_data)) {
+    # Quote string values and escape single quotes
+    db_escape_string_literal(conn, val)
+  } else {
+    # Default to quoted string for other types
+    db_escape_string_literal(conn, as.character(val))
+  }
+}
+
+# Check whether a row value should be rendered as SQL NULL.
+db_is_missing_sql_value <- function(val) {
+  is.null(val) || (length(val) == 1L && is.na(val))
+}
+
+# Format raw bytes as a Databricks binary literal.
+db_binary_literal <- function(val) {
+  if (!is.raw(val)) {
+    cli::cli_abort("Binary columns must contain raw vectors or `NULL` values.")
+  }
+
+  paste0("X'", paste(toupper(as.character(val)), collapse = ""), "'")
+}
+
 #' Escape string literals for inline SQL VALUES
 #' @keywords internal
 db_escape_string_literal <- function(conn, val) {

R/databricks-dbplyr.R

@@ -228,30 +228,7 @@ copy_to.DatabricksConnection <- function(
 #' @returns SQL VALUES clause
 #' @keywords internal
 db_generate_typed_values_sql_for_view <- function(con, data) {
-  # Convert each row to SQL values with proper typing
-  row_values <- apply(data, 1, function(row) {
-    values <- purrr::map2_chr(row, names(data), function(val, col_name) {
-      col_data <- data[[col_name]]
-      
-      if (is.na(val)) {
-        "NULL"
-      } else if (is.logical(col_data)) {
-        if (as.logical(val)) "TRUE" else "FALSE"
-      } else if (is.numeric(col_data)) {
-        # Don't quote numeric values to preserve type
-        as.character(val)
-      } else if (is.character(col_data)) {
-        # Quote string values and escape single quotes
-        db_escape_string_literal(con, val)
-      } else {
-        # Default to quoted string for other types
-        db_escape_string_literal(con, as.character(val))
-      }
-    })
-    paste0("(", paste(values, collapse = ", "), ")")
-  })
-  
-  paste(row_values, collapse = ", ")
+  db_generate_typed_values_sql(con, data)
 }
 
 # Slightly modified version of sparklyr/R/dplyr_sql_translation.R (thank you!)

tests/testthat/test-databricks-dbi-offline-helpers.R

@@ -204,6 +204,65 @@ test_that("dbWriteTable routes to standard path when volume staging is not prefe
   expect_false(state$show_progress)
 })
 
+test_that("dbWriteTable standard path supports binary columns", {
+  con <- make_dbi_test_con(show_progress = FALSE)
+  value <- data.frame(id = 1:3)
+  value$payload <- I(list(as.raw(c(0, 15, 255)), raw(0), NULL))
+  state <- new.env(parent = emptyenv())
+  state$sql <- character(0)
+
+  local_mocked_bindings(
+    dbExistsTable = function(...) FALSE,
+    db_should_use_volume_method = function(...) FALSE,
+    dbExecute = function(conn, statement, ...) {
+      state$sql <- c(state$sql, statement)
+      0L
+    },
+    db_sql_exec_and_wait = function(statement, ...) {
+      state$sql <- c(state$sql, statement)
+      list(status = list(state = "SUCCEEDED"))
+    },
+    .package = "brickster"
+  )
+
+  expect_invisible(dbWriteTable(con, "tbl_binary", value, overwrite = TRUE))
+  expect_identical(
+    state$sql[[1]],
+    "CREATE OR REPLACE TABLE `tbl_binary` (`id` INT, `payload` BINARY)"
+  )
+  expect_identical(
+    state$sql[[2]],
+    paste0(
+      "INSERT INTO `tbl_binary` (`id`, `payload`) VALUES ",
+      "(1, X'000FFF'), (2, X''), (3, NULL)"
+    )
+  )
+})
+
+test_that("dbAppendTable standard path supports binary columns", {
+  con <- make_dbi_test_con(show_progress = FALSE)
+  value <- data.frame(id = 4L)
+  value$payload <- I(list(as.raw(171)))
+  state <- new.env(parent = emptyenv())
+  state$sql <- NULL
+
+  local_mocked_bindings(
+    dbExistsTable = function(...) TRUE,
+    db_should_use_volume_method = function(...) FALSE,
+    db_sql_exec_and_wait = function(statement, ...) {
+      state$sql <- statement
+      list(status = list(state = "SUCCEEDED"))
+    },
+    .package = "brickster"
+  )
+
+  expect_invisible(dbAppendTable(con, "tbl_binary", value))
+  expect_identical(
+    state$sql,
+    "INSERT INTO `tbl_binary` (`id`, `payload`) VALUES (4, X'AB')"
+  )
+})
+
 test_that("dbWriteTable handles row.names consistently for character and Id signatures", {
   con <- make_dbi_test_con()
   state <- new.env(parent = emptyenv())