-
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
WIP: first commit - added aliasing to unique and not_null #2075
WIP: first commit - added aliasing to unique and not_null #2075
Conversation
…tests to fix bug where they would fail if model and column had the same name.
Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Teghan Nightengale.
|
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.
Hey @tnightengale - I just took a quick look here and dropped a comment in below. Let me know what you think about that.
Also, you'll need to sign the CLA signed to get this PR merged. Please give me a shout whenever you're able to do that. Thanks!
@@ -4,8 +4,8 @@ | |||
{% set column_name = kwargs.get('column_name', kwargs.get('arg')) %} | |||
|
|||
select count(*) | |||
from {{ model }} | |||
where {{ column_name }} is null | |||
from {{ model }} AS aliased |
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 just thought of something really critical here - you can currently supply an expression to these tests, eg:
models:
- name: my_model
tests:
- unique:
arg: concat(field_1, field_2)
I think code like this will no longer work if we purse this approach :/
Maybe an alternative idea is to do something like:
select count(*)
from (
select {{ column_name }} as __dbt_not_null_field from {{ model }}
) as __dbt_test_sbq
where __dbt_not_null_field is null
I think this should support existing use cases while also addressing the current issue with BigQuery table names masking column names.
@@ -7,11 +7,11 @@ select count(*) | |||
from ( | |||
|
|||
select | |||
{{ column_name }} | |||
aliased.{{ column_name }} |
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.
see above
hey @tnightengale - going to close this one out as stale for now, but please let me know if you're interested in picking this one back up! |
resolves #2061
Description
Small fix for the bug identified in this issue: #2061.
Added aliasing to the unique and not_null schema tests to fix bug where they would fail if model and column had the same name.
Changes
core/dbt/include/global_project/macros/schema_tests/not_null.sql -> added alias
core/dbt/include/global_project/macros/schema_tests/unique.sql -> added alias
Tests
Hoping that someone from fishtown can kick of the CI tests - I did not run the make file tests locally. I think the aliasing is completely standard sql and should be db agnostic - I am slightly concerned if
aliased.{{ column_name }}
will compile with a space toaliased. some_column
.