Repository navigation
Conversation
Macros were parsed under their own name, so a call to a macro from its own body resolved to the raw Jinja macro instead of the registry's _MacroWrapper. A return() inside the inner call therefore raised MacroReturnVal past the outer macro, which ended the outer call early and returned the inner call's value. Recursive dbt macros that collect results with return() (e.g. walking nested struct fields) only produced the result of the first recursive call. Define each macro under an internal name and call it by that name, so that references to the macro from within its body go through the wrapper, like calls from any other macro. The internal name is mapped back to the macro's own name in undefined variable error messages. This also replaces the private macro workaround, which renamed every call in a private macro's body to the macro itself and broke private macros that call other macros or functions. Fixes SQLMesh#5341 Signed-off-by: JanakiRamChittineni <341781521+raams95@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes #5341. The macro in that issue calls itself to walk nested struct fields, then returns the collected list with
return(). Under SQLMesh it only ever got the fields from the first recursive call. As the issue thread suggests, this isn't specific to BigQuery or toget_columns_in_relation. Any recursive macro that usesreturn()hits it. Here's a minimal repro with no adapter involved:dbt renders
[1, 2, 3, 4, 5, 6]. SQLMesh renders[2, 3].Root cause.
JinjaMacroRegistryparses each macro under its own name._MacroWrappercalls the macro and catchesMacroReturnVal, which is howreturn()works. Inside the macro's body, though, Jinja resolves the macro's own name to the raw template macro, not to the wrapper in the context. So the inner recursive call'sreturn()raisesMacroReturnValstraight through the outer call. The outer macro stops at its first recursive call and returns the inner call's value.Fix. Each macro is now defined under an internal name (
sqlmesh_macro__<name>), and the wrapper looks it up by that name. When the body refers to the macro's own name, Jinja resolves it from the context like any other macro reference, so recursive calls go through_MacroWrapperand each call handles its ownreturn(). dbt does the same thing: it renames macros todbt_macro__<name>. Undefined variable errors still show the macro's own name (extract_error_detailsstrips the prefix).This also replaces the private macro workaround (
_to_non_private_macro_def). That workaround renamed everyNamecall in a private macro's body to the macro itself, not just recursive calls. So a private macro that called anything else by name broke. For example,{% macro _helper() %}{{ other_macro() }}{% endmacro %}hitRecursionError, and{{ range(3) }}inside a private macro raisedTypeError: macro '...' takes not more than 0 argument(s). With the internal name, private macros are exported without rewriting their bodies.Rendered macro definitions,
data_hash_valuesand serialization don't change. Only the template that gets compiled at render time does, so model fingerprints are unaffected.Test Plan
test_macro_return_recursive(the repro above) andtest_macro_registry_render_private_macro_calling_other_functionstotests/utils/test_jinja.py. Onmain, the first renders[2, 3]and the second raisesTypeError. Both pass with this change. The existing private macro recursion test (test_macro_registry_render_private_macros) still passes.pytest tests/utils tests/dbt tests/core/test_model.py tests/core/test_context.py tests/core/test_audit.py tests/core/test_test.py(slow tests included) on Windows. Everything passes except 8 tests intest_context.py/test_test.py, and those fail the same way onmainon my machine (Windows path/env issues, unrelated to Jinja). That run includestest_dbt_jinja_macro_undefined_variable_errorandtest_jinja_macro_undefined_variable_error, which check the macro name in error messages. For the dbt tests I replaced thesushi_test/dbt_packagessymlinks with directory junctions locally, since git checks them out as plain files on Windows.ruff checkandruff format --checkare clean on the changed files.mypy sqlmesh/utils/jinja.pyonly reports the errors my local environment also gives onmain(missingsqlmesh._version,signal.SIGUSR1).Checklist
make styleand fixed any issues (ran ruff + mypy on the changed files only; on Windows, mypy only shows the same environment errors as onmain)make fast-test) (ran the related test modules listed above on Windows and compared the failures againstmain; I didn't run the full suite)git commit -s) per the DCO