Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

produce output for UPDATE-like queries using sql engine #2050

Open
3 tasks done
edalfon opened this issue Sep 24, 2021 · 3 comments · May be fixed by #2051
Open
3 tasks done

produce output for UPDATE-like queries using sql engine #2050

edalfon opened this issue Sep 24, 2021 · 3 comments · May be fixed by #2051

Comments

@edalfon
Copy link

edalfon commented Sep 24, 2021

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

  • I have fully read the issue guide at https://yihui.org/issue/.
  • 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.
  • 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.

@yihui
Copy link
Owner

yihui commented Sep 24, 2021

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!

@edalfon edalfon linked a pull request Sep 25, 2021 that will close this issue
@edalfon
Copy link
Author

edalfon commented Sep 25, 2021

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 edalfon closed this as completed Sep 25, 2021
@cderv
Copy link
Collaborator

cderv commented Sep 27, 2021

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

@cderv cderv reopened this Sep 27, 2021
@cderv cderv linked a pull request Sep 27, 2021 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants