-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Type aliasing for model contract column data_type #8592
Conversation
Codecov ReportAll modified lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #8592 +/- ##
=======================================
Coverage 86.43% 86.43%
=======================================
Files 176 176
Lines 26009 26019 +10
=======================================
+ Hits 22480 22490 +10
Misses 3529 3529
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
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.
Is there a net new check we can add to ensure that some of the most complicated types are parsed correctly?
Otherwise, LGTM.
This causes test breakage in the adapter repos (mostly for test_constraints.py), at first glance mainly because of SQL in which statements such as "id integer not null" are replaced by "id int not null" because the TYPE_LABELS in the base Column class change INTEGER to INT (and STRING to TEXT). @jtcohen6 @graciegoheen Just want to verify that this is something we actually want to have happen. |
Note: currently it looks like string => text and integer => int are the only data types changed. |
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.
LGTM, can we add a test in case an adapter hasn't implemented the translate_type
method?
if "data_type" in column: | ||
orig_data_type = column["data_type"] | ||
# translate data_type to value in Column.TYPE_LABELS | ||
new_data_type = self.adapter.Column.translate_type(orig_data_type) |
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.
What happens if the translate_type
function fails? Can we do this instead?
column["data_type"] = self.adapter.Column.translate_type(orig_data_type) or column["data_type"]
Also, can we add a test for this scenario?
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.
The base "translate_type" function return the passed in type if no translation exists. If the adapter hasn't implemented a different translate_type function it would use the base one.
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.
suggested change to changelog, but otherwise LGTM
Co-authored-by: colin-rogers-dbt <111200756+colin-rogers-dbt@users.noreply.github.com>
Opened a new issue in dbt-labs/docs.getdbt.com: dbt-labs/docs.getdbt.com#4241 |
resolves #8007
Problem
Currently contracts data_types must use the correct type for their warehouse, instead of using the built in type aliasing.
Solution
For model dictionaries provided by the provider context, convert the column data_type using the "translate_types" method.
Other teams
Affects adapter constraint tests, only BigQuery impacted: dbt-labs/dbt-bigquery#954
Checklist