-
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
schema tests defined by macros #339
Conversation
dbt/parser.py
Outdated
elif type(arg_val) in (list, tuple): | ||
parts = arg_val | ||
else: | ||
parts = [arg_val] |
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.
can this be anything but a dict? looks like below we make sure it's a dict before calling this. parts = kwargs.values()
should cover all the cases, right?
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.
This func takes arbitrary schema test configs and spits out a human readable, unique name.
my_model:
constraints:
test_something:
- { some_value: True, other_thing: ['abc', 'def'] }
So this func would operate on { some_value: True, other_thing: ['abc', 'def'] }
and spit out:
test_something_my_model_abc_def__True
I actually think that in practice this is kind of annoying/confusing. This "nice" name becomes the compiled filename and shows up in the dbt test
output. Ideally, we'd keep the test config args around and show something like:
ERROR running test "test_something" for model "my_model" with args:
some_value: True
other_thing: ['abc', 'def']
but that's not really how things work currently. Something to consider for the future though
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.
oh gosh, and to answer your question, yes: The arg_val
var will be the value of each item in the supplied dict (args
). So here it is a bool, then a dict. could also be a string/list/int etc
dbt/parser.py
Outdated
child_field = test_config.get('from') | ||
parent_field = test_config.get('field') | ||
parent_model = test_config.get('to') | ||
flat_args.extend([str(part) for part in parts]) |
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.
use dbt.compat.basestring
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.
👍
dbt/utils.py
Outdated
) | ||
logger.info(str(e)) | ||
def dependency_projects(project, include_global=True): | ||
if include_global: |
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 time when we would not want to include globals?
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.
nope, i'll remove this
cc2e91a
to
858d423
Compare
dbt/parser.py
Outdated
parent_ref=("{{ref('"+parent_model+"')}}")) | ||
def as_kwarg(key, value): | ||
test_value = to_string(value) | ||
is_function = re.match(r'^\s*(ref|var)\(.+\)$', test_value) is not None |
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.
@cmcarthur how do you feel about this?
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 feel ok about it. not in love with regex parsing but what can you do.
is the intention to support ref and var in custom schema tests? it doesn't look like those would be passed in here unless i'm missing something
dbt/parser.py
Outdated
parent_ref=("{{ref('"+parent_model+"')}}")) | ||
def as_kwarg(key, value): | ||
test_value = to_string(value) | ||
is_function = re.match(r'^\s*(ref|var)\(.+\)$', test_value) is not None |
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 feel ok about it. not in love with regex parsing but what can you do.
is the intention to support ref and var in custom schema tests? it doesn't look like those would be passed in here unless i'm missing something
@@ -29,7 +29,7 @@ table_summary: | |||
- { field: favorite_color, values: ['blue', 'green'] } | |||
|
|||
relationships: | |||
- { from: favorite_color, to: table_copy, field: favorite_color } | |||
- { from: favorite_color, to: ref('table_copy'), field: favorite_color } |
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.
ahh i understand now. is this a breaking change in how schema tests are defined?
Yeah, sorry for the lack of context. It is a breaking change, but we don't really have much of a choice! I think it's more canonical like this anyway -- kind of weird that the ref was implicit before. I ran it by @jthandy and he agreed
… On Mar 25, 2017, at 8:28 PM, Connor McArthur ***@***.***> wrote:
@cmcarthur commented on this pull request.
In dbt/parser.py:
>
- raw_sql = QUERY_VALIDATE_REFERENTIAL_INTEGRITY.format(
- child_field=child_field,
- child_ref="{{ref('"+model_name+"')}}",
- parent_field=parent_field,
- parent_ref=("{{ref('"+parent_model+"')}}"))
+def as_kwarg(key, value):
+ test_value = to_string(value)
+ is_function = re.match(r'^\s*(ref|var)\(.+\)$', test_value) is not None
i feel ok about it. not in love with regex parsing but what can you do.
is the intention to support ref and var in custom schema tests? it doesn't look like those would be passed in here unless i'm missing something
In test/integration/008_schema_tests_test/models/schema.yml:
> @@ -29,7 +29,7 @@ table_summary:
- { field: favorite_color, values: ['blue', 'green'] }
relationships:
- - { from: favorite_color, to: table_copy, field: favorite_color }
+ - { from: favorite_color, to: ref('table_copy'), field: favorite_color }
ahh i understand now. is this a breaking change in how schema tests are defined?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.
|
To create a custom schema test, create macros prefixed with
test_
. Thetest_
macros must takemodel
as the first argument. If the test takes a single argument (as with not null & unique tests), then the second argument should be calledarg
. Otherwise, the remaining arguments can be named as you like. Test configurations stated inschema.yml
will be mapped to the macro arguments as long as the names match. For example: