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

Fix the issue with SQL chunk when cache=TRUE and output.var is specified #1542

Closed
wants to merge 1 commit into from

Conversation

yutannihilation
Copy link
Collaborator

@yutannihilation yutannihilation commented May 9, 2018

fixes rstudio/rmarkdown#914

IIUC, SQL chunk is special in the sense the result of it can be either a text output or a data.frame, depending on whether output.var is set or not. Since both can be saved as an R object, it may be possible to use the same cache file. But, for simplicity and consistency with other engines (non-R engines seem to have their own cache file other than one for text outputs), I implemented to cache using the separate file, suffixed _sql_result.rds.

Copy link
Owner

@yihui yihui left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Actually I just realized that there could be a much easier fix:

knitr/R/block.R

Line 114 in 4b64980

if (options$engine == 'stan') options$engine.opts$x else character(0)

I think we can just change it to

switch(
  options$engine,
  'stan' = options$engine.opts$x, 'sql' = options$output.var
)

Can you test that?

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

Thanks, confirmed! It looks perfect. I didn't understand the way how object is passed to cache.

Since this PR seems superseded by your idea, I'm closing this. I'll try to achieve some actual contribution next time! 💪

@yihui
Copy link
Owner

yihui commented May 10, 2018

Please go ahead to submit another PR with my approach above. Thanks! :)

@yutannihilation yutannihilation deleted the cache-eng-sql branch May 10, 2018 21:50
@yutannihilation
Copy link
Collaborator Author

Hmm, generally, I rather hesitate to be the author of other people's idea... But, if you say so, I will. Thanks for always trying to educate me :)

yutannihilation added a commit to yutannihilation/knitr that referenced this pull request May 10, 2018
the original author of this idea is @yihui: yihui#1542 (review)
yutannihilation added a commit to yutannihilation/knitr that referenced this pull request May 10, 2018
the original author of this idea is @yihui: yihui#1542 (review)
@yihui
Copy link
Owner

yihui commented May 10, 2018

Haha, actually I almost merged this PR, and I suddenly remembered I did something special for the stan engine. It turned out that the sql engine could use the same idea.

yihui pushed a commit that referenced this pull request May 11, 2018
* cache sql result

the original author of this idea is @yihui: #1542 (review)

* add a NEWS item
@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

Successfully merging this pull request may close these issues.

Code folding and cache = TRUE do not work for SQL engine
2 participants