-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Preserve FK key metadata on native models #42866
Conversation
Fixes #42130. This PR assumes that the only way a native model can have columns with FK semantic types is if a user specified it by editing the metadata.
|
db (t2/select-one Database :id database_id) | ||
;; a native model can have columns with keys as semantic types on if a user specified it | ||
trust-semantic-keys? (and (= (:type card) :model) | ||
(= (-> card :dataset_query :type) :native))] |
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.
A regular (mbql) model can also have metadata overrides with FKs for columns that are not FKs in the DB
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.
@ranquild Do you have an example when that information gets lost? (The FK override on mbql models worked for me.)
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 have an example. I think you're right and that's the fix we need to do. It should be safe to "trust" FK metadata on native queries because it won't be set automatically.
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've just noticed the typo in the comment. I meant "only if a user specified it", not "on if a user specified it".
Fixes #42130.
This PR assumes that the only way a native model can have columns with FK semantic types is if a user specified it by editing the metadata.
TODO