-
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
Add necessary macros to schema test context namespace #3272
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,8 +14,12 @@ | |
# so that higher precedence macros are found first. | ||
# This functionality is also provided by the MacroNamespace, | ||
# but the intention is to eventually replace that class. | ||
# This enables us to get the macor unique_id without | ||
# This enables us to get the macro unique_id without | ||
# processing every macro in the project. | ||
# Note: the root project macros override everything in the | ||
# dbt internal projects. External projects (dependencies) will | ||
# use their own macros first, then pull from the root project | ||
# followed by dbt internal projects. | ||
class MacroResolver: | ||
def __init__( | ||
self, | ||
|
@@ -48,18 +52,29 @@ def _build_internal_packages_namespace(self): | |
self.internal_packages_namespace.update( | ||
self.internal_packages[pkg]) | ||
|
||
# search order: | ||
# local_namespace (package of particular node), not including | ||
# the internal packages or the root package | ||
# This means that within an extra package, it uses its own macros | ||
# root package namespace | ||
# non-internal packages (that aren't local or root) | ||
# dbt internal packages | ||
def _build_macros_by_name(self): | ||
macros_by_name = {} | ||
# search root package macros | ||
for macro in self.root_package_macros.values(): | ||
|
||
# all internal packages (already in the right order) | ||
for macro in self.internal_packages_namespace.values(): | ||
macros_by_name[macro.name] = macro | ||
# search miscellaneous non-internal packages | ||
|
||
# non-internal packages | ||
for fnamespace in self.packages.values(): | ||
for macro in fnamespace.values(): | ||
macros_by_name[macro.name] = macro | ||
# search all internal packages | ||
for macro in self.internal_packages_namespace.values(): | ||
|
||
# root package macros | ||
for macro in self.root_package_macros.values(): | ||
macros_by_name[macro.name] = macro | ||
|
||
self.macros_by_name = macros_by_name | ||
Comment on lines
62
to
78
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I'm having a little trouble following this change, was this ordering wrong before? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. It never actually worked, so we didn't notice. It needs to be built from lowest precedence to highest, and it was the opposite before. |
||
|
||
def _add_macro_to( | ||
|
@@ -97,18 +112,26 @@ def add_macros(self): | |
for macro in self.macros.values(): | ||
self.add_macro(macro) | ||
|
||
def get_macro_id(self, local_package, macro_name): | ||
def get_macro(self, local_package, macro_name): | ||
local_package_macros = {} | ||
if (local_package not in self.internal_package_names and | ||
local_package in self.packages): | ||
local_package_macros = self.packages[local_package] | ||
# First: search the local packages for this macro | ||
if macro_name in local_package_macros: | ||
return local_package_macros[macro_name].unique_id | ||
return local_package_macros[macro_name] | ||
# Now look up in the standard search order | ||
if macro_name in self.macros_by_name: | ||
return self.macros_by_name[macro_name].unique_id | ||
return self.macros_by_name[macro_name] | ||
return None | ||
|
||
def get_macro_id(self, local_package, macro_name): | ||
macro = self.get_macro(local_package, macro_name) | ||
if macro is None: | ||
return None | ||
else: | ||
return macro.unique_id | ||
|
||
|
||
# Currently this is just used by test processing in the schema | ||
# parser (in connection with the MacroResolver). Future work | ||
|
@@ -127,10 +150,11 @@ def __init__( | |
local_namespace = {} | ||
if depends_on_macros: | ||
for macro_unique_id in depends_on_macros: | ||
macro = self.manifest.macros[macro_unique_id] | ||
local_namespace[macro.name] = MacroGenerator( | ||
macro, self.ctx, self.node, self.thread_ctx, | ||
) | ||
if macro_unique_id in self.macro_resolver.macros: | ||
macro = self.macro_resolver.macros[macro_unique_id] | ||
local_namespace[macro.name] = MacroGenerator( | ||
macro, self.ctx, self.node, self.thread_ctx, | ||
) | ||
self.local_namespace = local_namespace | ||
|
||
def get_from_package( | ||
|
@@ -141,12 +165,14 @@ def get_from_package( | |
macro = self.macro_resolver.macros_by_name.get(name) | ||
elif package_name == GLOBAL_PROJECT_NAME: | ||
macro = self.macro_resolver.internal_packages_namespace.get(name) | ||
elif package_name in self.resolver.packages: | ||
elif package_name in self.macro_resolver.packages: | ||
macro = self.macro_resolver.packages[package_name].get(name) | ||
else: | ||
raise_compiler_error( | ||
f"Could not find package '{package_name}'" | ||
) | ||
if not macro: | ||
return None | ||
macro_func = MacroGenerator( | ||
macro, self.ctx, self.node, self.thread_ctx | ||
) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1408,7 +1408,12 @@ def __init__( | |
self.macro_resolver = macro_resolver | ||
self.thread_ctx = MacroStack() | ||
super().__init__(model, config, manifest, provider, context_config) | ||
self._build_test_namespace | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oof! we missed this line before There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep. One of the reasons that it didn't work :-) |
||
self._build_test_namespace() | ||
# We need to rebuild this because it's already been built by | ||
# the ProviderContext with the wrong namespace. | ||
self.db_wrapper = self.provider.DatabaseWrapper( | ||
self.adapter, self.namespace | ||
) | ||
|
||
def _build_namespace(self): | ||
return {} | ||
|
@@ -1421,11 +1426,17 @@ def _build_test_namespace(self): | |
depends_on_macros = [] | ||
if self.model.depends_on and self.model.depends_on.macros: | ||
depends_on_macros = self.model.depends_on.macros | ||
lookup_macros = depends_on_macros.copy() | ||
for macro_unique_id in lookup_macros: | ||
lookup_macro = self.macro_resolver.macros.get(macro_unique_id) | ||
if lookup_macro: | ||
depends_on_macros.extend(lookup_macro.depends_on.macros) | ||
|
||
macro_namespace = TestMacroNamespace( | ||
self.macro_resolver, self.ctx, self.node, self.thread_ctx, | ||
self.macro_resolver, self._ctx, self.model, self.thread_ctx, | ||
depends_on_macros | ||
) | ||
self._namespace = macro_namespace | ||
self.namespace = macro_namespace | ||
|
||
|
||
def generate_test_context( | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
{% macro test_type_one(model) %} | ||
|
||
select count(*) from ( | ||
|
||
select * from {{ model }} | ||
union all | ||
select * from {{ ref('model_b') }} | ||
|
||
) as Foo | ||
|
||
{% endmacro %} | ||
|
||
{% macro test_type_two(model) %} | ||
|
||
{{ config(severity = "WARN") }} | ||
|
||
select count(*) from {{ model }} | ||
|
||
{% endmacro %} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
select 1 as fun |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
select 1 as notfun |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
|
||
version: 2 | ||
|
||
models: | ||
- name: model_a | ||
tests: | ||
- type_one | ||
- type_two |
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.
are the empty lists from a previous approach? can
standard_calls
just be a set of strings?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.
That structure was there from an experiment that Drew wrote. It could be a set of strings right now, but I think I'll leave it like this as a connection to that previous experiment and to leave space for a possible future enhancement.