Add parens for MS access joins (#1688)

Fixes #1576

Author: hadley

NAMESPACE

@@ -293,6 +293,7 @@ S3method(sql_query_join,DBIConnection)
 S3method(sql_query_join,MariaDBConnection)
 S3method(sql_query_join,MySQL)
 S3method(sql_query_join,MySQLConnection)
+S3method(sql_query_multi_join,ACCESS)
 S3method(sql_query_multi_join,DBIConnection)
 S3method(sql_query_rows,DBIConnection)
 S3method(sql_query_save,"Microsoft SQL Server")

NEWS.md

@@ -1,5 +1,6 @@
 # dbplyr (development version)
 
+* MS Access now correctly generates SQL for multiple joins by adding required parentheses (#1576).
 * `.data$col`, `.data[[col]]`, `.env$var`, and `.env$[[var]]` now work correctly inside `across()` (#1520).
 * New `.sql` pronoun makes it a little easier to use known SQL functions in packages, requiring only `@importFrom dbplyr .sql` (#1117).
 * `join_by(between())` now correctly handles column renames (#1572).

R/backend-access.R

@@ -205,3 +205,49 @@ sql_escape_datetime.ACCESS <- function(con, x) {
 supports_window_clause.ACCESS <- function(con) {
   TRUE
 }
+
+#' @export
+sql_query_multi_join.ACCESS <- function(
+  con,
+  x,
+  joins,
+  table_names,
+  by_list,
+  select,
+  ...,
+  lvl = 0
+) {
+  if (vctrs::vec_duplicate_any(table_names)) {
+    cli_abort("{.arg table_names} must be unique.")
+  }
+
+  from <- dbplyr_sql_subquery(con, x, name = table_names[[1]], lvl = lvl)
+  names <- table_names[-1]
+  tables <- joins$table
+  types <- toupper(paste0(joins$type, " JOIN"))
+
+  n_joins <- length(types)
+
+  # MS Access requires: ((t1 JOIN t2 ON ...) JOIN t3 ON ...)
+  # N joins need N opening parens, each ON clause followed by closing paren
+  open_parens <- strrep("(", n_joins)
+  from <- sql(paste0(open_parens, from))
+
+  for (i in seq_len(n_joins)) {
+    table <- dbplyr_sql_subquery(con, tables[[i]], name = names[[i]], lvl = lvl)
+    by <- joins$by[[i]]
+    on <- sql_join_tbls(con, by = by, na_matches = by$na_matches)
+
+    from <- sql(paste0(
+      paste0(from, "\n"),
+      paste0(types[[i]], " ", table, "\n"),
+      paste0("ON ", on, ")")
+    ))
+  }
+
+  clauses <- list(
+    sql_clause_select(con, select),
+    sql_clause_from(from)
+  )
+  sql_format_clauses(clauses, lvl = lvl, con = con)
+}

R/db-sql.R

@@ -531,36 +531,27 @@ sql_query_multi_join.DBIConnection <- function(
   }
 
   from <- dbplyr_sql_subquery(con, x, name = table_names[[1]], lvl = lvl)
-
-  join_table_queries <- purrr::map2(
-    joins$table,
-    table_names[-1],
-    function(table, name) {
-      dbplyr_sql_subquery(con, table, name = name, lvl = lvl)
-    }
-  )
+  names <- table_names[-1]
+  tables <- joins$table
   types <- toupper(paste0(joins$type, " JOIN"))
-  join_clauses <- purrr::map2(
-    types,
-    join_table_queries,
-    \(join_kw, from) sql_clause(join_kw, from)
-  )
 
-  on_clauses <- purrr::map(
-    joins$by,
-    function(by) {
-      on <- sql_join_tbls(con, by = by, na_matches = by$na_matches)
-      sql_clause("ON", on, sep = " AND", parens = TRUE, lvl = 1)
-    }
-  )
-  join_on_clauses <- vctrs::vec_interleave(join_clauses, on_clauses)
+  n_joins <- length(types)
+  out <- vector("list", n_joins * 2)
+
+  for (i in seq_len(n_joins)) {
+    table <- dbplyr_sql_subquery(con, tables[[i]], name = names[[i]], lvl = lvl)
+    out[[2 * i - 1]] <- sql_clause(types[[i]], table)
+
+    by <- joins$by[[i]]
+    on <- sql_join_tbls(con, by = by, na_matches = by$na_matches)
+    out[[2 * i]] <- sql_clause("ON", on, sep = " AND", parens = TRUE, lvl = 1)
+  }
 
   clauses <- list2(
     sql_clause_select(con, select),
     sql_clause_from(from),
-    !!!join_on_clauses
+    !!!out
   )
-
   sql_format_clauses(clauses, lvl = lvl, con = con)
 }
 

R/sql.R

@@ -12,6 +12,10 @@ sql <- function(...) {
   structure(x, class = c("sql", "character"))
 }
 
+sql_c <- function(...) {
+  sql(paste0(unlist(list(...)), collapse = ""))
+}
+
 #' @export
 `[.sql` <- function(x, i) {
   sql(NextMethod())

tests/testthat/_snaps/backend-access.md

@@ -16,3 +16,31 @@
       SELECT TOP 6 `df`.*
       FROM `df`
 
+# multiple joins use parens #1576
+
+    Code
+      inner_join(left_join(lf1, lf2, by = "x"), lf3, by = "x")
+    Output
+      <SQL>
+      SELECT `lf1`.*, `b`, `c`
+      FROM ((`lf1`
+      LEFT JOIN `lf2`
+      ON `lf1`.`x` = `lf2`.`x`)
+      INNER JOIN `lf3`
+      ON `lf1`.`x` = `lf3`.`x`)
+
+---
+
+    Code
+      left_join(inner_join(left_join(lf1, lf2, by = "x"), lf3, by = "x"), lf4, by = "x")
+    Output
+      <SQL>
+      SELECT `lf1`.*, `b`, `c`, `d`
+      FROM (((`lf1`
+      LEFT JOIN `lf2`
+      ON `lf1`.`x` = `lf2`.`x`)
+      INNER JOIN `lf3`
+      ON `lf1`.`x` = `lf3`.`x`)
+      LEFT JOIN `lf4`
+      ON `lf1`.`x` = `lf4`.`x`)
+

tests/testthat/test-backend-access.R

@@ -109,3 +109,17 @@ test_that("queries translate correctly", {
   mf <- lazy_frame(x = 1, con = simulate_access())
   expect_snapshot(mf |> head())
 })
+
+test_that("multiple joins use parens #1576", {
+  lf1 <- lazy_frame(x = 1, a = 1, .name = "lf1", con = simulate_access())
+  lf2 <- lazy_frame(x = 1, b = 1, .name = "lf2", con = simulate_access())
+  lf3 <- lazy_frame(x = 1, c = 1, .name = "lf3", con = simulate_access())
+  lf4 <- lazy_frame(x = 1, d = 1, .name = "lf4", con = simulate_access())
+
+  expect_snapshot(left_join(lf1, lf2, by = "x") |> inner_join(lf3, by = "x"))
+  expect_snapshot(
+    left_join(lf1, lf2, by = "x") |>
+      inner_join(lf3, by = "x") |>
+      left_join(lf4, by = "x")
+  )
+})