feat: dbListTables() lists materialized views
#414
Conversation
|
This now has conflicts. |
dpprdan
changed the title
Current Aviator status
This pull request is currently open (not queued). How to mergeTo merge this PR, comment
See the real-time status of this PR on the
Aviator webapp.
Use the Aviator Chrome Extension
to see the status of your PR within GitHub.
|
krlmlr
force-pushed
the
feat/list_matviews
branch
3 times, most recently
from
0524d53
to
15b3cef
Compare
krlmlr
changed the title
dbListTables() lists materialized views
| @@ -14,11 +14,11 @@ dbListObjects_PqConnection_ANY <- function(conn, prefix = NULL, ...) { | |||
| } | |||
| query <- paste0( | |||
| "SELECT ", null_varchar, " AS schema, table_name AS table FROM ( \n", | |||
| list_tables(conn = conn, order_by = "table_type, table_name"), | |||
| list_tables(conn = conn, order_by = "table_type, table_name"), | |||
There was a problem hiding this comment.
styler won't like it. Maybe use braces or parens to enforce indent here?
There was a problem hiding this comment.
I'm fine with not having an extra indent here and prefer that to extra braces or parentheses.
If I run styler::style_package() with tidyverse_style() it'll style a lot more files than those relevant for this PR. So I'll just run styler on the latter and leave the others for a separate PR?!
| if (exists_table(conn, id)) { | ||
| # we know from exists_table() that id@name["table"] exists | ||
| # and the user has access priviledges | ||
| tname_str <- stats::na.omit(id@name[c("schema", "table")]) |
There was a problem hiding this comment.
Id() now has unnamed components only. How to deal with this here?
There was a problem hiding this comment.
We have more places where we rely on named Id()s, I am afraid. Also in main, which are not caught by current tests. 😬
I can take a stab at r-dbi/DBItest#367 if you want? Either way, I'll try to amend the tests for matviews first and work my way up from there:
Which name variants are (supposed to be) supported now?
- those formerly allowed already
Id("schema_name", "table_name")I("schema_name.table_name")as well?
Relatedly: Is r-dbi/DBItest#340 relevant for RPostgres?
| @@ -60,7 +67,8 @@ | |||
| dbConnect_PqDriver <- function(drv, dbname = NULL, | |||
| host = NULL, port = NULL, password = NULL, user = NULL, service = NULL, ..., | |||
| bigint = c("integer64", "integer", "numeric", "character"), | |||
| check_interrupts = FALSE, timezone = "UTC", timezone_out = NULL) { | |||
| check_interrupts = FALSE, system_catalogs = TRUE, | |||
There was a problem hiding this comment.
The new argument name is not very clear, perhaps use something like meta = c("system", "information_schema") ?
I'm not in love with meta either, can you think of a better name?
There was a problem hiding this comment.
meta seems quite broad and therefore not very clear as well, so how about meta_schema?
If we'd want to stick with a boolean, it could also be information_schema, but it's probably better to use a character argument to keep it extendable?
(Just documenting some related info:)
PostgreSQL calls the tables with the system information System Catalogs. There are also System Views, however, like pg_matviews. Both the Catalogs and the Views reside in the pg_catalog schema.
Redshift also has Materialized Views (out of scope for this PR). Their meta info resides (mostly) in ST_MV_INFO, one of the "system tables and views". The "System catalog tables", which seem to be legacy tables from the PostgreSQL v8 times, are a part of those as well.
Long story short: meta_schema = c("system", "information_schema") seems to be the most fitting argument to specify which schema should be queried for information on tables, (materialized) views and columns.
🥳 I'll look into it, but again, it might take a couple of days until I'll get around to it. |
Full history: 699813b refactor `dbListTables()` with `list_tables()`, now orders result by `table_type` and `table_name` refactor `dbExistsTable()` with `list_tables()` refactor `dbListObjects()` with `list_tables()` merge `find_table()` code into `list_fields()` `find_table()` isn't used anywhere else anymore (e.g. `exists_table()`) simplify the "get current_schemas() as table" code pass full `id` to `list_fields()` align `dbExistsTable()` with `dbListFields()` add some comments and whitespace simplify `where_schema` in `list_tables()` align `where_table` with `where_schema` in `list_tables()` add `system_catalogs` argument to `dbConnect()` add materialized view tests `list_tables()`: query system catalogs if available `list_fields()`: query system catalogs if available
This PR extends #413 and adds the queries of the system catalogs in order for
dbList*()anddbExistsTable()to return information about materialized views in PostgreSQL. For background see #413 and #251.In order to make the use of the system catalogs vis-a-vis
information_schema.tablesoptional, I added asystem_catalogsargument todbConnect()(as suggested in #261). I set the default toTRUE(i.e. for regularPqConnections).Redshift connections do not have the (explicit) argument, because there is (probably) no necessary extra information in it's system catalog views (i.e. above what's in
information_schema). Since Redshift connections inherit fromPqConnection,system_catalogsis set toFALSEfor them.I've added tests for materialized views, i.e. whether
dbList*()anddbExistsTable()return information about matviews on the search path and on a custom schema.closes #251