Repository: TobikoData/sqlmesh (python)

Issue:
Conditions in the Built-in Audits dont support macro vars
Slack context: https://tobiko-data.slack.com/archives/C044BRE5W4S/p1712179233739539

Built-in audits (such as `unique_values`) have a `condition`  property that can be used to pass extra conditions like so:

```
@MODEL (
   ...
  audits (
    unique_values(columns=(id), condition=foo='bar')
  )
)
```

However, if you want to use `@start_dt` and `@end_dt` in the `condition` to limit the amount of data scanned on the underlying engine, they dont expand out correctly.

For example, this `condition` fails:
```
condition=ts between @start_dt and @end_dt

-- ValueError: Unsupported expression type MacroVar
```

You can wrap it in `@SQL` to get further:
```
condition=@SQL('ts between @start_dt and @end_dt')

-- sqlglot.errors.ParseError: Invalid expression / Unexpected token. Line 1, Col: 38.
  ts between 2023-03-01 00:00:00+00:00 and 2023-03-10 23:59:59.999999+00:00
Error: Failed to resolve macro for expression. Error trying to eval macro.

Generated code: self.parse_one(self.template('ts between @start_dt and @end_dt', locals()))

Original sql: @SQL('ts between @start_dt and @end_dt') at '.'
```

It seems that it doesn't take the current dialect into account and just does a string substitution. That same expression in a normal model would correctly expand to `CAST('@start_dt' AS TIMESTAMP) AND CAST('@end_dt' AS TIMESTAMP)`

You can work around the dialect issue by writing SQL to convert the string values to timestamps:

```
condition=@SQL('ts between cast(''@start_dt'' as timestamp) and cast(''@end_dt'' as timestamp)')
```

However, that's a bit hacky. It would be good if the rules for the model SQL dialect were applied


Pull request:
fix: allow macros to recursively resolve closes #2399
macro variables were not resolving additional macro references. also, we ensure conversion of sql to the right dialect when using string templates like `@sql`

cc @erindru 

Patch:
diff --git a/sqlmesh/core/macros.py b/sqlmesh/core/macros.py
index f8edb22d..4fdffe67 100644
--- a/sqlmesh/core/macros.py
+++ b/sqlmesh/core/macros.py
@@ -3,6 +3,7 @@ from __future__ import annotations
 import typing as t
 from enum import Enum
 from functools import reduce
+from itertools import chain
 from string import Template
 
 import sqlglot
@@ -177,7 +178,15 @@ class MacroEvaluator:
                     return node
 
                 value = self.locals[node.name]
-                return exp.convert(tuple(value) if isinstance(value, list) else value)
+                if isinstance(value, list):
+                    return exp.convert(
+                        tuple(
+                            self.transform(v) if isinstance(v, exp.Expression) else v for v in value
+                        )
+                    )
+                return exp.convert(
+                    self.transform(value) if isinstance(value, exp.Expression) else value
+                )
             if isinstance(node, exp.Identifier):
                 text = self.template(node.this, self.locals)
                 if node.this != text:
@@ -222,7 +231,25 @@ class MacroEvaluator:
         Returns:
            The rendered string.
         """
-        return MacroStrTemplate(str(text)).safe_substitute(self.locals, **local_variables)
+        mapping = {}
+
+        for k, v in chain(self.locals.items(), local_variables.items()):
+            # try to convert all variables into sqlglot expressions
+            # because they're going to be converted into strings in sql
+            # we use bare Exception instead of ValueError because there's
+            # a recursive error with MagicMock.
+            # we don't convert strings because that would result in adding quotes
+            if not isinstance(v, str):
+                try:
+                    v = exp.convert(v)
+                except Exception:
+                    pass
+
+            if isinstance(v, exp.Expression):
+                v = v.sql(dialect=self.dialect)
+            mapping[k] = v
+
+        return MacroStrTemplate(str(text)).safe_substitute(mapping)
 
     def evaluate(self, node: MacroFunc) -> exp.Expression | t.List[exp.Expression] | None:
         if isinstance(node, MacroDef):


Test patch:
diff --git a/tests/core/test_audit.py b/tests/core/test_audit.py
index 25ab79fd..f418e0c3 100644
--- a/tests/core/test_audit.py
+++ b/tests/core/test_audit.py
@@ -686,3 +686,15 @@ def test_not_constant_audit(model: Model):
         rendered_query.sql()
         == """SELECT 1 AS "1" FROM (SELECT COUNT(DISTINCT "x") AS "t_cardinality" FROM (SELECT * FROM "db"."test_model" AS "test_model" WHERE "ds" BETWEEN '1970-01-01' AND '1970-01-01') AS "_q_0" WHERE "x" > 1) AS "r" WHERE "r"."t_cardinality" <= 1"""
     )
+
+
+def test_condition_with_macro_var(model: Model):
+    rendered_query = builtin.not_null_audit.render_query(
+        model,
+        columns=[exp.column("x")],
+        condition=exp.condition("dt BETWEEN @start_dt AND @end_dt"),
+    )
+    assert (
+        rendered_query.sql(dialect="duckdb")
+        == """SELECT * FROM (SELECT * FROM "db"."test_model" AS "test_model" WHERE "ds" BETWEEN '1970-01-01' AND '1970-01-01') AS "_q_0" WHERE "x" IS NULL AND "dt" BETWEEN CAST('1970-01-01 00:00:00+00:00' AS TIMESTAMP) AND CAST('1970-01-01 23:59:59.999999+00:00' AS TIMESTAMP)"""
+    )
diff --git a/tests/core/test_macros.py b/tests/core/test_macros.py
index c05b67bf..0c57f487 100644
--- a/tests/core/test_macros.py
+++ b/tests/core/test_macros.py
@@ -332,6 +332,13 @@ def test_ast_correctness(macro_evaluator):
             "SELECT * FROM x ORDER BY a, b",
             {"x": [exp.column("a"), exp.column("b")]},
         ),
+        (
+            """select @SQL('@x')""",
+            "SELECT VAR_MAP('a', 1)",
+            {
+                "x": {"a": 1},
+            },
+        ),
     ],
 )
 def test_macro_functions(macro_evaluator, assert_exp_eq, sql, expected, args):