Add `rows_upsert.tbl_dbi()` #616

mgirlich · 2021-09-14T12:28:23Z

Together with rows_patch() this would complete the support for the rows_*() functions 😄

I only added SQLite, MariaDB and Postgres for now.
I used the ON CONFLICT clause. This only works when there also is a unique constraint but - at least for Postgres - has some advantages but also the disadvantage that the primary key sequence is increased.

krlmlr

Thanks, looks good.

The test failure looks weird, does it succeed locally?

What do we need unique indices for? Can you summarize the advantages of ON CONFLICT -- or why we shouldn't use MERGE here?

R/rows-db.R

tests/testthat/test-rows-db.R

mgirlich · 2021-09-16T06:23:52Z

What do we need unique indices for? Can you summarize the advantages of ON CONFLICT -- or why we shouldn't use MERGE here?

Upsert Support

According to Wikipedia the following Databases support MERGE:

Oracle Database
DB2
Teradata
EXASOL
Firebird (with some restrictions according to Wikipedia?)
CUBRID
H2
HSQLDB
MS SQL
Vectorwise
Apache Derby

Alternative in MySQL, SQLite, PostgreSQL

MySQL
- INSERT ... ON DUPLICATE KEY UPDATE -> join on PRIMARY KEY or UNIQUE constraints
- REPLACE INTO -> attempt insert, if it fails delete the row (if existing) and insert the new one
- Support for ignoring duplicate keys (insert only new rows)
SQLite
- INSERT OR REPLACE INTO -> similiar to MySQL
PostgreSQL
- INSERT INTO ... ON CONFLICT [ conflict_target ] conflict_action -> similar to MySQL

Other Databases

Apache Phoenix supports UPSERT VALUES[12] and UPSERT SELECT[13] syntax.
Spark SQL supports UPDATE SET * and INSERT * clauses in actions.[14]
Apache Impala supports UPSERT INTO ... SELECT.[15]

Merge

There are a couple of reason why Postgres doesn't support MERGE. One of them is issues with concurrency:

the SQL standard does not require that MERGE be atomic in the sense of atomically providing either an INSERT or UPDATE, and other products do not provide any such guarantees with their MERGE statement.

The biggest issue with INSERT INTO ... ON CONFLICT is that the typical ID field (i.e. an auto incrementing sequence) is increased by one for every row that one tries to insert, i.e. in particular also for existing rows that end up being updated. If you have frequent big upsert where most rows already exists this can blow up your ID quite a big and suddenly it has to be a bigint. Then the fun starts thanks to the terrible support of big integers in R...

Alternative for MySQL, SQLite, PostgreSQL

If there is no unique constraint one can build an upsert query like follows

WITH updated AS
( 
    UPDATE table
        SET ...
    FROM new_values
    WHERE ...
    RETURNING ...
)
INSERT INTO table
SELECT ...
FROM new_values
WHERE NOT EXISTS (
    SELECT 1 
    FROM updated
    WHERE ... -- if there is a primary key one can use it. Otherwise basically the same condition as in the CTE
)

Apart from a longer syntax this has concurrency issues (there are many SO threads on this).

--> What should be supported for these three databases? The official solution with ON CONFLICT? The old solution? Both?

mgirlich · 2021-09-16T06:28:57Z

The test failure looks weird, does it succeed locally?

It works when I add Sys.setenv(DM_TEST_SRC = "postgres") to helper-src.R but produces the same error without it. I'll have a look.

EDIT: There is an issue in waldo 0.3.0 which is already fixed in 0.3.1. See news. Though the test still fails but this time with more useful information.

mgirlich · 2021-09-16T06:48:09Z

There seems to be another issue for SQLite with temporary tables and RETURNING

