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.

Sign up for GitHub

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

Jump to bottom

feat: dm_from_con() can use multiple schemata #1449

Merged
merged 6 commits into from Sep 20, 2022
Merged

feat: dm_from_con() can use multiple schemata #1449

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
5a7b266
Can use multiple schemata in `dm_from_con()`
mgirlich Sep 2, 2022
ca41ff2
Update R/meta.R
mgirlich Sep 5, 2022
1c6a593
Fix `is.na()` check
mgirlich Sep 5, 2022
174c278
Merged origin/dm-from-con-multiple-schema into dm-from-con-multiple-s…
mgirlich Sep 5, 2022
fe68485
Merge branch 'main' into dm-from-con-multiple-schema
krlmlr Sep 19, 2022
51d8498
Update R/meta.R
krlmlr Sep 20, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 0 additions & 1 deletion R/db-helpers.R
View file
Open in desktop
Expand Up @@ -125,7 +125,6 @@ get_src_tbl_names <- function(src, schema = NULL, dbname = NULL) {

if (!is.null(schema)) {
check_param_class(schema, "character")
check_param_length(schema)
}

if (is_mssql(src)) {
Expand Down
6 changes: 5 additions & 1 deletion R/meta.R
View file
Open in desktop
Expand Up @@ -225,6 +225,10 @@ filter_dm_meta <- function(dm_meta, catalog = NULL, schema = NULL) {
force(catalog)
force(schema)

if (length(schema) > 1 && anyNA(schema)) {
cli::cli_abort("{.arg schema} must not contain NA if it has more than one element.")
}

schemata <- dm_meta$schemata
tables <- dm_meta$tables
columns <- dm_meta$columns
Expand All @@ -241,7 +245,7 @@ filter_dm_meta <- function(dm_meta, catalog = NULL, schema = NULL) {
constraint_column_usage <- constraint_column_usage %>% filter(table_catalog %in% !!catalog)
}

if (!is.null(schema) && !is.na(schema)) {
if (!is.null(schema) && all(!is.na(schema))) {
krlmlr marked this conversation as resolved.
Show resolved Hide resolved
schemata <- schemata %>% filter(schema_name %in% !!schema)
tables <- tables %>% filter(table_schema %in% !!schema)
columns <- columns %>% filter(table_schema %in% !!schema)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The condition below

} else if (!isTRUE(is.na(schema)) && is_mariadb(dm_get_con(dm_meta))) {

is in my opinion confusing. I think the idea is to use a default for MariaDB if schema is NULL. If so, this should use is.null(schema). Also, it might be better to implement these defaults differently. There is a function schema_mariadb() which seems to implement this default. But it always uses database() and here the default handling is different in case database() is NULL.

Copy link
Collaborator

Choose a reason for hiding this comment

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

IIRC DATABASE() can be NULL in MariaDB/MySQL if connecting without a default database, and this could be the reason for this ... hack?

Expand Down
28 changes: 28 additions & 0 deletions tests/testthat/_snaps/learn.md
View file
Open in desktop
Expand Up @@ -397,3 +397,31 @@
8 tkeys id
9 trans id

---



---

Code
dm::dm_get_all_fks(my_dm)
Output
# A tibble: 3 x 5
child_table child_fk_cols parent_table parent_key_cols on_delete
<chr> <keys> <chr> <keys> <chr>
1 oseba id_nesreca nesreca id_nesreca no_action
2 nesreca upravna_enota upravna_enota id_upravna_enota no_action
3 oseba upravna_enota upravna_enota id_upravna_enota no_action

---

Code
dm::dm_get_all_pks(my_dm)
Output
# A tibble: 3 x 2
table pk_col
<chr> <keys>
1 ad ts, ad_id, user_id
2 nesreca id_nesreca
3 upravna_enota id_upravna_enota

5 changes: 5 additions & 0 deletions tests/testthat/test-learn.R
View file
Open in desktop
Expand Up @@ -440,4 +440,9 @@ test_that("dm_from_con() with mariaDB", {
expect_snapshot_output(my_dm <- dm_from_con(my_db))
expect_snapshot(dm::dm_get_all_fks(my_dm))
expect_snapshot(dm::dm_get_all_pks(my_dm))

# multiple schemata work
expect_snapshot_output(my_dm <- dm_from_con(my_db, schema = c("Accidents", "Ad")))
expect_snapshot(dm::dm_get_all_fks(my_dm))
expect_snapshot(dm::dm_get_all_pks(my_dm))
Comment on lines +445 to +447
Copy link
Collaborator

Choose a reason for hiding this comment

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

The test output looks odd, perhaps because the connection uses dbname = "Financial_ijs" ?

I wonder what's a good test design on Postgres and MSSQL for this feature.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe simply setup our own DB.

})