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

sql chunk handles statements well with "Knit" but poorly with "Run" #1637

Closed
itcarroll opened this issue Nov 15, 2018 · 15 comments
Closed

sql chunk handles statements well with "Knit" but poorly with "Run" #1637

itcarroll opened this issue Nov 15, 2018 · 15 comments
Milestone

Comments

@itcarroll
Copy link

@itcarroll itcarroll commented Nov 15, 2018

Knitr appropriately handles chunks using the sql engine for statements (i.e. INSERT, UPDATE, CREATE, etc.) differently from how it handles chunks making queries (i.e. SELECT): it does not call dbFetch. In RStudio however, when you click "Run" (or the play button on a specific sql chunk), you get ugly results and a warning:

In result_fetch(res@ptr, n = n) :
  Don't need to call dbFetch() for statements, only for queries

The following Rmd is a reproduces the warning, if you run all the chunks rather than knit.


---
editor_options: 
  chunk_output_type: inline
---

```{r}
library(RSQLite)
conn <- dbConnect(SQLite(), ":memory:")
```
```{sql, connection = conn}
CREATE TABLE test (
 id integer
)
```
```{sql, connection = conn}
SELECT * FROM test
```
```{r}
dbDisconnect(conn)
```

My system information:

> sessionInfo()
R version 3.5.0 (2018-04-23)
Platform: x86_64-apple-darwin15.6.0 (64-bit)
Running under: macOS High Sierra 10.13.6

Matrix products: default
BLAS: /System/Library/Frameworks/Accelerate.framework/Versions/A/Frameworks/vecLib.framework/Versions/A/libBLAS.dylib
LAPACK: /Library/Frameworks/R.framework/Versions/3.5/Resources/lib/libRlapack.dylib

locale:
[1] en_US.UTF-8/en_US.UTF-8/en_US.UTF-8/C/en_US.UTF-8/en_US.UTF-8

attached base packages:
[1] stats     graphics  grDevices utils     datasets  methods   base     

other attached packages:
[1] RSQLite_2.1.1

loaded via a namespace (and not attached):
 [1] Rcpp_1.0.0      digest_0.6.18   rprojroot_1.3-2 DBI_1.0.0       backports_1.1.2
 [6] evaluate_0.12   blob_1.1.1      rmarkdown_1.10  tools_3.5.0     bit64_0.9-7    
[11] bit_1.1-14      yaml_2.2.0      compiler_3.5.0  pkgconfig_2.0.2 memoise_1.1.0  
[16] htmltools_0.3.6 knitr_1.20
@yutannihilation
Copy link
Collaborator

@yutannihilation yutannihilation commented Nov 16, 2018

Thanks, confirmed.

Knitr appropriately handles chunks using the sql engine for statements

Unfortunately, it seems not. I guess it's just the warnings are not captured on knitting because it doesn't come from the chunks, but from knitr itself. The problem is here:

knitr/R/engine.R

Lines 550 to 558 in dc5ead7

# execute query -- when we are printing with an enforced max.print we
# use dbFetch so as to only pull down the required number of records
if (is.null(varname) && max.print > 0 && !is_sql_update_query(query)) {
res = DBI::dbSendQuery(conn, query)
data = DBI::dbFetch(res, n = max.print)
DBI::dbClearResult(res)
} else {
data = DBI::dbGetQuery(conn, query)
}

This probably should be something like below.

  if (is_sql_update_query(query)) {
    data = DBI::dbExecute(conn, query)
  } else if (is.null(varname) && max.print > 0) {
    res = DBI::dbSendQuery(conn, query)
    data = DBI::dbFetch(res, n = max.print)
    DBI::dbClearResult(res)
  } else {
    data = DBI::dbGetQuery(conn, query)
  }

@yihui
Copy link
Owner

@yihui yihui commented Nov 16, 2018

@yutannihilation I'm not an expert here, so please go ahead and push the fix to the master branch. No need to create a PR. Thanks!

@yutannihilation
Copy link
Collaborator

@yutannihilation yutannihilation commented Nov 17, 2018

Sure.

@yutannihilation
Copy link
Collaborator

@yutannihilation yutannihilation commented Nov 17, 2018