test_sqlite <- function(temporary) {
  con <- RSQLite::dbConnect(RSQLite::SQLite(), ":memory:")
  
  if (temporary) {
    DBI::dbExecute(con, 'CREATE TEMPORARY TABLE test_frame_2021_09_14_11_22_08_18668_10 ("select" integer, "where" varchar(20), "exists" REAL);')
  } else {
    DBI::dbExecute(con, 'CREATE TABLE test_frame_2021_09_14_11_22_08_18668_10 ("select" integer, "where" varchar(20), "exists" REAL);')
  }
  DBI::dbExecute(con, "INSERT INTO test_frame_2021_09_14_11_22_08_18668_10 VALUES (1, 'a', 0.5);")
  DBI::dbExecute(con, "INSERT INTO test_frame_2021_09_14_11_22_08_18668_10 VALUES (2, 'b', 1.5);")
  DBI::dbExecute(con, "INSERT INTO test_frame_2021_09_14_11_22_08_18668_10 VALUES (3, NULL, 2.5);")
  DBI::dbExecute(con, 'CREATE UNIQUE INDEX unique_select_id ON test_frame_2021_09_14_11_22_08_18668_10 ("select");')
  
  DBI::dbExecute(con, 'CREATE TEMPORARY TABLE dbplyr_031 ("select" integer, "where" varchar(20));')
  DBI::dbExecute(con, "INSERT INTO dbplyr_031 VALUES (2, 'x');")
  DBI::dbExecute(con, "INSERT INTO dbplyr_031 VALUES (3, 'y');")
  DBI::dbExecute(con, "INSERT INTO dbplyr_031 VALUES (4, 'z');")
  
  result <- DBI::dbGetQuery(
    con,
    'WITH "...y"("select", "where") AS (
    SELECT *
    FROM "dbplyr_031"
    )
    INSERT INTO test_frame_2021_09_14_11_22_08_18668_10 ("select", "where")
    SELECT * FROM "...y"
    WHERE true
    ON CONFLICT ("select")
    DO UPDATE
    SET "where" = excluded."where"
    RETURNING *;'
  )
  
  print(result)
  
  DBI::dbRemoveTable(con, "test_frame_2021_09_14_11_22_08_18668_10")
}

test_sqlite(FALSE)
#>   select where exists
#> 1      2     x    1.5
#> 2      3     y    2.5
#> 3      4     z     NA
test_sqlite(TRUE)
#>   select where exists
#> 1      4     z     NA

^{Created on 2021-09-16 by the reprex package (v2.0.1)}

Conflicts: R/rows-db.R man/rows-db.Rd tests/testthat/_snaps/rows-db.md tests/testthat/test-rows-db.R

Conflicts: NAMESPACE

mgirlich · 2021-09-16T14:29:42Z

I don't really know why it doesn't work for MS SQL. Could someone please get rid of MS SQL?...

It works fine on sqliteonline

Microsoft SQL Server 2019 (RTM-CU9) (KB5000642) - 15.0.4102.2 (X64)
Jan 25 2021 20:16:12
Copyright (C) 2019 Microsoft Corporation
Express Edition (64-bit) on Linux (CentOS Linux 8)

but not on sqlfiddle

Microsoft SQL Server 2017 (RTM-CU2) (KB4052574) - 14.0.3008.27 (X64)
Nov 16 2017 10:00:49
Copyright (C) 2017 Microsoft Corporation
Express Edition (64-bit) on Linux (Ubuntu 16.04.3 LTS)

So probably it only works with MS SQL 2019.

krlmlr · 2021-09-16T14:33:12Z

I feel your pain with MSSQL. We keep it as a special case for entertainment.

The tests complain about a semicolon, can we pass it via DBI?

Is there an upstream bug report for SQLite, is it perhaps fixed already?

mgirlich · 2021-09-16T14:33:25Z

To me the syntax looks the same for 2017 and 2019.

mgirlich · 2021-09-16T14:38:02Z

Is there an upstream bug report for SQLite, is it perhaps fixed already?

I haven't created one and in a quick search I haven't seen one.

krlmlr · 2021-09-16T14:40:49Z

Can we repro using sqlite3 ?

I don't mind installing MSSQL 2019 on GitHub Actions, perhaps in parallel to MSSQL 2017. There is https://github.com/ankane/setup-sqlserver that can help with that, or perhaps install manually.

mgirlich · 2021-09-16T14:44:31Z

Can we repro using sqlite3 ?

See my comment above

I don't mind installing MSSQL 2019 on GitHub Actions, perhaps in parallel to MSSQL 2017. There is https://github.com/ankane/setup-sqlserver that can help with that, or perhaps install manually.

You can if you want to. I don't care so much about MS SQL itself. I mainly implemented it because it was one of the databases with support for MERGE.

krlmlr · 2021-09-16T15:29:38Z

Yeah, let's skip the test for now with a comment.

mgirlich · 2021-09-17T07:09:46Z

@krlmlr Do you know why it fails for MS SQL?

Error (test-dplyr-src.R:34:3): 'copy_to.dm()' works
Error: Error: promise already under evaluation: recursive default argument reference or earlier problems?
Backtrace:

dm:::test_src_frame(!!!mtcars)
dbplyr:::copy_to.src_sql(...)
dbplyr:::db_copy_to.DBIConnection(...)
dbplyr:::create_indexes(con, table, unique_indexes, unique = TRUE)

mgirlich · 2021-09-20T07:07:22Z

The code coverage check fails because

currently no tests are run for Maria DB. Do you want to add them to the workflow?
upsert tests are skipped for MS SQL -> I remove them from the coverage with #nocov
no database in the workflows use sql_rows_upsert.tbl_sql() -> I remove them from the coverage with #nocov

