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

Add rows_*() verbs #748

Merged
merged 103 commits into from
Apr 30, 2022
Merged

Add rows_*() verbs #748

merged 103 commits into from
Apr 30, 2022

Conversation

mgirlich
Copy link
Collaborator

@mgirlich mgirlich commented Jan 11, 2022

Closes #736.

returning: This argument is still experimental and not (yet?) in the generic. To support tidyselect and not trigger a warning by rlang::check_dots_used() (in the generic) the user has to quote it manually (see tidyverse/dplyr#5985). (Edit: not necessary anymore after check_dots_used() was removed in the generic).
Theoretically, one could execute arbitrary SQL in the RETURNING clause but I'm not sure whether there are sufficient use cases to support this.

nonexistent: The current implementation is nonexistent = "ignore". To support nonexistent = "error" one could use a foreign key constraint on y. I'll tackle this in another PR. (Update: renamed to unmatched and "error" is not supported for now)

rows_upsert(): This is the most tricky and needs different methods:

  • Some databases (e.g. Oracle or MS SQL) support MERGE INTO ... WHEN (NOT) MATCHED
  • Some databases (e.g. SQLite, Postgres) have an upsert if there is a unique constraint, basically INSERT ... ON CONFLICT DO UPDATE/NOTHING
  • If there is no unique constraint this can still be done in one query with a CTE (e.g. Postgres) or two queries (e.g. SQLite)

It might make sense to add an argument so that the user can choose whether a unique constraint exists or not. See
cynkra/dm#616 (comment) for some more information on the Pros/Cons. I will do this in another PR.

@hadley hadley added this to the 2.2.0 milestone Apr 28, 2022
@krlmlr
Copy link
Member

krlmlr commented Apr 29, 2022

Are we keeping the returning argument, or do we introduce new rows_*_returning() functions?

I saw that the default for in_place has changed to FALSE, I guess this means we no longer give a message if in_place is omitted?

Regarding integration with dm, I think we can release dbplyr first and then adapt dm -- the dbplyr update doesn't break dm's tests, because the latter still overwrites the methods. Should dbplyr give a warning mentioning that the rows_*() API behaves differently when dm <= 0.2.8 is loaded?

It looks like the SQL Server implementation needs more work, I'll propose something today. Postgres looks fine.

@krlmlr
Copy link
Member

krlmlr commented Apr 29, 2022

MSSQL seems good now.

@mgirlich
Copy link
Collaborator Author

Are we keeping the returning argument, or do we introduce new rows_*_returning() functions?

I'm okay with both. Do you have a preference?

I saw that the default for in_place has changed to FALSE, I guess this means we no longer give a message if in_place is omitted?

Yes, I don't see a need to give a message.

Regarding integration with dm, I think we can release dbplyr first and then adapt dm -- the dbplyr update doesn't break dm's tests, because the latter still overwrites the methods. Should dbplyr give a warning mentioning that the rows_*() API behaves differently when dm <= 0.2.8 is loaded?

Feel free to add a message though it will probably affect only few users.

MSSQL seems good now.

Thanks!

Copy link
Member

@krlmlr krlmlr left a comment

Choose a reason for hiding this comment

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

Thanks! Looks good to me, with cynkra/dm#988 we get the new behavior also for the legacy code in dm. I can release dm before or shortly after dbplyr, doesn't really matter.

@krlmlr
Copy link
Member

krlmlr commented Apr 29, 2022

So, let's keep the returning argument and avoid adding new messages.

@mgirlich mgirlich merged commit 7e348c2 into main Apr 30, 2022
@mgirlich mgirlich deleted the rows-update branch April 30, 2022 08:48
@mgirlich mgirlich mentioned this pull request Apr 30, 2022
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.

Add rows_*.tbl_dbi() methods
3 participants