No TRUE literal in SQL Server, should `escape()` default to `list(clause = "SELECT")` ?

[krlmlr]

MSSQL has no TRUE literal, but it's returned from translations: https://stackoverflow.com/a/7171264/946850.

dm uses a home-grown routine to insert records into MS SQL Server, via dbplyr::escape() . This breaks with dbplyr >= 2.2.0.

The reprex illustrates the problem. What's a good solution?

library(dbplyr)

con <- simulate_mssql()

# Buggy, no TRUE literal: https://stackoverflow.com/a/7171264/946850
escape(TRUE, con = con)
#> <SQL> TRUE

# This is what dm needs:
escape(c(TRUE, FALSE), parens = FALSE, collapse = NULL, con = con)
#> <SQL> TRUE
#> <SQL> FALSE

# Still buggy:
translate_sql_(list(TRUE), con = con)
#> <SQL> TRUE

# Correct, but how to achieve for a vector?
translate_sql_(list(TRUE), con = con, context = list(clause = "SELECT"))
#> <SQL> 1

# Useless for dm:
translate_sql_(list(c(TRUE, FALSE)), con = con, context = list(clause = "SELECT"))
#> <SQL> (1, 0)

^{Created on 2022-07-06 by the reprex package (v2.0.1)}

CC @TSchiefer.

[hadley]

Do you have a specific use case?

[krlmlr]

My use case is table ingestion in dm. When this is fixed, users could also do filter(some_bit_value == TRUE) to clarify intent. Desired behavior:

library(dbplyr)

con <- simulate_mssql()

# This is what dm needs:
escape(c(TRUE, FALSE), parens = FALSE, collapse = NULL, con = con)
#> <SQL> 1
#> <SQL> 0

[hadley]

There's a bit of description about the problem in https://dbplyr.tidyverse.org/reference/backend-mssql.html#bit-vs-boolean

Currently we have https://github.com/tidyverse/dbplyr/blob/main/R/backend-mssql.R#L509-L517

And I think you're suggesting it should just be:

#' @export
`sql_escape_logical.Microsoft SQL Server` <- function(con, x) {
  y <- ifelse(x, "1", "0")
  y[is.na(x)] <- "NULL"
  y
}

I think would be a safe change since you shouldn't be doing (e.g.) df %>% filter(x == FALSE) but instead should do df %>% filter(!x).

[krlmlr]

Perhaps shorter:

#' @export
`sql_escape_logical.Microsoft SQL Server` <- function(con, x) {
  if_else(x, "1", "0", "NULL")
}

I don't see any situation where the current behavior is correct, it might be safe to change.

[hadley]

Want to do a PR?