Bug in sql_query_upsert for oracle backend

[TBlackmore]

I'm not sure if i'm just oblivious to something here but the SQL generated by sql_query_upsert for an Oracle connection seems to have a bug in it. It's referencing a table called 'excluded' which doesn't seem to be invloved in anything. It also failed to wrap the ON clause in parenthesis which I think is required by oracle.

library(dplyr)
#> 
#> Attaching package: 'dplyr'
#> The following objects are masked from 'package:stats':
#> 
#>     filter, lag
#> The following objects are masked from 'package:base':
#> 
#>     intersect, setdiff, setequal, union
library(dbplyr)
#> 
#> Attaching package: 'dbplyr'
#> The following objects are masked from 'package:dplyr':
#> 
#>     ident, sql

dbplyr::sql_query_upsert( con = simulate_oracle(),
                          table = ident("t1"),
                          from = ident("t2"),
                          by = c("a"),
                          update_cols = c("b")
)
#> <SQL> MERGE INTO `t1`
#> USING `t2` AS `...y`
#>   ON `...y`.`a` = `t1`.`a`
#> WHEN MATCHED THEN
#>   UPDATE SET `b` = `excluded`.`b`
#> WHEN NOT MATCHED THEN hi tim
#>   INSERT (`a`, `b`)
#>   VALUES (`...y`.`a`, `...y`.`b`)
#> ;

^{Created on 2023-05-22 with reprex v2.0.2}

I've found that the following changes to the sql_query_upsert.Oracle function in backend-oracle.R resolve the issue:

  parts <- rows_prep(con, table, from, by, lvl = 0)
  update_cols_esc <- sql(sql_escape_ident(con, update_cols))
  #update_values <- sql_table_prefix(con, update_cols, ident("excluded"))
  update_values <- sql_table_prefix(con, update_cols, from)
  update_clause <- sql(paste0(update_cols_esc, " = ", update_values))

  insert_cols <- c(by, update_cols)
  insert_cols_esc <- escape(ident(insert_cols), parens = FALSE, con = con)
  insert_values <- sql_table_prefix(con, insert_cols, ident("...y"))

  clauses <- list(
    sql_clause("MERGE INTO", table),
    sql_clause("USING", parts$from),
    #sql_clause_on(parts$where, lvl = 1),
    sql_clause("ON", parts$where, sep = " AND", lvl = 1, parens = TRUE),
    sql("WHEN MATCHED THEN"),
    sql_clause("UPDATE SET", update_clause, lvl = 1),
    sql("WHEN NOT MATCHED THEN"),
    sql_clause_insert(con, insert_cols_esc, lvl = 1),
    sql_clause("VALUES", insert_values, parens = TRUE, lvl = 1),
    sql_returning_cols(con, returning_cols, table),
    sql(";")
  )

with the above changes (updated lines commented out)
the reprex becomes:

library(dplyr)
#> 
#> Attaching package: 'dplyr'
#> The following objects are masked from 'package:stats':
#> 
#>     filter, lag
#> The following objects are masked from 'package:base':
#> 
#>     intersect, setdiff, setequal, union
library(dbplyr)
#> 
#> Attaching package: 'dbplyr'
#> The following objects are masked from 'package:dplyr':
#> 
#>     ident, sql

dbplyr::sql_query_upsert( con = simulate_oracle(),
                                        table = ident("t1"),
                                        from = ident("t2"),
                                        by = c("a"),
                                        update_cols = c("b")
)
#> <SQL> MERGE INTO `t1`
#> USING `t2` AS `...y`
#>   ON (`...y`.`a` = `t1`.`a`)
#> WHEN MATCHED THEN
#>   UPDATE SET `b` = `t2`.`b`
#> WHEN NOT MATCHED THEN
#>   INSERT (`a`, `b`)
#>   VALUES (`...y`.`a`, `...y`.`b`)
#> ;

^{Created on 2023-05-22 with reprex v2.0.2}

I'd be interested to submit a PR if you feel its warrented.

Cheers,
Tim

[TBlackmore]

After looking closer at the test-backend-oracle.R outputs I've revised my proposed changes:

@@ -83,7 +83,7 @@ sql_query_upsert.Oracle <- function(con,
   # https://oracle-base.com/articles/9i/merge-statement
   parts <- rows_prep(con, table, from, by, lvl = 0)
   update_cols_esc <- sql(sql_escape_ident(con, update_cols))
-  update_values <- sql_table_prefix(con, update_cols, ident("excluded"))
+  update_values <- sql_table_prefix(con, update_cols, ident("...y"))
   update_clause <- sql(paste0(update_cols_esc, " = ", update_values))
 
   insert_cols <- c(by, update_cols)
@@ -93,7 +93,7 @@ sql_query_upsert.Oracle <- function(con,
   clauses <- list(
     sql_clause("MERGE INTO", table),
     sql_clause("USING", parts$from),
-    sql_clause_on(parts$where, lvl = 1),
+    sql_clause("ON", parts$where, sep = " AND", lvl = 1, parens = TRUE),
     sql("WHEN MATCHED THEN"),
     sql_clause("UPDATE SET", update_clause, lvl = 1),
     sql("WHEN NOT MATCHED THEN"),

[mgirlich]

@TBlackmore Thanks for the info on how to fix the bug. I added a PR for it that mentions you in the NEWS to honour your contributions to the fix 😄