Skip to content

Commit 5cf5813

Browse files
georgesittasgeorgesittas
committed
Fix edge case
1 parent 7a9aa66 commit 5cf5813

3 files changed

Lines changed: 67 additions & 44 deletions

File tree

‎sqlmesh/core/model/common.py‎

Lines changed: 50 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -48,51 +48,64 @@ def make_python_env(
4848

4949
expressions = ensure_list(expressions)
5050
for expression in expressions:
51-
if not isinstance(expression, d.Jinja):
52-
for macro_func_or_var in expression.find_all(d.MacroFunc, d.MacroVar, exp.Identifier):
53-
if macro_func_or_var.__class__ is d.MacroFunc:
54-
name = macro_func_or_var.this.name.lower()
55-
if name in macros:
56-
used_macro = macros[name]
57-
if callable(used_macro) and expression.meta.get("metadata_only"):
58-
setattr(used_macro.func, c.SQLMESH_METADATA, True)
59-
60-
used_macros[name] = used_macro
61-
if name == c.VAR:
62-
args = macro_func_or_var.this.expressions
63-
if len(args) < 1:
64-
raise_config_error("Macro VAR requires at least one argument", path)
65-
if not args[0].is_string:
66-
raise_config_error(
67-
f"The variable name must be a string literal, '{args[0].sql()}' was given instead",
68-
path,
69-
)
70-
used_variables.add(args[0].this.lower())
71-
elif macro_func_or_var.__class__ is d.MacroVar:
72-
name = macro_func_or_var.name.lower()
73-
if name in macros:
74-
used_macros[name] = macros[name]
75-
elif name in variables:
76-
used_variables.add(name)
77-
elif (
78-
isinstance(macro_func_or_var, (exp.Identifier, d.MacroStrReplace, d.MacroSQL))
79-
) and "@" in macro_func_or_var.name:
80-
for _, identifier, braced_identifier, _ in MacroStrTemplate.pattern.findall(
81-
macro_func_or_var.name
82-
):
83-
var_name = braced_identifier or identifier
84-
if var_name in variables:
85-
used_variables.add(var_name)
51+
if isinstance(expression, d.Jinja):
52+
continue
53+
54+
for macro_func_or_var in expression.find_all(d.MacroFunc, d.MacroVar, exp.Identifier):
55+
if macro_func_or_var.__class__ is d.MacroFunc:
56+
name = macro_func_or_var.this.name.lower()
57+
if name not in macros:
58+
continue
59+
60+
# If this macro has been seen before as a non-metadata macro, prioritize that
61+
used_macros[name] = (
62+
macros[name],
63+
(used_macros.get(name) or (None, expression.meta.get("is_metadata")))[1],
64+
)
65+
if name == c.VAR:
66+
args = macro_func_or_var.this.expressions
67+
if len(args) < 1:
68+
raise_config_error("Macro VAR requires at least one argument", path)
69+
if not args[0].is_string:
70+
raise_config_error(
71+
f"The variable name must be a string literal, '{args[0].sql()}' was given instead",
72+
path,
73+
)
74+
used_variables.add(args[0].this.lower())
75+
elif macro_func_or_var.__class__ is d.MacroVar:
76+
name = macro_func_or_var.name.lower()
77+
if name in macros:
78+
# If this macro has been seen before as a non-metadata macro, prioritize that
79+
used_macros[name] = (
80+
macros[name],
81+
(used_macros.get(name) or (None, expression.meta.get("is_metadata")))[1],
82+
)
83+
elif name in variables:
84+
used_variables.add(name)
85+
elif (
86+
isinstance(macro_func_or_var, (exp.Identifier, d.MacroStrReplace, d.MacroSQL))
87+
) and "@" in macro_func_or_var.name:
88+
for _, identifier, braced_identifier, _ in MacroStrTemplate.pattern.findall(
89+
macro_func_or_var.name
90+
):
91+
var_name = braced_identifier or identifier
92+
if var_name in variables:
93+
used_variables.add(var_name)
8694

8795
for macro_ref in jinja_macro_references or set():
8896
if macro_ref.package is None and macro_ref.name in macros:
89-
used_macros[macro_ref.name] = macros[macro_ref.name]
97+
used_macros[macro_ref.name] = (macros[macro_ref.name], None)
9098

91-
for name, used_macro in used_macros.items():
99+
for name, (used_macro, is_metadata) in used_macros.items():
92100
if isinstance(used_macro, Executable):
93101
python_env[name] = used_macro
94102
elif not hasattr(used_macro, c.SQLMESH_BUILTIN) and name not in python_env:
103+
used_macro_func = used_macro.func
104+
previous_is_metadata = getattr(used_macro_func, c.SQLMESH_METADATA, None)
105+
106+
setattr(used_macro_func, c.SQLMESH_METADATA, is_metadata)
95107
build_env(used_macro.func, env=env, name=name, path=module_path)
108+
setattr(used_macro_func, c.SQLMESH_METADATA, previous_is_metadata)
96109

97110
python_env.update(serialize_env(env, path=module_path))
98111
return _add_variables_to_python_env(

‎sqlmesh/core/model/definition.py‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2463,12 +2463,12 @@ def _create_model(
24632463
statements.extend(audit.query for audit in audit_definitions.values())
24642464
for _, audit_args in model.audits:
24652465
for audit_arg_expression in audit_args.values():
2466-
audit_arg_expression.meta["metadata_only"] = True
2466+
audit_arg_expression.meta["is_metadata"] = True
24672467
statements.append(audit_arg_expression)
24682468

24692469
for _, kwargs in model.signals:
24702470
for signal_kwarg in kwargs.values():
2471-
signal_kwarg.meta["metadata_only"] = True
2471+
signal_kwarg.meta["is_metadata"] = True
24722472
statements.append(signal_kwarg)
24732473

24742474
python_env = python_env or {}

‎tests/core/test_model.py‎

Lines changed: 15 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -8811,15 +8811,16 @@ def test_macros_referenced_in_audits_or_signals_are_metadata_only(tmp_path: Path
88118811
name test_model,
88128812
kind FULL,
88138813
signals (
8814-
test_signal_always_true(arg := @m1())
8814+
test_signal_always_true(arg1 := @m1(), arg2 := @non_metadata_macro())
88158815
),
88168816
audits (
88178817
unique_values(columns := @m2())
88188818
),
88198819
);
88208820
88218821
SELECT
8822-
1 AS c
8822+
1 AS c1,
8823+
@non_metadata_macro() AS c2,
88238824
"""
88248825
)
88258826

@@ -8837,7 +8838,11 @@ def m1(evaluator):
88378838
88388839
@macro()
88398840
def m2(evaluator):
8840-
return exp.column("c")"""
8841+
return exp.column("c")
8842+
8843+
@macro()
8844+
def non_metadata_macro(evaluator):
8845+
return 1"""
88418846

88428847
test_macros = tmp_path / "macros/test_macros.py"
88438848
test_macros.parent.mkdir(parents=True, exist_ok=True)
@@ -8852,7 +8857,7 @@ def bar():
88528857
pass
88538858
88548859
@signal()
8855-
def test_signal_always_true(batch, arg):
8860+
def test_signal_always_true(batch, arg1, arg2):
88568861
bar()
88578862
return True"""
88588863

@@ -8869,10 +8874,15 @@ def test_signal_always_true(batch, arg):
88698874

88708875
python_env = model.python_env
88718876

8872-
assert len(python_env) == 6
8877+
assert len(python_env) == 7
88738878
assert (python_env.get("test_signal_always_true") or empty_executable).is_metadata
88748879
assert (python_env.get("bar") or empty_executable).is_metadata
88758880
assert (python_env.get("m1") or empty_executable).is_metadata
88768881
assert (python_env.get("baz") or empty_executable).is_metadata
88778882
assert (python_env.get("m2") or empty_executable).is_metadata
88788883
assert (python_env.get("exp") or empty_executable).is_metadata
8884+
8885+
# non_metadata_macro is referenced in the signal, which makes that reference "metadata only",
8886+
# but it's also referenced in the model's query and the macro itself is not "metadata only",
8887+
# so the corresponding executable needs to be included in the data hash calculation
8888+
assert not (python_env.get("non_metadata_macro") or empty_executable).is_metadata

0 commit comments

Comments
 (0)