-
Notifications
You must be signed in to change notification settings - Fork 50
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
feat: New dm_add_uk()
, dm_rm_uk()
and dm_get_all_uks()
functions for explicit support of unique keys
#1716
Conversation
currently still missing:
Thinking that this could be done in a follow-up PR or 2 |
now suppressMessages(devtools::load_all("~/git/cynkra/dm"))
dm_for_filter() %>%
dm_rm_pk(tf_4, h) %>%
dm_get_all_uks()
#> # A tibble: 7 × 3
#> table uk_col kind
#> <chr> <keys> <chr>
#> 1 tf_1 a PK
#> 2 tf_2 c PK
#> 3 tf_3 f, f1 PK
#> 4 tf_5 k PK
#> 5 tf_6 o PK
#> 6 tf_6 n explicit UK
#> 7 tf_4 h implicit UK Created on 2023-01-10 by the reprex package (v2.0.1) |
I think I now addressed everything apart from #1716 (comment) |
Wanted to mention, that currently there is no message that an explicit UK is set when adding a FK with no corresponding PK in the parent table. But I think that could be done in a later PR. |
There's no longer the need to add an explicit UK when adding a new FK for new columns, these UKs remain implicit. Or were you referring to implicit UKs? We don't need to message for implicit UKs either. |
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.
Thanks. Can we do one more run, and merge when green?
R/foreign-keys.R
Outdated
bind_rows(dm_get_all_uks_impl(dm, ref_table_name)) | ||
# setequal() could also be used for matching, but IMHO the order should matter | ||
matches_keys <- map_lgl(all_keys$uk_col, identical, ref_col_name) | ||
if (!any(matches_keys)) { |
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.
I believe 93-105 can go now entirely?
# Execute | ||
# in case `length(i) > 1`: all tables have all their UKs removed, respectively | ||
def$uks[i] <- if (length(i) > 1) { | ||
list_of(new_uk()) | ||
} else { | ||
list_of(filter(def$uks[[i]], !ii)) | ||
} |
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.
I don't understand this code, but we'll notice if it causes trouble.
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.
ii
contains the columns of the UK that is to be removed. We take the list of the current UKs and filter out ii
. Well, as you say, let's see
Right, I thought you did what needed to be done in that respect after your comment #1716 (comment). We can do it this way of course, then there are 2 ways of creating implicit UKs:
Going to update the docs as well |
I addressed your latest comments, merging now. |
unique key = UK.
Can possibly also be called "weak keys".
print.dm()
, should they existdm_paste()
in case optionkeys
is chosenThis PR is already based on #1715 before its merge (1st commit here) to avoid having to deal with ever changing snapshots.
closes #622