* test error with DuckDB instead of skipping * remove `sql_rows_upsert()` for MS SQL from coverage * remove `sql_rows_upsert()` for `tbl_sql` from coverage

krlmlr · 2021-09-25T18:03:29Z

I resolved the conflict by keeping both changes. At some point the new _prep() function should be integrated with sql_rows_prep() from #624?

krlmlr

Thanks! Looks like we no longer need sql_rows_update_prep() -- I'll clean up later.

Add rows_upsert.tbl_dbi()

ac59dab

krlmlr reviewed Sep 16, 2021

View reviewed changes

R/rows-db.R Show resolved Hide resolved

tests/testthat/test-rows-db.R Outdated Show resolved Hide resolved

mgirlich added 5 commits September 16, 2021 08:04

Workaround SQLite bug with RETURNING

5db9fcd

Support MS SQL in sql_rows_upsert()

fc7b29b

Merge commit '4ff3b2cc41f92181641775ad05bce71cb1e4f329'

108f2fd

Conflicts: R/rows-db.R man/rows-db.Rd tests/testthat/_snaps/rows-db.md tests/testthat/test-rows-db.R

Merge commit 'f01cfda72fd4633da8504f958f0d05ae81ddc3ce'

ccd288a

Conflicts: NAMESPACE

Improve SQLite detection for workaround

1d61fae

mgirlich added 2 commits September 16, 2021 14:36

Forgot to commit function is_my_test_src_sqlite()

6f19d89

Add semicolon for MS SQL and pray that it works

dad2d54

krlmlr enabled auto-merge September 16, 2021 15:30

Skip upsert tests for MS SQL

22e5d69

auto-merge was automatically disabled September 17, 2021 06:52
Head branch was pushed to by a user without write access

Fix argument .unique_indexes in test_src_frame()

0dda9fe

mgirlich and others added 3 commits September 20, 2021 07:10

Increase test coverage

11f61f6

* test error with DuckDB instead of skipping * remove `sql_rows_upsert()` for MS SQL from coverage * remove `sql_rows_upsert()` for `tbl_sql` from coverage

Remove outdated comment

7c28f64

Merge branch 'main' into rows_upsert

0496b24

krlmlr approved these changes Sep 26, 2021

View reviewed changes

krlmlr merged commit 6fd3206 into cynkra:main Sep 26, 2021

mgirlich deleted the rows_upsert branch September 28, 2021 08:00

mgirlich mentioned this pull request Jan 11, 2022

Add rows_*() verbs tidyverse/dbplyr#748

Merged

github-actions bot locked as resolved and limited conversation to collaborators Sep 29, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `rows_upsert.tbl_dbi()` #616

Add `rows_upsert.tbl_dbi()` #616

mgirlich commented Sep 14, 2021

krlmlr left a comment

mgirlich commented Sep 16, 2021 •

edited

Loading

mgirlich commented Sep 16, 2021 •

edited

Loading

mgirlich commented Sep 16, 2021

mgirlich commented Sep 16, 2021

krlmlr commented Sep 16, 2021

mgirlich commented Sep 16, 2021

mgirlich commented Sep 16, 2021

krlmlr commented Sep 16, 2021

mgirlich commented Sep 16, 2021

krlmlr commented Sep 16, 2021

mgirlich commented Sep 17, 2021

mgirlich commented Sep 20, 2021

krlmlr commented Sep 25, 2021

krlmlr left a comment

Add rows_upsert.tbl_dbi() #616

Add rows_upsert.tbl_dbi() #616

Conversation

mgirlich commented Sep 14, 2021

krlmlr left a comment

Choose a reason for hiding this comment

mgirlich commented Sep 16, 2021 • edited Loading

Upsert Support

Merge

Alternative for MySQL, SQLite, PostgreSQL

mgirlich commented Sep 16, 2021 • edited Loading

mgirlich commented Sep 16, 2021

mgirlich commented Sep 16, 2021

krlmlr commented Sep 16, 2021

mgirlich commented Sep 16, 2021

mgirlich commented Sep 16, 2021

krlmlr commented Sep 16, 2021

mgirlich commented Sep 16, 2021

krlmlr commented Sep 16, 2021

mgirlich commented Sep 17, 2021

mgirlich commented Sep 20, 2021

krlmlr commented Sep 25, 2021

krlmlr left a comment

Choose a reason for hiding this comment

Add `rows_upsert.tbl_dbi()` #616

Add `rows_upsert.tbl_dbi()` #616

mgirlich commented Sep 16, 2021 •

edited

Loading

mgirlich commented Sep 16, 2021 •

edited

Loading