knitr icon indicating copy to clipboard operation
knitr copied to clipboard

produce output for UPDATE-like queries using sql engine

Open edalfon opened this issue 3 years ago • 3 comments

The issue: knitr does not produce any output for UPDATE-like queries using the sql engine. Nor pass it any data to output.var, when this is set.

knitr's sql engine was a game-changer for me because it enables a smooth integration of SQL-based workflows in R. But since it was launched, I've missed getting feedback for UPDATE-like queries. I guess I am not alone in this, because anyone that has worked with popular RDBMS has probably seen a nicely informative message such as

09:51:47 Number of affected rows: 10

I've been slightly frustrated by the lack of feedback for UPDATE-like queries in .Rmd, but not so much to do something about it. But googling something else, I ended up reading issue #1637 and I figured it'd be relatively straightforward to add such support. So I went ahead, forked this repo, tweaked two things in R/engine.R and voila, it works! (but I am unfamiliar with knitr's code base, so I didn't dare to create a PR).

Could you take a look and see if this is something you would merge into knitr?

Working solution is in this fork github.com/edalfon/knitr and I informally tested it in this public project on RStudio Cloud rstudio.cloud/project/2931829 (just a new clean project, freshly installed packages, but using the forked version of knitr -remotes::install_github("edalfon/knitr")- and it has a .Rmd file).

The forked version has just two changes:

First commit just takes the return value of DBI::dbExecute(conn, query) and assign it to data instead of discarding it and setting data = NULL, as it currently does knitr's main branch.

From ?DBI::dbExecute you have that

dbExecute() always returns a scalar numeric that specifies the number of rows affected by the statement.

so data will simply be a numeric of length 1

This change does not produce any output yet, but you can already set output.var for the sql chunk and get the number of rows affected.

```{sql, connection=db, output.var="rows_affected"}
CREATE TABLE toy AS
SELECT cyl, COUNT(*) AS cnt 
FROM mtcars
GROUP BY cyl
;```

The second commit deals with producing a default output (simply a string indicating the number of rows and formatting the number. ..., and forcing always options$results = 'asis').

In this public project on RStudio Cloud rstudio.cloud/project/2931829 I tested both, the default output and passing the number of rows affected to a variable using output.var. Please note it uses DuckDB because it seems SQLite does not actually return the number of rows affected (apparently always returns 0). Yet, this is pretty standard in most RDBMS and it is also the default in DBI::dbExecute, so I guess this is just a quirk specific to SQLite. I also tested it locally using PostgreSQL and Microsoft SQL Server and found no hiccups.

It all works smoothly when you knit the .Rmd, but not when you use RStudio to manually run the chunk. I guess a similar change would be needed in RStudio's side, as it was the case in the aforementioned issue (#1637).

So, again, it seems all pretty straightforward. But I am not aware of potential issues downstream, 'knitr' testing approach, etc., so I preferred to open this issue to see if this is viable/interesting, instead of creating a PR right away.


By filing an issue to this repo, I promise that

  • [x] I have fully read the issue guide at https://yihui.org/issue/.
  • [x] I have provided the necessary information about my issue.
    • If I'm asking a question, I have already asked it on Stack Overflow or RStudio Community, waited for at least 24 hours, and included a link to my question there.
    • If I'm filing a bug report, I have included a minimal, self-contained, and reproducible example, and have also included xfun::session_info('knitr'). I have upgraded all my packages to their latest versions (e.g., R, RStudio, and R packages), and also tried the development version: remotes::install_github('yihui/knitr').
    • If I have posted the same issue elsewhere, I have also mentioned it in this issue.
  • [x] I have learned the Github Markdown syntax, and formatted my issue correctly.

I understand that my issue may be closed if I don't fulfill my promises.

edalfon avatar Sep 24 '21 13:09 edalfon

I briefly looked at the two commits, and they look good to me. I'm not the original author of this sql engine, though, and I'm not really familiar with the SQL stuff in R. I tend to just merge the two commits, but I may need someone else to take a look before I do that. Please feel free to send a PR. Thank you very much!

yihui avatar Sep 24 '21 18:09 yihui

I briefly looked at the two commits, and they look good to me. I'm not the original author of this sql engine, though, and I'm not really familiar with the SQL stuff in R. I tend to just merge the two commits, but I may need someone else to take a look before I do that. Please feel free to send a PR. Thank you very much!

Great!, many thanks. I've just sent the PR

edalfon avatar Sep 25 '21 22:09 edalfon

@edalfon let's leave the issue opened and close only PR is merged. That is our usual process. Thanks!

cderv avatar Sep 27 '21 09:09 cderv