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.

Sign up for GitHub

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

Jump to bottom

Improved SQL dbExecute vs dbGetQuery #1896

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Improved SQL dbExecute vs dbGetQuery #1896

wants to merge 4 commits into from

Conversation

rnorberg
Copy link

Adds additional SQL "update" keywords (ALTER, GRANT, MERGE, etc) to is_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 to DBI::dbExecute or DBI::dbGetQuery via the new sql.is_update_query chunk option.

@CLAassistant
Copy link

CLAassistant commented Sep 22, 2020 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@rnorberg rnorberg mentioned this pull request Sep 29, 2020
Closed
@dpprdan
Copy link
Contributor

dpprdan commented Dec 5, 2022

I've also run into this issue.

Details

I.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

  1. The chunk option should be called something more "formal" e.g. sql.is_statement (is_update_query is a contradiction IIUC, since a statement with UPDATE (usually) does not return a result set, so it isn't a query).
  2. For PostgreSQL support I'd add SET, REFRESH and VACUUM as keywords. (But this isn't strictly necessary with the sql.is_* option).

NB: It's possible to have SELECT statements that are not queries (e.g. SELECT INTO), as well as statements with a keyword that are queries (e.g. UPDATE [...] RETURNING), which makes the sql.is_* option the most important element of this PR.

@cderv do you have time to look into this?

Copy link
Collaborator

Dec 6, 2022 •
edited
Loading

@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.

@dpprdan
Copy link
Contributor

dpprdan commented Dec 6, 2022

@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.

Copy link
Owner

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).

  1. The chunk option should be called something more "formal" e.g. sql.is_statement

I think that's a great suggestion.

next Issues/PRs considered for the next release label Dec 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

cderv

Labels
next Issues/PRs considered for the next release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@rnorberg @CLAassistant @dpprdan