Unskip and rename test_expression_metric (#8578)

* Add docstrings to `contracts/graph/metrics.py` functions to document what they do

Used [#5607](#5607)
for context on what the functions should do.

* Add typing to `reverse_dag_parsing` and update function to work on 1.6+ metrics

* Add typing to `parent_metrics` and `parent_metrics_names`

* Add typing to `base_metric_dependency` and `derived_metric_dependency` and update functions to work on 1.6+ metrics

* Simplify implementations of `basic_metric_dependency` and `derived_metric_dependnecy`

* Add typing to `ResolvedMetricReference` initialization

* Add typing to `derived_metric_dependency_graph`

* Simplify conditional controls in `ResolvedMetricReference` functions

The functions in `ResolvedMetricReference` use `manifest.metric.get(...)`
which will only return either a `Metric` or `None`, never a different
node type. Thus we don't need to check that the returned metric is
a metric.

* Don't recurse on over `depends_on` for non-derived metrics in `reverse_dag_parsing`

The function `reverse_dag_parsing` only cares about derived metrics,
that is metrics that depend on other metrics. Metrics only depend on
other metrics if they are one of the `DERIVED_METRICS` types. Thus
doing a recursive call to `reverse_dag_parsing` for non `DERIVED_METRICS`
types is unnecessary. Previously we were iterating over a metric's
`depends_on` property regardless of whether the metric was a `DERIVED_METRICS`
type. Now we only do this work if the metric is of a `DERIVED_METRICS`
type.

* Simplify `parent_metrics_names` by having it call `parent_metrics`

* Unskip `TestMetricHelperFunctions.test_derived_metric` and update fixture setup

* Add changie doc for metric helper function updates

* Get manifest in `test_derived_metric` from the parse dbt_run invocation

* Remove `Relation` a intiatlization attribute for `ResolvedMetricReference`

* Add return typing to class `__` functions of `ResolvedMetricReference`

* Move from `manifest.metrics.get` to `manifest.expect` in metric helpers

Previously with `manifest.metrics.get` we were just skipping when `None`
was returned. Getting `None` back was expected in that `parent_unique_id`s
that didn't belong to metrics should return `None` when calling
`manifest.metrics.get`, and these are fine to skip. However, there's
an edgecase where a `parent_unique_id` is supposed to be a metric, but
isn't found, thus returning `None`. How likely this edge case could
get hit, I'm not sure, but it's a possible edge case. Using `manifest.metrics.get`
it we can't actually tell if we're in the edge case or not. By moving
to `manifest.expect` we get the error handling built in, and the only
trade off is that we need to change our conditional to skip returned
nodes that aren't metrics.

.changes/unreleased/Fixes-20230906-120212.yaml

@@ -0,0 +1,6 @@
+kind: Fixes
+body: Update metric helper functions to work with new semantic layer metrics
+time: 2023-09-06T12:02:12.156534-07:00
+custom:
+  Author: QMalcolm
+  Issue: "8134"

core/dbt/context/providers.py

@@ -618,7 +618,7 @@ def resolve(self, target_name: str, target_package: Optional[str] = None) -> Met
                 target_package=target_package,
             )
 
-        return ResolvedMetricReference(target_metric, self.manifest, self.Relation)
+        return ResolvedMetricReference(target_metric, self.manifest)
 
 
 # `var` implementations.

core/dbt/contracts/graph/metrics.py

@@ -1,4 +1,11 @@
-from dbt.node_types import NodeType
+from dbt.contracts.graph.manifest import Manifest, Metric
+from dbt_semantic_interfaces.type_enums import MetricType
+
+from typing import Any, Dict, Iterator, List
+
+
+DERIVED_METRICS = [MetricType.DERIVED, MetricType.RATIO]
+BASE_METRICS = [MetricType.SIMPLE, MetricType.CUMULATIVE]
 
 
 class MetricReference(object):
@@ -17,76 +24,74 @@ class ResolvedMetricReference(MetricReference):
     for working with metrics (ie. __str__ and templating functions)
     """
 
-    def __init__(self, node, manifest, Relation):
+    def __init__(self, node: Metric, manifest: Manifest) -> None:
         super().__init__(node.name, node.package_name)
         self.node = node
         self.manifest = manifest
-        self.Relation = Relation
 
-    def __getattr__(self, key):
+    def __getattr__(self, key) -> Any:
         return getattr(self.node, key)
 
-    def __str__(self):
+    def __str__(self) -> str:
         return f"{self.node.name}"
 
     @classmethod
-    def parent_metrics(cls, metric_node, manifest):
+    def parent_metrics(cls, metric_node: Metric, manifest: Manifest) -> Iterator[Metric]:
+        """For a given metric, yeilds all upstream metrics."""
         yield metric_node
 
         for parent_unique_id in metric_node.depends_on.nodes:
-            node = manifest.metrics.get(parent_unique_id)
-            if node and node.resource_type == NodeType.Metric:
+            node = manifest.expect(parent_unique_id)
+            if isinstance(node, Metric):
                 yield from cls.parent_metrics(node, manifest)
 
     @classmethod
-    def parent_metrics_names(cls, metric_node, manifest):
-        yield metric_node.name
-
-        for parent_unique_id in metric_node.depends_on.nodes:
-            node = manifest.metrics.get(parent_unique_id)
-            if node and node.resource_type == NodeType.Metric:
-                yield from cls.parent_metrics_names(node, manifest)
+    def parent_metrics_names(cls, metric_node: Metric, manifest: Manifest) -> Iterator[str]:
+        """For a given metric, yeilds all upstream metric names"""
+        for metric in cls.parent_metrics(metric_node, manifest):
+            yield metric.name
 
     @classmethod
-    def reverse_dag_parsing(cls, metric_node, manifest, metric_depth_count):
-        if metric_node.calculation_method == "derived":
+    def reverse_dag_parsing(
+        cls, metric_node: Metric, manifest: Manifest, metric_depth_count: int
+    ) -> Iterator[Dict[str, int]]:
+        """For the given metric, yeilds dictionaries having {<metric_name>: <depth_from_initial_metric} of upstream derived metrics.
+
+        This function is intended as a helper function for other metric helper functions.
+        """
+        if metric_node.type in DERIVED_METRICS:
             yield {metric_node.name: metric_depth_count}
-            metric_depth_count = metric_depth_count + 1
 
-        for parent_unique_id in metric_node.depends_on.nodes:
-            node = manifest.metrics.get(parent_unique_id)
-            if (
-                node
-                and node.resource_type == NodeType.Metric
-                and node.calculation_method == "derived"
-            ):
-                yield from cls.reverse_dag_parsing(node, manifest, metric_depth_count)
+            for parent_unique_id in metric_node.depends_on.nodes:
+                node = manifest.expect(parent_unique_id)
+                if isinstance(node, Metric):
+                    yield from cls.reverse_dag_parsing(node, manifest, metric_depth_count + 1)
 
     def full_metric_dependency(self):
+        """Returns a unique list of all upstream metric names."""
         to_return = list(set(self.parent_metrics_names(self.node, self.manifest)))
         return to_return
 
-    def base_metric_dependency(self):
+    def base_metric_dependency(self) -> List[str]:
+        """Returns a unique list of names for all upstream non-derived metrics."""
         in_scope_metrics = list(self.parent_metrics(self.node, self.manifest))
+        base_metrics = {
+            metric.name for metric in in_scope_metrics if metric.type not in DERIVED_METRICS
+        }
 
-        to_return = []
-        for metric in in_scope_metrics:
-            if metric.calculation_method != "derived" and metric.name not in to_return:
-                to_return.append(metric.name)
-
-        return to_return
+        return list(base_metrics)
 
-    def derived_metric_dependency(self):
+    def derived_metric_dependency(self) -> List[str]:
+        """Returns a unique list of names for all upstream derived metrics."""
         in_scope_metrics = list(self.parent_metrics(self.node, self.manifest))
+        derived_metrics = {
+            metric.name for metric in in_scope_metrics if metric.type in DERIVED_METRICS
+        }
 
-        to_return = []
-        for metric in in_scope_metrics:
-            if metric.calculation_method == "derived" and metric.name not in to_return:
-                to_return.append(metric.name)
-
-        return to_return
+        return list(derived_metrics)
 
-    def derived_metric_dependency_depth(self):
+    def derived_metric_dependency_depth(self) -> List[Dict[str, int]]:
+        """Returns a list of {<metric_name>: <depth_from_initial_metric>} for all upstream metrics."""
         metric_depth_count = 1
         to_return = list(self.reverse_dag_parsing(self.node, self.manifest, metric_depth_count))
 

tests/functional/metrics/test_metric_helper_functions.py

@@ -1,34 +1,38 @@
 import pytest
 
-from dbt.tests.util import run_dbt, get_manifest
+from dbt.tests.util import run_dbt
+from dbt.contracts.graph.manifest import Manifest
 from dbt.contracts.graph.metrics import ResolvedMetricReference
 
-from tests.functional.metrics.fixtures import models_people_sql, basic_metrics_yml
+from tests.functional.metrics.fixtures import (
+    models_people_sql,
+    basic_metrics_yml,
+    semantic_model_people_yml,
+    metricflow_time_spine_sql,
+)
 
 
 class TestMetricHelperFunctions:
     @pytest.fixture(scope="class")
     def models(self):
         return {
             "metrics.yml": basic_metrics_yml,
+            "semantic_people.yml": semantic_model_people_yml,
+            "metricflow_time_spine.sql": metricflow_time_spine_sql,
             "people.sql": models_people_sql,
         }
 
-    @pytest.mark.skip(
-        "TODO reactivate after we begin property hydrating metric `depends_on` and `refs`"
-    )
-    def test_expression_metric(
+    def test_derived_metric(
         self,
         project,
     ):
 
         # initial parse
-        run_dbt(["compile"])
+        manifest = run_dbt(["parse"])
+        assert isinstance(manifest, Manifest)
 
-        # make sure all the metrics are in the manifest
-        manifest = get_manifest(project.project_root)
         parsed_metric = manifest.metrics["metric.test.average_tenure_plus_one"]
-        testing_metric = ResolvedMetricReference(parsed_metric, manifest, None)
+        testing_metric = ResolvedMetricReference(parsed_metric, manifest)
 
         full_metric_dependency = set(testing_metric.full_metric_dependency())
         expected_full_metric_dependency = set(