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

Let additional arguments pass from db_compute.DBIConnection() to dbplyr_save_query() #1037

Merged
merged 4 commits into from
Dec 21, 2023

Conversation

rsund
Copy link
Contributor

@rsund rsund commented Oct 31, 2022

Tailored backends may want to support additional arguments in compute(). Unfortunately db_compute.DBIConnection is not passing all arguments to dbplyr_save_query() and therefore additional arguments never reach the sql_query_save() -method.

For example, overwrite argument is not supported in dbplyr (#488). Adding it to tailored backend is more complicated than necessary as additional arguments are not passed through the whole chain from compute() to sql_query_save-method (see duckdb/duckdb#5115).

Fix seems to be easy and non harmful. If that is not the case and/or there is a reason for not letting arguments pass through, please close this PR.

@mgirlich
Copy link
Collaborator

On the one hand this makes sense on the other you kind of take away the possibility to use the dots for db_compute() methods.
@hadley What do you think?

@mgirlich mgirlich requested a review from hadley November 15, 2022 08:59
@hadley
Copy link
Member

hadley commented Nov 15, 2022

My main concern is that it's not at all obvious where those dots go, and what they match against. That makes the risk of unintended consequences high and makes it difficult to document the behaviour.

@hadley
Copy link
Member

hadley commented Dec 21, 2023

OTOH there's no other place for ... to go here, and adding it seems generally harmless.

@hadley hadley merged commit 3ab0ebe into tidyverse:main Dec 21, 2023
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants