feat: Added schema support for SQLite connections#67
feat: Added schema support for SQLite connections#67marcusmunch merged 22 commits intossi-dk:mainfrom
Conversation
* schema_exists: Reworked to S3 class
- Added methods for SQLite and Postgres using pragma_table_list and
information_schema.schemata respectively
- Previous method moved to schema_exists.default
* get_tables: Reworked to S3 class
- Method for SQLiteConnection calls pragma_table_list, discarding
"temp" schema and sqlite_stat* tables
|
I now notice that 605861c has become a part of this PR. However, it should not break stuff (it passes my local tests) |
| for (conn in conns) { | ||
| # This test exploits ambiguous notation requiring schemas to exits | ||
| if (!inherits(conn, "PqConnection")) next | ||
| if (!inherits(conn, "SQLiteConnection")) next |
There was a problem hiding this comment.
You dont want this test to run on Postgres?
There was a problem hiding this comment.
The tests exploit the same case, so I would actually be in favor of keeping the test only on SQLite, which would in turn no longer require the schema test.one to exist in the Postgres database.
There was a problem hiding this comment.
Additionally, as the test now calls ATTACH [ expr ] AS [ schema-name ] — which is most likely SQLite only — I decided against writing a "universal" test, as any difference in results across backends would trace back to get_tables behaving inconsistently across backends.
Also, as this test is the only one which requires a specific schema ("test.one") to exist, it's better to quickly attach a temporary file in SQLite than to write a test for all supported backends.
For once, SQLite reigns supreme.
RasmusSkytte
left a comment
RasmusSkytte
left a comment
There was a problem hiding this comment.
Only a few comments / thing I want to double check.
-
I am not sure the logic of
table_exists.SQLiteConnectionis correct. But I may be missing something... -
A lot of examples now has a call to schema_exists. I think you can make these more pedagogical by inserting a SCDB::id() in the example chan. (See specifc comment).
If you agree, it should be changed throughout. -
Similarily, there are examples with calls to table_exists which I don't understand why are there. It seems the table is created just before?
RasmusSkytte
left a comment
RasmusSkytte
left a comment
There was a problem hiding this comment.
Only a few comments / thing I want to double check.
-
I am not sure the logic of
table_exists.SQLiteConnectionis correct. But I may be missing something... -
A lot of examples now has a call to schema_exists. I think you can make these more pedagogical by inserting a SCDB::id() in the example chan. (See specifc comment).
If you agree, it should be changed throughout. -
Similarily, there are examples with calls to table_exists which I don't understand why are there. It seems the table is created just before?
RasmusSkytte
left a comment
RasmusSkytte
left a comment
There was a problem hiding this comment.
You have linked the documentation for your S3 methods for table_exists but not for get_schema, schema_exists and get_tables.
The S3 methods for db_timestamp also has linked documentation.
Do you want to link get_schema, schema_exists and get_tables also?
I think it makes the documentation more obvious as to what methods exists.
That is, you can read it in the docs without using methods.
|
I've merged the branch as-is. From what I can see, the Diseasy documentation guide and references do not contain any one guide as to whether or not S3 methods should always be linked with As for SCDB, #69 has been opened to ensure all S3 methods are reviewed as part of the v1.0 milestone. |
2 participants
Intent
This PR adds improved SQLite support to
SCDBby allowing schemas in SQLite connections.Approach
schema_existsandget_tableshave been reworked to S3 classes.schema_existslooks up the schema name in pragma_table_list and information_schema.schemata for SQLite and Postgres respectively.get_tablescurrently only has a method (beside default) for SQLiteConnection, since the previous function was applicable in Postgres without using Postgres-specific features.This in turn broke a lot of examples, likely due to schema-related functions being treated uniquely for
SQLiteConnections.These examples have been updated to only fetch the items in question if relevant schemata/tables exist.
An argument
allow_table_only(defaultTRUE) has been added toidwhich returns aDBI::Id table = db_table_idif a schema cannot be parsed from the input.Known issues
The return value of
idwithallow_table_only = TRUEchanges depending on the presence of a matching schema, which may not be apparent to the user. For more roustness,DBI::Idshould be used.The default value of
TRUEwas chosen for backwards compatibility and may be changed pending a v1.0 release.Checklist
NEWS.md