Skip to content
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

wrap models using macros #356

Merged
merged 39 commits into from
Apr 5, 2017

Conversation

drewbanin
Copy link
Contributor

  • this removes the Model.compile call (and the associated, duplicate context that we need to create).

This branch is currently a proof-of concept, but in it's current state:

  • macros work in hooks
  • already_exists works (kind of)
  • we no longer "compile twice" (we do in practice, but we won't have to with this setup)

Copy link
Member

@cmcarthur cmcarthur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is cool. what's the next step?

@@ -30,7 +31,7 @@ def sort_qualifier(cls, sort_type, sort):
.format(sort_type, valid_sort_types)
)

if type(sort) == str:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great catch

dbt/main.py Outdated
@@ -31,6 +31,7 @@ def main(args=None):
handle(args)

except RuntimeError as e:
raise
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cmcarthur we should remove this -- i just threw it in there for debugging

Copy link
Contributor Author

@drewbanin drewbanin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is looking really good @cmcarthur. Couple of notes, but i think once we get these PRs merged we'll be well suited to do a final pass + cleanup


connection = cls.get_connection(profile, model_name)

cls.commit(connection)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cmcarthur is there any problem with committing if there is no open transaction?

@@ -291,34 +286,7 @@ def get_compiler_context(self, linker, model, flat_graph):

return context

def get_context(self, linker, model, models):
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

{%- set already_exists = funcs.already_exists(schema, identifier) -%}
{%- set non_destructive_mode = flags.NON_DESTRUCTIVE == True -%}

{%- if non_destructive_mode -%}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this the right logic for views in non-destructive mode?

@@ -142,7 +188,7 @@ def parse_node(node, node_path, root_project_config, package_project_config,

context = {}

context['ref'] = lambda *args: ''
context['ref'] = __ref(node)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 +1 +!

dbt/runner.py Outdated
@@ -239,6 +239,9 @@ def execute_model(profile, model, existing):

result = None

if get_materialization(model) == 'ephemeral':
return 'SKIP'
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we're just going to silently skip these, right?

dbt/wrapper.py Outdated
sort_keys = model_config.get('sort')
sort_type = model_config.get('sort_type', 'compound')

if type(sort_type) != str:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:)

@cmcarthur cmcarthur mentioned this pull request Apr 3, 2017
Copy link
Contributor Author

@drewbanin drewbanin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looking good! will give this a spin today and update w/ any bugs i find

dbt/parser.py Outdated
@@ -399,7 +396,7 @@ def parse_schema_test(test_base, model_name, test_config, test_type,

raw_sql = "{{{{ {macro}(model=ref('{model}'), {kwargs}) }}}}".format(**{
'model': model_name,
'macro': "test_{}".format(test_type),
'macro': "dbt.test_{}".format(test_type),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is ok for now, but i think ideally we wouldn't namespace this. Idea being that end users could 1) define their own tests (not in the dbt namespace) or 2) override existing tests

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point, i will switch this back.

@cmcarthur cmcarthur merged commit e8d70a6 into feature/schema-tests-defined-by-macros Apr 5, 2017
@cmcarthur cmcarthur deleted the kill-model-py branch April 5, 2017 12:05
@cmcarthur cmcarthur restored the kill-model-py branch April 5, 2017 12:41
@cmcarthur cmcarthur deleted the kill-model-py branch April 5, 2017 12:42
@drewbanin drewbanin restored the kill-model-py branch August 5, 2017 19:07
@drewbanin drewbanin deleted the kill-model-py branch August 5, 2017 19:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants