Support returning in rows_*.tbl_dbi()

[mgirlich]

Closes #593.

• No tests yet. For SQLite tests are blocked by dbGetQuery() returns no columns for INSERT ... RETURNING on temporary tables r-dbi/RSQLite#380 and hindered by RETURNING produces wrong column name r-dbi/RSQLite#381.
• You might prefer a different code style. For convenience I used the dbplyr functions ident() and escape()
• The generic dplyr::rows_update() doesn't have an argument returning but now rows_update.tbl_dbi() has it. To use the tidyselect interface I enquo(). This doesn't work with the check_dots_used() used in the generic.

[krlmlr]

Thanks. Should we discuss conditional updates and mutating joins separately?

[krlmlr]

Thanks, this is a good start.

I think type stability for the return values is really important here. Can we return the inserted rows in an attribute to the returned table?

[krlmlr]

Please also run devtools::document() to avoid check errors.

[mgirlich]

I think type stability for the return values is really important here. Can we return the inserted rows in an attribute to the returned table?

You mean something like

returned_rows <- dbGetQuery(con, sql, immediate = TRUE)
attr(x, "returned_rows") <- returned_rows
invisible(x)

and then have a helper function e.g. get_returned_rows(x) to get the returned rows?

I can do that and I think type stability is important. On the other hand it also seems a bit counterintuitive to me. The x argument can only be a table that wasn't modified (otherwise remote_table() doesn't work and also doesn't really make sense). And using returning seems to be a very good indicator that one wants to use the result.

In case you have a pipeline of insert/update/upsert operations in mind, i.e.

x %>% 
  dm::rows_insert(y, returning = "id") %>% 
  dm::rows_insert(y2, returning = "id") %>% 
  dm::rows_update(z, returning = "id")

it might make sense that the user could choose the name of the attribute so that all results from returning can still be accessed.

[krlmlr]

Thanks. Can we simplify further?

[krlmlr]

Do we really need to pass returning to the sql_() methods?

Suggested change

	sql <- sql_rows_insert(x, y, returning = returning)
	sql <- paste0(
	sql_rows_insert(x, y, returning = returning)
	if (!is.null(returning)) paste0("\n", sql_returning(...))
	)

[mgirlich]

If returning is not an argument of the sql_() methods then I would rather have something like

sql_rows_insert(x, y, returning = returning) %>%
  sql_add_returning(x, returning)

and export sql_add_returning() (this should be exported to complete the other sql functions). To me this feels like a more user-friendly interface. What do you think?

[krlmlr]

I think we need an sql_returning() generic anyway -- to special-case both the clause and the way column expressions are constructed (to work around that RSQLite bug). I don't mind sql_add_returning() as an equivalent to the paste0() call in my proposal.

[krlmlr]

Thanks, the attribute looks good.

Regarding stacking of "returning" data, I think it's dangerous to use the pipe for functions that have side effects:

x %>% 
  dm::rows_insert(y, returning = "id", in_place = TRUE) %>% 
  dm::rows_insert(y2, returning = "id", in_place = TRUE) %>% 
  dm::rows_update(z, returning = "id", in_place = TRUE)

If the first call succeeds but the second call fails, you might be tempted to re-run the pipe, which might now also fail the first call. It's safer to un-pipe this and consume return values implicitly. I don't think we need to act here.

[krlmlr]

Thanks, a few final nitpicks.

For tests of this feature, we can use persistent tables, referring to the blocking issue. If you could add them, I'll start the CI run and merge.

[mgirlich]

For tests of this feature, we can use persistent tables, referring to the blocking issue. If you could add them, I'll start the CI run and merge.

I think it should be sufficient to rely on the dev version of RSQLite as the bug is fixed in there.

[krlmlr]

RSQLite 2.2.8 is on CRAN now.

[krlmlr]

Thanks, looks great! Running checks now, one last question.

[krlmlr]

We need has_returned_rows() in pkgdown: https://github.com/cynkra/dm/runs/3428475709#step:17:55.

[krlmlr]

Thanks, this is huge! Looking forward to using it.

[krlmlr]

I love SQL Server...

[krlmlr]

Thanks!