Support macros in execute/compile tasks (#1372) #1375
Conversation
| if block.block_type_name == 'macro': | ||
| macro_blocks.append(block.full_block) | ||
| else: | ||
| data_chunks.append(block.full_block) |
There was a problem hiding this comment.
what does the faux "block" look like here?
There was a problem hiding this comment.
Those blocks are defined here: https://github.com/fishtown-analytics/dbt/blob/0a9ed9977/core/dbt/clients/_jinja_blocks.py#L10-L15
If it's a "real block", like {% if ... %} ... {% endif %} it is the exact contents of the block from open to close. Raw/comment blocks are kind of special, but not really special, and end up as 'data blocks'.
However, your comment just made me realize that the current code loses top-level comments and raw blocks. That seems pretty bad! I've added a fix for that and a corresponding test.
Yes. Any rejections that should have been accepted or mis-parsings are bugs. I do want to be clear that accepting stuff that jinja later rejects isn't a bug! |
There was a problem hiding this comment.
If @cmcarthur feels good about the block parsing part of this, then I'm super into it! I smoke tested this locally with some macros, and it looks like this does everything it's supposed to :)
Can you add a test which asserts that macros defined in the query will override any macros in the context? It appears that this is how it works currently, but that's an important nuance that's worth testing imo!
| if not matches: | ||
| return None | ||
| # if there are multiple matches, pick the least greedy match | ||
| # TODO: do I need to account for m.start(), or is this ok? |
There was a problem hiding this comment.
I still don't know! But I can't break it, so maybe everything is fine.
There was a problem hiding this comment.
ok - happy to leave that in there but wanted to make sure you knew :)
| 'compile', | ||
| '{% if True %}select {{ happy_little_macro() }}{% endif %}', | ||
| name='foo', | ||
| macros='{% macro override_me() %}2 as id{% endmacro %}' |
There was a problem hiding this comment.
are we keeping the macros argument to query? My understanding was that this argument would go away
There was a problem hiding this comment.
Yup, it did go away: I changed self.query in this test class to just append the two, instead of making it a parameter: https://github.com/fishtown-analytics/dbt/blob/0a9ed9977b76f363b6ef988753d6a3b8310608cb/test/integration/042_sources_test/test_sources.py#L325-L333 is the new behavior, it just appends macros to any sql it got.
@drewbanin I think this test does it? It has a macro called |
|
I see! Ok, that's perfect then. Nice work :) |
Fixes #1372
I pulled in the existing work lexer work from the archives project into this PR and made some minor tweaks to support preserving top-level data as well. It explicitly only sucks out macros and leaves the rest of the data behind, to preserve jinja control flow.
Other than the state machine/regex horror, there's not much to this PR: extract macro blocks from the SQL, compile them on their own, compile the sql on its own using those macros, execute like normal.