Skip to content

Commit 3e3a6ac

Browse files
committed
fix: validate metric dependency references
Resolve metric dependencies iteratively and report cycles or unknown references with their dependency chain and defining file. Preserve forward references, shared dependencies, and case-insensitive metric names with execution-based regression coverage. Signed-off-by: tchivs <topivn@live.cn>
1 parent b1e36b9 commit 3e3a6ac

3 files changed

Lines changed: 211 additions & 182 deletions

File tree

docs/concepts/metrics/definition.md

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,7 @@ Because the `prod.users.country` and `prod.searches.num_searches` models have sp
5959

6060
Metrics can perform additional operations/calculations with other metrics.
6161

62-
In this example, the third metric `clicks_per_search` is calculated by dividing the first metric `total_searches` by the second metric `total_clicks`:
62+
In this example, the third metric `clicks_per_search` is calculated by dividing `total_clicks` by `total_searches`:
6363

6464
```sql linenums="1"
6565
METRIC (
@@ -78,6 +78,8 @@ METRIC (
7878
);
7979
```
8080

81+
Metric references are case insensitive, including quoted references, and may refer to metrics defined later in the project. Shared dependencies are resolved once during expansion. An unknown dependency or a dependency cycle raises a configuration error with the dependency chain and the path of the definition containing the invalid reference.
82+
8183
## Properties
8284

8385
The `METRIC` definition supports the following keys. The `name` and `expression` keys are required.

sqlmesh/core/metric/definition.py

Lines changed: 65 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -99,43 +99,71 @@ def to_metric(
9999
self, metas: t.Dict[str, MetricMeta], metrics: UniqueKeyDict[str, Metric]
100100
) -> Metric:
101101
"""Converts a metric meta into a fully expanded and standalone metric."""
102-
metric_refs = {}
103-
agg_or_ref = False
104-
105-
for node in self.expression.walk():
106-
if isinstance(node, exp.Alias):
107-
_raise_metric_config_error(
108-
f"Alias found for metric '{self.name}' which is not allowed", self._path
109-
)
110-
elif isinstance(node, exp.AggFunc):
111-
agg_or_ref = True
112-
elif isinstance(node, exp.Column) and not node.table:
113-
agg_or_ref = True
114-
ref = node.sql(dialect=self.dialect)
115-
116-
if ref not in metrics:
117-
metrics[ref] = metas[ref].to_metric(metas, metrics)
118-
119-
metric_refs[node] = metrics[ref]
120-
121-
if not agg_or_ref:
122-
_raise_metric_config_error(
123-
f"Metric '{self.name}' missing an aggregation or metric ref", self._path
124-
)
125-
126-
if metric_refs:
127-
expanded = self.expression.copy()
128-
for column in expanded.find_all(exp.Column):
129-
metric = metric_refs.get(column)
130-
131-
if metric:
132-
column.replace(metric.expanded.copy())
133-
else:
134-
expanded = exp.alias_(self.expression, self.name)
135-
136-
metric = Metric(**self.dict(), expanded=expanded)
137-
metric._path = self._path
138-
return metric
102+
# Suspend each expression walk while resolving a dependency so cycles of
103+
# any depth can be diagnosed without using the Python call stack.
104+
stack: t.List[t.Tuple[MetricMeta, t.Iterator[exp.Expr], t.Dict[exp.Column, str], bool]] = [
105+
(self, self.expression.walk(), {}, False)
106+
]
107+
visiting = {self.name}
108+
109+
while True:
110+
meta, nodes, metric_refs, agg_or_ref = stack.pop()
111+
112+
for node in nodes:
113+
if isinstance(node, exp.Alias):
114+
_raise_metric_config_error(
115+
f"Alias found for metric '{meta.name}' which is not allowed", meta._path
116+
)
117+
elif isinstance(node, exp.AggFunc):
118+
agg_or_ref = True
119+
elif isinstance(node, exp.Column) and not node.table:
120+
agg_or_ref = True
121+
ref = node.name.lower()
122+
metric_refs[node] = ref
123+
124+
if ref not in metrics:
125+
is_cycle = ref in visiting
126+
if is_cycle or ref not in metas:
127+
dependency_path = " -> ".join(
128+
[frame[0].name for frame in stack] + [meta.name, ref]
129+
)
130+
if is_cycle:
131+
message = f"Metric dependency cycle detected: {dependency_path}"
132+
else:
133+
message = (
134+
f"Unknown metric '{ref}' referenced by metric '{meta.name}' "
135+
f"(dependency path: {dependency_path})"
136+
)
137+
_raise_metric_config_error(message, meta._path)
138+
139+
dependency = metas[ref]
140+
stack.append((meta, nodes, metric_refs, agg_or_ref))
141+
stack.append((dependency, dependency.expression.walk(), {}, False))
142+
visiting.add(ref)
143+
break
144+
else:
145+
if not agg_or_ref:
146+
_raise_metric_config_error(
147+
f"Metric '{meta.name}' missing an aggregation or metric ref", meta._path
148+
)
149+
150+
if metric_refs:
151+
expanded = meta.expression.copy()
152+
for column in expanded.find_all(exp.Column):
153+
reference = metric_refs.get(column)
154+
if reference is not None:
155+
column.replace(metrics[reference].expanded.copy())
156+
else:
157+
expanded = exp.alias_(meta.expression, meta.name)
158+
159+
metric = Metric(**meta.dict(), expanded=expanded)
160+
metric._path = meta._path
161+
visiting.remove(meta.name)
162+
163+
if not stack:
164+
return metric
165+
166+
metrics[meta.name] = metric
139167

140168

141169
class Metric(MetricMeta, frozen=True):

0 commit comments

Comments
 (0)