Render sql_header for dbt show
#8436
Conversation
|
Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide. |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #8436 +/- ##
==========================================
- Coverage 86.61% 83.89% -2.72%
==========================================
Files 175 175
Lines 25595 25602 +7
==========================================
- Hits 22168 21478 -690
- Misses 3427 4124 +697
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
jtcohen6
force-pushed
the
jerco/fix-8417
branch
from
7784625 to
d73b1b9
Compare
| if "sql_header" in compiled_node.unrendered_config: | ||
| compiled_node.compiled_code = ( |
There was a problem hiding this comment.
Previously we were mutating the compiled_code attribute on compiled_node directly in addition to executing it, whereas the changes here just execute it.
I imagine modifying compiled_node.compiled_code had some downstream effects such as surfacing the sql_header in the target/compiled files. Is the motivation for this change to be more consistent with what target/compiled files typically contain for run models?
There was a problem hiding this comment.
Is the motivation for this change to be more consistent with what target/compiled files typically contain for run models?
Yes, this is what I was thinking. The sql_header isn't part of a node's compiled_code when it's compiled or materialized, so it feels inconsistent that it would be included just for the manifest produced by show.
core/dbt/task/show.py
Outdated
| # See dbt-core issues #2793, #3264, #7151 | ||
| # So technically this should be "generate_parser_model_context" (I think) instead of "generate_runtime_model_context" | ||
| # Generating the context will be slower if we don't actually need to render the sql_header (if it contains no Jinja) | ||
| context = generate_runtime_model_context(compiled_node, self.config, manifest) |
There was a problem hiding this comment.
One dumb optimization we could make here: only use this slower code path if there is a { in the string.
There was a problem hiding this comment.
Yeah, we have some prior art for that: _HAS_RENDER_CHARS_PAT.search().
We bake this into our Jinja rendering, to no-op if there are no render characters. What do you think? Worth doing?
There was a problem hiding this comment.
Up to you, honestly. Timeboxing to 15 minutes seems fine to me.
jtcohen6
force-pushed
the
jerco/fix-8417
branch
from
c70fb87 to
4b46087
Compare
|
Closing in favor of #8641 |
resolves #8417
Problem
sql_headeris not rendered fordbt show. It is while running dbt models, as{{ sql_header }}is included within the materialization logic.Solution
Always render
sql_header, if configured, before prepending to the model'scompiled_codeChecklist