ENH: New internal dm_meta() for learning a data model from the database, for now for SQL Server only (#342)
#517
Conversation
|
@TSchiefer: This is as good as it gets in this iteration. Let's file follow-up tickets. What do we need to change before merging? |
There was a problem hiding this comment.
This is great.
I didn't go through every detail, but the behavior is nice.
Shall we also already use dm_meta() for learning from PG in this PR?
I tried it for both learning from a specific schema and without providing a schema name. It works great when providing a schema name, but I think we could filter out some or all of the non-user tables when not learning from a specific schema(?)
| "constraint_schema", | ||
| "constraint_type", | ||
| "dbname", | ||
| "FIXME", |
| df_info <- | ||
| info %>% | ||
| dm_select_tbl(-schemata) %>% | ||
| collect() |
There was a problem hiding this comment.
collect() is already done at the end of dm_meta()
| if (is_mssql(con)) { | ||
| if (is.null(catalog)) { | ||
| # FIXME: Classed error message? | ||
| abort("SQL server only supports learning from one database.") |
There was a problem hiding this comment.
classed error msg in follow-up PR?
| @@ -41,6 +41,130 @@ dm_learn_from_db <- function(dest, dbname = NULL, ...) { | |||
| return() | |||
| } | |||
|
|
|||
| if (!is_mssql(con)) { | |||
There was a problem hiding this comment.
AFAICT, dm_meta() works well for PG too. We could learn from PG using the function as well in this PR.
|
I forgot: should we also add a direct test for |
krlmlr
changed the title
dm_meta() for learning a data model from the database, for now for SQL Server only
krlmlr
changed the title
dm_meta() for learning a data model from the database, for now for SQL Server onlydm_meta() for learning a data model from the database, for now for SQL Server only (#342)
Part of #342.