-
-
Notifications
You must be signed in to change notification settings - Fork 878
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
Comments
I briefly looked at the two commits, and they look good to me. I'm not the original author of this |
Great!, many thanks. I've just sent the PR |
@edalfon let's leave the issue opened and close only PR is merged. That is our usual process. Thanks! |
The issue:
knitr
does not produce any output for UPDATE-like queries using the sql engine. Nor pass it any data tooutput.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 asI'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 withknitr
'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 todata
instead of discarding it and settingdata = NULL
, as it currently doesknitr
's main branch.From
?DBI::dbExecute
you have thatso 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.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 inDBI::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
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')
.I understand that my issue may be closed if I don't fulfill my promises.
The text was updated successfully, but these errors were encountered: