-
-
Notifications
You must be signed in to change notification settings - Fork 872
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
Improved SQL dbExecute vs dbGetQuery #1896
base: master
Are you sure you want to change the base?
Conversation
… sql.is_update_query chunk option
I've also run into this issue. DetailsI.e. the DB driver emits a warning, when you have a SQL statement without one of the current keywords inside a sql chunk. E.g. RPostgres emits In result_fetch(res@ptr, n = n) :
Don't need to call dbFetch() for statements, only for queries Other driver may emit warnings with different wordings, but they are essentially the same, e.g. RSQLite. I therefore support this PR. However, I would suggest two changes
NB: It's possible to have @cderv do you have time to look into this? |
@dpprdan thanks for pinging on this issue. I believe this was on long hold due to r-dbi/DBI#321 (comment) - It would be better for the long term that the SQL engine for knitr leaves elsewhere than inside knitr. However, it seems the DBI is not a good place. Engine can be in their own package and SQL feature is not core of knitr directly. I think this could make sense to have an improved and new engine in its own package now, and that it is maintained with the help of SQL users and expert. What do you think ? I'll run this project idea internally too, I think we can start that in 2023. |
@cderv That sounds very reasonable. I can see myself in "with the help of SQL users", so feel free to ping me if you think I can assist somewhere. You probably thought of this already, but it may make sense to consider the glue_sql engine here as well. |
Since we have gotten another vote from users, I can definitely consider merging this PR (since it doesn't appear to be too complicated). In the long run, I still think it will be great if we have one or two helpers who could have some soft commitment to maintain this engine in knitr (it may not require a lot of effort, and Posit/RStudio can also send gifts as incentives if necessary).
I think that's a great suggestion. |
Adds additional SQL "update" keywords (
ALTER
,GRANT
,MERGE
, etc) tois_sql_update_query
(which is a package internal function and not exported) and allow users to force a query contained in a SQL chunk to be dispatched toDBI::dbExecute
orDBI::dbGetQuery
via the newsql.is_update_query
chunk option.