Repository navigation
Conversation
RPostgres' dbListTables() silently ignores the schema argument, so dbListTables(), getTableNames() and existsTable() on a connection made with createDbiConnectionDetails() listed the tables on the search path instead of the requested schema. Use the information schema query that is already used for DuckDB for PostgreSQL and Redshift as well. Fixes OHDSI#327
Author
|
@schuemie requesting review |
Author
|
Hi, would someone have time to look at this? It fixes #327: on connections from |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #327.
Problem
For connections made with
createDbiConnectionDetails(),dbListTables(conn, databaseSchema = ...)falls through toDBI::dbListTables(conn@dbiConnection, schema = databaseSchema).RPostgres::dbListTables()does not have aschemaargument and silently drops it, so it returns the tables on the search path (usuallypublic) instead of the tables in the requested schema. BecausegetTableNames()andexistsTable()build on this, they give wrong answers:It can also return a false positive when a table with the same name exists in
public:existsTable(connection, "non_existing_schema", "person")returnsTRUE.Change
dbListTables()for DBI connections already has a branch that queriesinformation_schema.tablesfor DuckDB. This PR uses that same branch forpostgresqlandredshift, which is also whatgetTableNames()already does for a plainPqConnection/RedshiftConnection. Nothing changes for other platforms or for JDBC connections.Tests
I added a test to
tests/testthat/test-getTableNames.Rthat connects throughcreateDbiConnectionDetails()withRPostgres::Postgres(), using the existingCDM5_POSTGRESQL_*environment variables. It checks thatpersonis listed in the CDM schema, thatexistsTable()finds it, and thatexistsTable()returnsFALSEfor a schema that doesn't exist. The test is skipped when no Postgres test server is configured or RPostgres is not installed.CDM5_POSTGRESQL_*variables pointing at it). Without the fix it fails in both setups: whenpersonis only in the CDM schema, the positive checks fail, and when there is also apublic.person, the negative check fails. With the fix it passes in both.testthat::test_local()with only the local SQLite/DuckDB servers: 112 expectations passed, 0 failures. There were 4 errors, and I got the same 4 on unmodifieddevelop. They come from my local setup (no JDBC driver folder, bigrquery not installed).R CMD check --no-manual --ignore-vignettes: 0 errors, 0 warnings, 0 notes.I did not touch NEWS.md.