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 support for primary keys in dbCreateTable() and sqlCreateTable() #351

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

Conversation

krlmlr
Copy link
Member

@krlmlr krlmlr commented Apr 26, 2021

This is important, because it is difficult and computationally intensive (and sometimes impossible) to change the primary key after creating the table.

Should we also support foreign keys and unique constraints?

This is a blocker for cynkra/dm#3. We would also need to adapt dbplyr::copy_to() and perhaps dbplyr::compute().

@krlmlr krlmlr requested a review from hadley April 26, 2021 15:54
field_names <- dbQuoteIdentifier(con, names(fields))
field_types <- unname(fields)
fields <- paste0(field_names, " ", field_types)

SQL(paste0(
"CREATE ", if (temporary) "TEMPORARY ", "TABLE ", table, " (\n",
" ", paste(fields, collapse = ",\n "), "\n)\n"
" ", paste0(fields, nullable, collapse = ",\n "),
if (!is.null(pk)) paste0(",\n PRIMARY KEY (", paste(dbQuoteIdentifier(con, pk), collapse = ", "), ")"),
Copy link
Member

Choose a reason for hiding this comment

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

How standard is this syntax?

Copy link
Member Author

Choose a reason for hiding this comment

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

Pretty much, I think, but I don't know. We'll find out soon enough, we can override in backends -- this is a method.

R/table-create.R Outdated
@@ -24,6 +24,10 @@ NULL
#' @param temporary If `TRUE`, will generate a temporary table statement.
#' @inheritParams rownames
#' @param ... Other arguments used by individual methods.
#' @param pk Primary key columns.
Copy link
Member

Choose a reason for hiding this comment

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

This seems too short compared to other argument names

Copy link
Member Author

Choose a reason for hiding this comment

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

primary.key? I'm not sure this package already uses underscores for argument names :-\

R/table-create.R Outdated Show resolved Hide resolved
Co-authored-by: Hadley Wickham <[email protected]>
@pnacht
Copy link

pnacht commented May 6, 2021

As I understand it, DBI tries to be SQL 99 compliant. If so, I believe the standard syntax is in fact

CREATE TABLE {tableName} (
	{column Definitions}, 
	CONSTRAINT {PK_Name} PRIMARY KEY ({column Names}) 
) 

Per W3Schools, the PRIMARY KEY (x) syntax is only valid for MySQL.

@vnijs
Copy link

vnijs commented Aug 21, 2022

I was looking for a way to add a primary key when using dbCreateTable() or dbWriteTable(). Specifically, updating a row if a key is present or adding a row if not, using the below requires a constraint or primary key. Is there still a plan to add the option in this PR? Would be very convenient. Thanks

upsert_str <- glue::glue("INSERT INTO student_data ({vars_str}) VALUES ({values_str}) ON CONFLICT({key_str}) DO UPDATE SET {set_str}")
dbExecute(pool, upsert_str)

@krlmlr
Copy link
Member Author

krlmlr commented Aug 21, 2022

@vnijs: The dm package supports this via copy_dm_to(set_key_constraints = TRUE) : https://cynkra.github.io/dm/articles/howto-dm-copy.html#copy-model. The documentation doesn't mention this explicitly, it should.

@vnijs
Copy link

vnijs commented Aug 21, 2022

@krlmlr dm looks like an excellent package! Thanks for the reference. The below worked, where db is the connection made using DBI.

dm_sdata <- dm(pre_student_data = pre_sdata, student_data = sdata) %>%
  dm_add_pk(pre_student_data, user_name) %>%
  dm_add_pk(student_data, user_name)

## create sqlite dbase in site directory
copy_dm_to(db, dm_sdata, temporary = FALSE)

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.

4 participants