Fix edge case

georgesittas · georgesittas · commit f44332ea092f · 2025-04-10T00:57:15.000+03:00
diff --git a/sqlmesh/core/model/common.py b/sqlmesh/core/model/common.py
@@ -24,8 +24,11 @@
 
 if t.TYPE_CHECKING:
     from sqlglot.dialects.dialect import DialectType
+    from sqlmesh.utils import registry_decorator
     from sqlmesh.utils.jinja import MacroReference
 
+    MacroCallable = registry_decorator
+
 
 def make_python_env(
     expressions: t.Union[exp.Expression, t.List[exp.Expression]],
@@ -43,56 +46,74 @@ def make_python_env(
     python_env = {} if python_env is None else python_env
     variables = variables or {}
     env: t.Dict[str, t.Tuple[t.Any, t.Optional[bool]]] = {}
-    used_macros = {}
+    used_macros: t.Dict[
+        str,
+        t.Tuple[t.Union[Executable | MacroCallable], t.Optional[bool]],
+    ] = {}
     used_variables = (used_variables or set()).copy()
 
     expressions = ensure_list(expressions)
     for expression in expressions:
-        if not isinstance(expression, d.Jinja):
-            for macro_func_or_var in expression.find_all(d.MacroFunc, d.MacroVar, exp.Identifier):
-                if macro_func_or_var.__class__ is d.MacroFunc:
-                    name = macro_func_or_var.this.name.lower()
-                    if name in macros:
-                        used_macro = macros[name]
-                        if callable(used_macro) and expression.meta.get("metadata_only"):
-                            setattr(used_macro.func, c.SQLMESH_METADATA, True)
-
-                        used_macros[name] = used_macro
-                        if name == c.VAR:
-                            args = macro_func_or_var.this.expressions
-                            if len(args) < 1:
-                                raise_config_error("Macro VAR requires at least one argument", path)
-                            if not args[0].is_string:
-                                raise_config_error(
-                                    f"The variable name must be a string literal, '{args[0].sql()}' was given instead",
-                                    path,
-                                )
-                            used_variables.add(args[0].this.lower())
-                elif macro_func_or_var.__class__ is d.MacroVar:
-                    name = macro_func_or_var.name.lower()
-                    if name in macros:
-                        used_macros[name] = macros[name]
-                    elif name in variables:
-                        used_variables.add(name)
-                elif (
-                    isinstance(macro_func_or_var, (exp.Identifier, d.MacroStrReplace, d.MacroSQL))
-                ) and "@" in macro_func_or_var.name:
-                    for _, identifier, braced_identifier, _ in MacroStrTemplate.pattern.findall(
-                        macro_func_or_var.name
-                    ):
-                        var_name = braced_identifier or identifier
-                        if var_name in variables:
-                            used_variables.add(var_name)
+        if isinstance(expression, d.Jinja):
+            continue
+
+        for macro_func_or_var in expression.find_all(d.MacroFunc, d.MacroVar, exp.Identifier):
+            if macro_func_or_var.__class__ is d.MacroFunc:
+                name = macro_func_or_var.this.name.lower()
+                if name not in macros:
+                    continue
+
+                # If this macro has been seen before as a non-metadata macro, prioritize that
+                used_macros[name] = (
+                    macros[name],
+                    (used_macros.get(name) or (None, expression.meta.get("is_metadata")))[1],
+                )
+                if name == c.VAR:
+                    args = macro_func_or_var.this.expressions
+                    if len(args) < 1:
+                        raise_config_error("Macro VAR requires at least one argument", path)
+                    if not args[0].is_string:
+                        raise_config_error(
+                            f"The variable name must be a string literal, '{args[0].sql()}' was given instead",
+                            path,
+                        )
+                    used_variables.add(args[0].this.lower())
+            elif macro_func_or_var.__class__ is d.MacroVar:
+                name = macro_func_or_var.name.lower()
+                if name in macros:
+                    # If this macro has been seen before as a non-metadata macro, prioritize that
+                    used_macros[name] = (
+                        macros[name],
+                        (used_macros.get(name) or (None, expression.meta.get("is_metadata")))[1],
+                    )
+                elif name in variables:
+                    used_variables.add(name)
+            elif (
+                isinstance(macro_func_or_var, (exp.Identifier, d.MacroStrReplace, d.MacroSQL))
+            ) and "@" in macro_func_or_var.name:
+                for _, identifier, braced_identifier, _ in MacroStrTemplate.pattern.findall(
+                    macro_func_or_var.name
+                ):
+                    var_name = braced_identifier or identifier
+                    if var_name in variables:
+                        used_variables.add(var_name)
 
     for macro_ref in jinja_macro_references or set():
         if macro_ref.package is None and macro_ref.name in macros:
-            used_macros[macro_ref.name] = macros[macro_ref.name]
+            used_macros[macro_ref.name] = (macros[macro_ref.name], None)
 
-    for name, used_macro in used_macros.items():
+    for name, (used_macro, is_metadata) in used_macros.items():
         if isinstance(used_macro, Executable):
             python_env[name] = used_macro
         elif not hasattr(used_macro, c.SQLMESH_BUILTIN) and name not in python_env:
+            used_macro_func = used_macro.func
+            previous_is_metadata = getattr(used_macro_func, c.SQLMESH_METADATA, None)
+
+            if is_metadata:
+                setattr(used_macro_func, c.SQLMESH_METADATA, is_metadata)
+
             build_env(used_macro.func, env=env, name=name, path=module_path)
+            setattr(used_macro_func, c.SQLMESH_METADATA, previous_is_metadata)
 
     python_env.update(serialize_env(env, path=module_path))
     return _add_variables_to_python_env(
diff --git a/sqlmesh/core/model/definition.py b/sqlmesh/core/model/definition.py
@@ -2463,12 +2463,12 @@ def _create_model(
     statements.extend(audit.query for audit in audit_definitions.values())
     for _, audit_args in model.audits:
         for audit_arg_expression in audit_args.values():
-            audit_arg_expression.meta["metadata_only"] = True
+            audit_arg_expression.meta["is_metadata"] = True
             statements.append(audit_arg_expression)
 
     for _, kwargs in model.signals:
         for signal_kwarg in kwargs.values():
-            signal_kwarg.meta["metadata_only"] = True
+            signal_kwarg.meta["is_metadata"] = True
             statements.append(signal_kwarg)
 
     python_env = python_env or {}
diff --git a/tests/core/test_model.py b/tests/core/test_model.py
@@ -8811,15 +8811,16 @@ def test_macros_referenced_in_audits_or_signals_are_metadata_only(tmp_path: Path
           name test_model,
           kind FULL,
           signals (
-            test_signal_always_true(arg := @m1())
+            test_signal_always_true(arg1 := @m1(), arg2 := @non_metadata_macro())
           ),
           audits (
             unique_values(columns := @m2())
           ),
         );
 
         SELECT
-          1 AS c
+          1 AS c1,
+          @non_metadata_macro() AS c2,
         """
     )
 
@@ -8837,7 +8838,11 @@ def m1(evaluator):
 
 @macro()
 def m2(evaluator):
-    return exp.column("c")"""
+    return exp.column("c")
+
+@macro()
+def non_metadata_macro(evaluator):
+    return 1"""
 
     test_macros = tmp_path / "macros/test_macros.py"
     test_macros.parent.mkdir(parents=True, exist_ok=True)
@@ -8852,7 +8857,7 @@ def bar():
     pass
 
 @signal()
-def test_signal_always_true(batch, arg):
+def test_signal_always_true(batch, arg1, arg2):
     bar()
     return True"""
 
@@ -8869,14 +8874,19 @@ def test_signal_always_true(batch, arg):
 
     python_env = model.python_env
 
-    assert len(python_env) == 6
+    assert len(python_env) == 7
     assert (python_env.get("test_signal_always_true") or empty_executable).is_metadata
     assert (python_env.get("bar") or empty_executable).is_metadata
     assert (python_env.get("m1") or empty_executable).is_metadata
     assert (python_env.get("baz") or empty_executable).is_metadata
     assert (python_env.get("m2") or empty_executable).is_metadata
     assert (python_env.get("exp") or empty_executable).is_metadata
 
+    # non_metadata_macro is referenced in the signal, which makes that reference "metadata only",
+    # but it's also referenced in the model's query and the macro itself is not "metadata only",
+    # so the corresponding executable needs to be included in the data hash calculation
+    assert not (python_env.get("non_metadata_macro") or empty_executable).is_metadata
+
 
 def test_scd_type_2_full_history_restatement():
     assert ModelKindName.SCD_TYPE_2.full_history_restatement_only is True
