-
Notifications
You must be signed in to change notification settings - Fork 79
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
chore: refactor dbListTables()
et al.
#413
Conversation
@@ -156,25 +206,31 @@ find_table <- function(conn, id, inf_table = "tables", only_first = FALSE) { | |||
) | |||
only_first <- FALSE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This override was already present in find_table()
(but find_table()
was generally called with only_first = TRUE
by list_fields()
).
I think this is a remainder of the first two commits in #326, where indeed only_first
isn't necessary, because that used current_schema() (not "schema_s_").
It should be only_first = TRUE
now though because there can be multiple schemas on the search path on Redshift as well (I think).
63490cf
to
a60fc4e
Compare
Conflicts here, too. |
dbListTables()
et al.dbListTables()
et al.
Current Aviator status
This PR was merged using Aviator.
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.
|
607d2df
to
356b790
Compare
Thanks! |
Original: 7730f13 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()` simplify `where_schema` in `list_tables()` align `where_table` with `where_schema` in `list_tables()`
356b790
to
f789767
Compare
I would like to take another stab at #251 and this some preparatory work for that.
Background
Materialized views are currently not returned by
dbListTables()
et al (#251). Materialized Views are not included inINFORMATION_SCHEMA.tables
(see this thread for why they are not. Tl;dr: "They are not defined by the SQL standard.")Sidenote: Most if not all tables in information_schema are just views of the system catalogs, see e.g.
Since mviews are PostgreSQL-specific features we need to query the system catalogs. In particular, we need to use
pg_class
/pg_namespace
instead ofINFORMATION_SCHEMA.tables
.However, in order to retain support for Redshift, we need to keep the
INFORMATION_SCHEMA
queries as well.What happens in this PR?
The above affects the queries that underly the
dbListTables()
,dbExistsTable()
,dbListObjects()
, anddbListFields()
functions.dbListTables()
,dbExistsTable()
anddbListObjects()
do essentially the same thing, namely callINFORMATION_SCHEMA.tables
.In order to make the abovementioned adjustments easier and keep duplicated code at a minimum, I refactored those functions to use a common core function
list_tables()
, which returns the SQL code to queryINFORMATION_SCHEMA.tables
. The (alternative) queries topg_class
(to be added in another PR) will then live here.I refactored the
find_table()
andlist_fields()
functions that underlydbListFields()
in a similar fashion.Details
dbListFields()
: find the first table on the search pathcan be re-written as (with 2 instead of 4 nested queries)
Essentially I applied similar query code here as in #261, but calling
information_schema
instead of the system catalogs.I will open another PR to incorporate the queries to the system catalogs.
It is probably easiest to review this by going through the individual commits.
I do not have access to Redshift, so I was not able to test this PR there. There might be small adjustments necessary (see below). Is it somehow possible for me to test on Redshift (e.g. via GHA)? Or how does this work in general?
Related issues
#251
#261
This does not address #388. I think it is best to address this after #251 is resolved.
#390 is also related, but relatively trivial to apply before or after this.