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

Name collision in compute(temporary = FALSE) #1154

Closed
bairdj opened this issue Feb 13, 2023 · 4 comments · Fixed by #1265
Closed

Name collision in compute(temporary = FALSE) #1154

bairdj opened this issue Feb 13, 2023 · 4 comments · Fixed by #1265

Comments

@bairdj
Copy link
Contributor

bairdj commented Feb 13, 2023

By default, calling compute creates a table with name "dbplyr_n". Although temporary tables are isolated between sessions, if a user calls temporary = FALSE without explicitly setting a name, then the table is persisted using the dbplyr_ name. Other users using dbplyr on the same database can then experience collisions when calling compute as the generated table name already exists. Best practice would be to avoid persisting these temp tables without specifying a name, but it can happen.

Possible solutions:

  1. Allow user to set an option to replace the default "dbplyr_" prefix with something unique to them
  2. Don't allow user to set temporary = FALSE without specifying a table name
  3. Check if generated table name exists before trying to create it, and create another one if not
  4. Use a suffix of random letters instead of sequential numbers

Option 1 relies on the user to set the option, and they could still potentially collide with their own tables.
Option 2 should stop the problem at the root, but would be a breaking change.
Option 3 is undesirable IMO due to extra call to DB, and theoretically could result in many calls if all generated names were taken.
Option 4 would be a reasonable solution and miniscule probability of collisions

@mgirlich
Copy link
Collaborator

I prefer option 2, even though it is a breaking change.
@hadley What's your opinion here/

@hadley
Copy link
Member

hadley commented Apr 25, 2023

Most temporary tables are session specific. What databases is in use here?

@mgirlich
Copy link
Collaborator

@hadley The title is misleading here. As I understand it, the issue is about compute(temporary = FALSE).

@mgirlich mgirlich changed the title Temporary table name collisions Name collision in compute(temporary = FALSE) Apr 25, 2023
@hadley
Copy link
Member

hadley commented Apr 25, 2023

Oooh, got it. In that case, option two sounds right to me. To avoid breaking existing code, we could start by making this a warning, rather than an error.

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 a pull request may close this issue.

3 participants