@yihui I've modified the code and installed locally, but it seems RStudio still uses the unmodified version. Do you know when and where RStudio captures knitr's codes?

This is what I see with debug(DBI::dbFetch). It says this function is in the namespace tools:rstudio, not in knitr namespace...

image

@yihui
Copy link
Owner

@yihui yihui commented Nov 17, 2018

Interesting. Did you restart R?

@yutannihilation
Copy link
Collaborator

@yutannihilation yutannihilation commented Nov 17, 2018

Yes, I even restarted RStudio, but no luck...

@yihui
Copy link
Owner

@yihui yihui commented Nov 21, 2018

That's weird. I cannot reproduce your issue. While applying your patch, I found I needed to patch the next if statement, too:

diff --git a/R/engine.R b/R/engine.R
index a6d1bd25..b57b760e 100644
--- a/R/engine.R
+++ b/R/engine.R
@@ -549,7 +549,9 @@ eng_sql = function(options) {

   # execute query -- when we are printing with an enforced max.print we
   # use dbFetch so as to only pull down the required number of records
-  if (is.null(varname) && max.print > 0 && !is_sql_update_query(query)) {
+  if (is_sql_update_query(query)) {
+    data = DBI::dbExecute(conn, query)
+  } else if (is.null(varname) && max.print > 0) {
     res = DBI::dbSendQuery(conn, query)
     data = DBI::dbFetch(res, n = max.print)
     DBI::dbClearResult(res)
@@ -558,7 +560,7 @@ eng_sql = function(options) {
   }

   # create output if needed (we have data and we aren't assigning it to a variable)
-  output = if (!is.null(data) && ncol(data) > 0 && is.null(varname)) capture.output({
+  output = if (length(dim(data)) == 2 && is.null(varname)) capture.output({

     # apply max.print to data
     display_data = if (max.print == -1) data else head(data, n = max.print)

This is because data can be 0 (returned from DBI::dbExecute()), and ncol(0) is NULL. Then if (NULL > 0) will signal an error.

@yihui yihui added this to the v1.21 milestone Nov 21, 2018
@yutannihilation
Copy link
Collaborator

@yutannihilation yutannihilation commented Nov 26, 2018

That's weird. I cannot reproduce your issue.

Really?? I cannot avoid reproducing this issue.... Here's what I did:

  1. Install the patched branch.
devtools::install_github("yutannihilation/knitr@test-notebook-sql-chunk")
  1. Close and relaunch RStudio.
  2. Open a new R Markdown file, and copy and paste the above Rmd codes.
  3. Run chunks by ▷ ("Run Current Chunk") buttons on the top right of the chunks.

And I still see the warning...:

image

patch the next if statement, too:

Thanks for pointing out! I got the error when I pushed knit button.

@yutannihilation
Copy link
Collaborator

@yutannihilation yutannihilation commented Dec 4, 2018

TIL: Run button is out of knitr's control and we need to fix RStudio...

https://community.rstudio.com/t/where-is-knitr-used-by-rstudio/19244/4

@yihui
Copy link
Owner

@yihui yihui commented Dec 4, 2018

Hmm. I didn't know that. So we'll have to fix both knitr and RStudio? If the patch above looks good to you, please feel free to push it to the master branch (I can do it, too). Thanks!

@yutannihilation
Copy link
Collaborator

@yutannihilation yutannihilation commented Dec 4, 2018

Sure, it looks good to me. Will do.

@yutannihilation
Copy link
Collaborator

@yutannihilation yutannihilation commented Dec 4, 2018

Done!

7b737bd

I'll send a PR to RStudio as well. May I keep this open until it's merged?

@yutannihilation
Copy link
Collaborator

@yutannihilation yutannihilation commented Dec 6, 2018

rstudio/rstudio#3991 is merged. So, this should be fixed in the next release of RStudio.

@yihui
Copy link
Owner

@yihui yihui commented Dec 6, 2018

Great. Thanks again!

yihui pushed a commit to SiriChandanaGoruganti/knitr that referenced this issue Dec 9, 2018
@github-actions
Copy link

@github-actions github-actions bot commented Nov 10, 2020

This old thread has been automatically locked. If you think you have found something related to this, please open a new issue by following the issue guide (https://yihui.org/issue/), and link to this old issue if necessary.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants