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

Overhaul the table identification system #93

Merged
merged 35 commits into from
Jan 31, 2024
Merged

Conversation

RasmusSkytte
Copy link
Contributor

@RasmusSkytte RasmusSkytte commented Jan 29, 2024

Intent

This PR introduces a number of changes -- including breaking -- to the systems using id().

I was experiencing problems with using a number of the SCDB functions programmatically over in diseasystore.
Which also led to me creating #92, since, in my opinion, table_exists() did not function intuitively as I expected.

When investigating this issue, I found several "design issues" as I would call them that this PR attempts to rectify.

The overall issue stems, in my view, from our table identification being too ambiguous. Specifically, I do not believe we live up to the mantra: Explicit is better than implicit

This PR therefore attempts to remove all ambiguities related to table identification.

Fixes #92

Approach

A lot of things are being done in this PR.

Here I list the changes as written in the NEWS:

BREAKING CHANGES:

  • Table identification is now more specific (#??):

    Most SCDB functions allow for tables to be specified by a character representation of "schema.table".

    Before, if no schema was implied in this context, SCDB would attempt to match the table among both
    permanent and temporary tables.

    Now, it will always assume that a lack of schema means the default schema should be used.
    This is also the case if DBI::Id() is used without a schema specification.

  • The show_temporary argument of get_tables() is now a simple logical (#??).

    In addition, schema is always returned in the list of table (no longer NA for default schema).

  • Tables created with create_table() will now be temporary or permanent dependent on the default value of
    DBI::dbCreateTable() (#??).

    If you wish to overwrite this, use ... arguments which are passed to DBI::dbCreateTable().

  • If a SQLiteConnection is passed to get_schema(), the returned schema will always be "main" (#??).

Features

  • The S3 method as.character.Id() is added which converts DBI::Id() to character (#??).

Improvements and Fixes

  • Improvements for create_table() (#??):

    • now writes the table if a remote connection is given. Before, it would only create the
      table with corresponding columns.
    • can now create temporary tables for Microsoft SQL Server.
  • get_tables() now supports temporary tables for Microsoft SQL Server (#??).

Testing

Added missing tests for create_logs_if_missing() (#??).

Improved tests for get_tables(), table_exists(), and create_table() (#??).

Known issues

This PR introduces breaking changes, since we change the behaviour of the table identification.

However, when testing these changes in diseasystore, they should more fairly just be called "fixes".
For the testing suite in diseasystore it means that I can now use table_exists() and create_table().
It also means I can simplify a lot of other code with the new id() behaviour.

Checklist

  • The PR passes all local unit tests
  • I have documented any new features introduced
  • If the PR adds a new feature, please add an entry in NEWS.md
  • A reviewer is assigned to this PR

RasmusSkytte and others added 30 commits January 24, 2024 08:32
BREAKING CHANGE: any missing schema information will now
assume the default schema should be used.
BREAKING CHANGE: `show_temporary` argument is now a simple logical
and schema is always returned (no longer NA for default schema)
BREAKING CHANGE: This changes the default value to FALSE at present
@RasmusSkytte RasmusSkytte self-assigned this Jan 29, 2024
@RasmusSkytte RasmusSkytte added the enhancement New feature or request label Jan 29, 2024
NAMESPACE Show resolved Hide resolved
Copy link
Collaborator

@marcusmunch marcusmunch left a comment

Choose a reason for hiding this comment

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

Looks good to me!

R/get_table.R Outdated Show resolved Hide resolved
)
}
} else {
rlang::abort("Only character or DBI::Id inputs to table_exists is allowed!")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Technically, a tbl_dbi input is also allowed (see the generic function), which could (for readability) also go into its own S3 method, but this works as-is.

tests/testthat/test-get_table.R Outdated Show resolved Hide resolved
Co-authored-by: Marcus Munch <marcus@marcusmunch.dk>
@RasmusSkytte RasmusSkytte merged commit f22e842 into main Jan 31, 2024
24 checks passed
@RasmusSkytte RasmusSkytte deleted the fix/id_consistency branch January 31, 2024 13:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

table_exists incorrectly identifies table as existing on SQLIte backends with schemas
2 participants