Add necessary macros to schema test context namespace #3272
Conversation
gshank
force-pushed
the
test_context_regression
branch
from
a17eac6
to
1c634af
Compare
There was a problem hiding this comment.
This is amazing!
I'm so excited by the potential here. Over the next few months, I could see us harnessing depends_on.macros to power up partial parsing and in stateful node selection (state:modified):
- Could we extend
statically_extract_macro_callsto also capture calls toref(),source(),config()? We could mark these as "dirty macros" that have implications at parse-time. - Could we also capture calls to
var(),env_var()? (Values too?) Then we'd know whether a node depends on a given variable, and/or whether that node's file needs to be re-parsed.
In the much shorter term, this neatly resolves the regressions we saw popping up in v0.19.1. After merging, how big of a lift would it be to cherry-pick this onto 0.19.latest and fold it into a v0.19.2 bugfix release? Or are the changes here too foundational / the git histories too far diverged?
|
Some of the code has moved so I'd have to hand update the changes in core/dbt/parser/manifest.py, but the rest of it should apply pretty cleanly. I'd say 2-4 hours to backport. We could certainly look at capturing some of those other calls. We'd have to do some experiments. There's more work to do with adapter.dispatch too. |
| standard_calls = { | ||
| 'source': [], | ||
| 'ref': [], | ||
| 'config': [], | ||
| } |
There was a problem hiding this comment.
are the empty lists from a previous approach? can standard_calls just be a set of strings?
There was a problem hiding this comment.
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.
| 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 |
There was a problem hiding this comment.
I think I'm having a little trouble following this change, was this ordering wrong before?
There was a problem hiding this comment.
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.
| @@ -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.
oof! we missed this line before
There was a problem hiding this comment.
Yep. One of the reasons that it didn't work :-)
resolves #3229, #3240
Description
Ensure TestMacroNamespace is loaded in the TestContext and that necessary macros for rendering tests are loaded.
Checklist
CHANGELOG.mdand added information about my change to the "dbt next" section.