From 1ac2f04974e9c174434dbc04463bf572b78bf675 Mon Sep 17 00:00:00 2001 From: Quigley Malcolm Date: Tue, 5 Sep 2023 13:17:39 -0700 Subject: [PATCH 01/16] Add docstrings to `contracts/graph/metrics.py` functions to document what they do Used [dbt-labs/dbt-core#5607](https://github.com/dbt-labs/dbt-core/pull/5607) for context on what the functions should do. --- core/dbt/contracts/graph/metrics.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/core/dbt/contracts/graph/metrics.py b/core/dbt/contracts/graph/metrics.py index b895aa5e2f5..811a201bd12 100644 --- a/core/dbt/contracts/graph/metrics.py +++ b/core/dbt/contracts/graph/metrics.py @@ -31,6 +31,7 @@ def __str__(self): @classmethod def parent_metrics(cls, metric_node, manifest): + """For a given metric, yeilds all upstream metrics.""" yield metric_node for parent_unique_id in metric_node.depends_on.nodes: @@ -40,6 +41,7 @@ def parent_metrics(cls, metric_node, manifest): @classmethod def parent_metrics_names(cls, metric_node, manifest): + """For a given metric, yeilds all upstream metric names""" yield metric_node.name for parent_unique_id in metric_node.depends_on.nodes: @@ -49,6 +51,10 @@ def parent_metrics_names(cls, metric_node, manifest): @classmethod def reverse_dag_parsing(cls, metric_node, manifest, metric_depth_count): + """For the given metric, yeilds dictionaries having {: : } for all upstream metrics.""" metric_depth_count = 1 to_return = list(self.reverse_dag_parsing(self.node, self.manifest, metric_depth_count)) From b84b5750b95134d34e7f872a2c9d51b02ff5561b Mon Sep 17 00:00:00 2001 From: Quigley Malcolm Date: Wed, 6 Sep 2023 08:48:11 -0700 Subject: [PATCH 02/16] Add typing to `reverse_dag_parsing` and update function to work on 1.6+ metrics --- core/dbt/contracts/graph/metrics.py | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/core/dbt/contracts/graph/metrics.py b/core/dbt/contracts/graph/metrics.py index 811a201bd12..f0526881481 100644 --- a/core/dbt/contracts/graph/metrics.py +++ b/core/dbt/contracts/graph/metrics.py @@ -1,4 +1,12 @@ 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 Dict, Iterator + + +DERIVED_METRICS = [MetricType.DERIVED, MetricType.RATIO] +BASE_METRICS = [MetricType.SIMPLE, MetricType.CUMULATIVE] class MetricReference(object): @@ -50,22 +58,20 @@ def parent_metrics_names(cls, metric_node, manifest): yield from cls.parent_metrics_names(node, manifest) @classmethod - def reverse_dag_parsing(cls, metric_node, manifest, metric_depth_count): + 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 {: Date: Wed, 6 Sep 2023 09:14:42 -0700 Subject: [PATCH 03/16] Add typing to `parent_metrics` and `parent_metrics_names` --- core/dbt/contracts/graph/metrics.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/dbt/contracts/graph/metrics.py b/core/dbt/contracts/graph/metrics.py index f0526881481..aa07ebfeaf2 100644 --- a/core/dbt/contracts/graph/metrics.py +++ b/core/dbt/contracts/graph/metrics.py @@ -38,7 +38,7 @@ def __str__(self): 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 @@ -48,7 +48,7 @@ def parent_metrics(cls, metric_node, manifest): yield from cls.parent_metrics(node, manifest) @classmethod - def parent_metrics_names(cls, metric_node, manifest): + def parent_metrics_names(cls, metric_node: Metric, manifest: Manifest) -> Iterator[str]: """For a given metric, yeilds all upstream metric names""" yield metric_node.name From c8389f9186ce897b24779ed6a9704802415f90e7 Mon Sep 17 00:00:00 2001 From: Quigley Malcolm Date: Wed, 6 Sep 2023 09:26:25 -0700 Subject: [PATCH 04/16] Add typing to `base_metric_dependency` and `derived_metric_dependency` and update functions to work on 1.6+ metrics --- core/dbt/contracts/graph/metrics.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/core/dbt/contracts/graph/metrics.py b/core/dbt/contracts/graph/metrics.py index aa07ebfeaf2..6a6fe6dd3a7 100644 --- a/core/dbt/contracts/graph/metrics.py +++ b/core/dbt/contracts/graph/metrics.py @@ -2,7 +2,7 @@ from dbt.contracts.graph.manifest import Manifest, Metric from dbt_semantic_interfaces.type_enums import MetricType -from typing import Dict, Iterator +from typing import Dict, Iterator, List DERIVED_METRICS = [MetricType.DERIVED, MetricType.RATIO] @@ -79,24 +79,24 @@ def full_metric_dependency(self): 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 list of names for all upstream non-derived metrics.""" in_scope_metrics = list(self.parent_metrics(self.node, self.manifest)) to_return = [] for metric in in_scope_metrics: - if metric.calculation_method != "derived" and metric.name not in to_return: + if metric.type not in DERIVED_METRICS and metric.name not in to_return: to_return.append(metric.name) return to_return - def derived_metric_dependency(self): + def derived_metric_dependency(self) -> List[str]: """Returns a list of names for all upstream derived metrics.""" in_scope_metrics = list(self.parent_metrics(self.node, self.manifest)) to_return = [] for metric in in_scope_metrics: - if metric.calculation_method == "derived" and metric.name not in to_return: + if metric.type in DERIVED_METRICS and metric.name not in to_return: to_return.append(metric.name) return to_return From 8f0c5c61f45375895028b3f865911ea875493bfc Mon Sep 17 00:00:00 2001 From: Quigley Malcolm Date: Wed, 6 Sep 2023 09:32:53 -0700 Subject: [PATCH 05/16] Simplify implementations of `basic_metric_dependency` and `derived_metric_dependnecy` --- core/dbt/contracts/graph/metrics.py | 24 ++++++++++-------------- 1 file changed, 10 insertions(+), 14 deletions(-) diff --git a/core/dbt/contracts/graph/metrics.py b/core/dbt/contracts/graph/metrics.py index 6a6fe6dd3a7..bcc35c8c4cc 100644 --- a/core/dbt/contracts/graph/metrics.py +++ b/core/dbt/contracts/graph/metrics.py @@ -80,26 +80,22 @@ def full_metric_dependency(self): return to_return def base_metric_dependency(self) -> List[str]: - """Returns a list of names for all upstream non-derived metrics.""" + """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.type not in DERIVED_METRICS and metric.name not in to_return: - to_return.append(metric.name) - - return to_return + return list(base_metrics) def derived_metric_dependency(self) -> List[str]: - """Returns a list of names for all upstream derived metrics.""" + """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.type in DERIVED_METRICS 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): """Returns a list of {: } for all upstream metrics.""" From 4e062cb7c09a6f00ec6e0647257fe3765c37fc64 Mon Sep 17 00:00:00 2001 From: Quigley Malcolm Date: Wed, 6 Sep 2023 09:36:50 -0700 Subject: [PATCH 06/16] Add typing to `ResolvedMetricReference` initialization --- core/dbt/contracts/graph/metrics.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/dbt/contracts/graph/metrics.py b/core/dbt/contracts/graph/metrics.py index bcc35c8c4cc..2568d472bfd 100644 --- a/core/dbt/contracts/graph/metrics.py +++ b/core/dbt/contracts/graph/metrics.py @@ -25,7 +25,7 @@ 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, Relation=None): super().__init__(node.name, node.package_name) self.node = node self.manifest = manifest From 4dfe53967331909e69ab7bff5442f8c4e35413cf Mon Sep 17 00:00:00 2001 From: Quigley Malcolm Date: Wed, 6 Sep 2023 09:37:53 -0700 Subject: [PATCH 07/16] Add typing to `derived_metric_dependency_graph` --- core/dbt/contracts/graph/metrics.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/dbt/contracts/graph/metrics.py b/core/dbt/contracts/graph/metrics.py index 2568d472bfd..35a587a3c2f 100644 --- a/core/dbt/contracts/graph/metrics.py +++ b/core/dbt/contracts/graph/metrics.py @@ -97,7 +97,7 @@ def derived_metric_dependency(self) -> List[str]: return list(derived_metrics) - def derived_metric_dependency_depth(self): + def derived_metric_dependency_depth(self) -> List[Dict[str, int]]: """Returns a list of {: } for all upstream metrics.""" metric_depth_count = 1 to_return = list(self.reverse_dag_parsing(self.node, self.manifest, metric_depth_count)) From 4a11c9fd2a5d37754d382a89cb27484da58d5f75 Mon Sep 17 00:00:00 2001 From: Quigley Malcolm Date: Wed, 6 Sep 2023 11:19:03 -0700 Subject: [PATCH 08/16] 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. --- core/dbt/contracts/graph/metrics.py | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/core/dbt/contracts/graph/metrics.py b/core/dbt/contracts/graph/metrics.py index 35a587a3c2f..9d2c3b61b7b 100644 --- a/core/dbt/contracts/graph/metrics.py +++ b/core/dbt/contracts/graph/metrics.py @@ -1,4 +1,3 @@ -from dbt.node_types import NodeType from dbt.contracts.graph.manifest import Manifest, Metric from dbt_semantic_interfaces.type_enums import MetricType @@ -43,9 +42,9 @@ def parent_metrics(cls, metric_node: Metric, manifest: Manifest) -> Iterator[Met 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: - yield from cls.parent_metrics(node, manifest) + metric = manifest.metrics.get(parent_unique_id) + if metric is not None: + yield from cls.parent_metrics(metric, manifest) @classmethod def parent_metrics_names(cls, metric_node: Metric, manifest: Manifest) -> Iterator[str]: @@ -53,9 +52,9 @@ def parent_metrics_names(cls, metric_node: Metric, manifest: Manifest) -> Iterat 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) + metric = manifest.metrics.get(parent_unique_id) + if metric is not None: + yield from cls.parent_metrics_names(metric, manifest) @classmethod def reverse_dag_parsing( @@ -70,9 +69,9 @@ def reverse_dag_parsing( 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.type in DERIVED_METRICS: - yield from cls.reverse_dag_parsing(node, manifest, metric_depth_count) + metric = manifest.metrics.get(parent_unique_id) + if metric is not None and metric.type in DERIVED_METRICS: + yield from cls.reverse_dag_parsing(metric, manifest, metric_depth_count) def full_metric_dependency(self): """Returns a unique list of all upstream metric names.""" From 41c8ad0d9dadf9bc25259c0b8a1fa6f17027486c Mon Sep 17 00:00:00 2001 From: Quigley Malcolm Date: Wed, 6 Sep 2023 11:33:20 -0700 Subject: [PATCH 09/16] 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. --- core/dbt/contracts/graph/metrics.py | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/core/dbt/contracts/graph/metrics.py b/core/dbt/contracts/graph/metrics.py index 9d2c3b61b7b..39d8c3bcecf 100644 --- a/core/dbt/contracts/graph/metrics.py +++ b/core/dbt/contracts/graph/metrics.py @@ -66,12 +66,11 @@ def reverse_dag_parsing( """ 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: - metric = manifest.metrics.get(parent_unique_id) - if metric is not None and metric.type in DERIVED_METRICS: - yield from cls.reverse_dag_parsing(metric, manifest, metric_depth_count) + for parent_unique_id in metric_node.depends_on.nodes: + metric = manifest.metrics.get(parent_unique_id) + if metric is not None: + yield from cls.reverse_dag_parsing(metric, manifest, metric_depth_count + 1) def full_metric_dependency(self): """Returns a unique list of all upstream metric names.""" From b476f28a743ca8ee7ce7409abaacd2a53754ea79 Mon Sep 17 00:00:00 2001 From: Quigley Malcolm Date: Wed, 6 Sep 2023 11:48:24 -0700 Subject: [PATCH 10/16] Simplify `parent_metrics_names` by having it call `parent_metrics` --- core/dbt/contracts/graph/metrics.py | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/core/dbt/contracts/graph/metrics.py b/core/dbt/contracts/graph/metrics.py index 39d8c3bcecf..0b5284ce59e 100644 --- a/core/dbt/contracts/graph/metrics.py +++ b/core/dbt/contracts/graph/metrics.py @@ -49,12 +49,8 @@ def parent_metrics(cls, metric_node: Metric, manifest: Manifest) -> Iterator[Met @classmethod def parent_metrics_names(cls, metric_node: Metric, manifest: Manifest) -> Iterator[str]: """For a given metric, yeilds all upstream metric names""" - yield metric_node.name - - for parent_unique_id in metric_node.depends_on.nodes: - metric = manifest.metrics.get(parent_unique_id) - if metric is not None: - yield from cls.parent_metrics_names(metric, manifest) + for metric in cls.parent_metrics(metric_node, manifest): + yield metric.name @classmethod def reverse_dag_parsing( From 84c509106cc22888cd82ba56e3818a59cf57bfe7 Mon Sep 17 00:00:00 2001 From: Quigley Malcolm Date: Wed, 6 Sep 2023 11:49:47 -0700 Subject: [PATCH 11/16] Unskip `TestMetricHelperFunctions.test_derived_metric` and update fixture setup --- .../metrics/test_metric_helper_functions.py | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/tests/functional/metrics/test_metric_helper_functions.py b/tests/functional/metrics/test_metric_helper_functions.py index ec1015aa637..9b5e3c340ea 100644 --- a/tests/functional/metrics/test_metric_helper_functions.py +++ b/tests/functional/metrics/test_metric_helper_functions.py @@ -3,7 +3,12 @@ from dbt.tests.util import run_dbt, get_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: @@ -11,22 +16,22 @@ class TestMetricHelperFunctions: 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"]) + run_dbt(["parse"]) # make sure all the metrics are in the manifest manifest = get_manifest(project.project_root) + assert manifest is not None parsed_metric = manifest.metrics["metric.test.average_tenure_plus_one"] testing_metric = ResolvedMetricReference(parsed_metric, manifest, None) From b073cfffa04b1f5f30c159a074f469530e5b4e76 Mon Sep 17 00:00:00 2001 From: Quigley Malcolm Date: Wed, 6 Sep 2023 12:02:36 -0700 Subject: [PATCH 12/16] Add changie doc for metric helper function updates --- .changes/unreleased/Fixes-20230906-120212.yaml | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 .changes/unreleased/Fixes-20230906-120212.yaml diff --git a/.changes/unreleased/Fixes-20230906-120212.yaml b/.changes/unreleased/Fixes-20230906-120212.yaml new file mode 100644 index 00000000000..f6237393707 --- /dev/null +++ b/.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" From 711ca98692e38272c32c8d9bdd8060eef635fdb2 Mon Sep 17 00:00:00 2001 From: Quigley Malcolm Date: Wed, 6 Sep 2023 14:53:36 -0700 Subject: [PATCH 13/16] Get manifest in `test_derived_metric` from the parse dbt_run invocation --- tests/functional/metrics/test_metric_helper_functions.py | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/tests/functional/metrics/test_metric_helper_functions.py b/tests/functional/metrics/test_metric_helper_functions.py index 9b5e3c340ea..efa4d6065d9 100644 --- a/tests/functional/metrics/test_metric_helper_functions.py +++ b/tests/functional/metrics/test_metric_helper_functions.py @@ -1,6 +1,7 @@ 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 ( @@ -27,11 +28,9 @@ def test_derived_metric( ): # initial parse - run_dbt(["parse"]) + manifest = run_dbt(["parse"]) + assert isinstance(manifest, Manifest) - # make sure all the metrics are in the manifest - manifest = get_manifest(project.project_root) - assert manifest is not None parsed_metric = manifest.metrics["metric.test.average_tenure_plus_one"] testing_metric = ResolvedMetricReference(parsed_metric, manifest, None) From 70091dfb90928ddc4780b2c6c8b379a3245861e1 Mon Sep 17 00:00:00 2001 From: Quigley Malcolm Date: Wed, 6 Sep 2023 14:58:05 -0700 Subject: [PATCH 14/16] Remove `Relation` a intiatlization attribute for `ResolvedMetricReference` --- core/dbt/context/providers.py | 2 +- core/dbt/contracts/graph/metrics.py | 3 +-- tests/functional/metrics/test_metric_helper_functions.py | 2 +- 3 files changed, 3 insertions(+), 4 deletions(-) diff --git a/core/dbt/context/providers.py b/core/dbt/context/providers.py index ffc1f6d07b4..996d5027c58 100644 --- a/core/dbt/context/providers.py +++ b/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. diff --git a/core/dbt/contracts/graph/metrics.py b/core/dbt/contracts/graph/metrics.py index 0b5284ce59e..5bf7f81909f 100644 --- a/core/dbt/contracts/graph/metrics.py +++ b/core/dbt/contracts/graph/metrics.py @@ -24,11 +24,10 @@ class ResolvedMetricReference(MetricReference): for working with metrics (ie. __str__ and templating functions) """ - def __init__(self, node: Metric, manifest: Manifest, Relation=None): + def __init__(self, node: Metric, manifest: Manifest): super().__init__(node.name, node.package_name) self.node = node self.manifest = manifest - self.Relation = Relation def __getattr__(self, key): return getattr(self.node, key) diff --git a/tests/functional/metrics/test_metric_helper_functions.py b/tests/functional/metrics/test_metric_helper_functions.py index efa4d6065d9..a5775a0ed74 100644 --- a/tests/functional/metrics/test_metric_helper_functions.py +++ b/tests/functional/metrics/test_metric_helper_functions.py @@ -32,7 +32,7 @@ def test_derived_metric( assert isinstance(manifest, Manifest) 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( From da99e599ed54786f37ccf5f66b0387f56b771af8 Mon Sep 17 00:00:00 2001 From: Quigley Malcolm Date: Wed, 6 Sep 2023 14:59:23 -0700 Subject: [PATCH 15/16] Add return typing to class `__` functions of `ResolvedMetricReference` --- core/dbt/contracts/graph/metrics.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/core/dbt/contracts/graph/metrics.py b/core/dbt/contracts/graph/metrics.py index 5bf7f81909f..d7e65540dec 100644 --- a/core/dbt/contracts/graph/metrics.py +++ b/core/dbt/contracts/graph/metrics.py @@ -1,7 +1,7 @@ from dbt.contracts.graph.manifest import Manifest, Metric from dbt_semantic_interfaces.type_enums import MetricType -from typing import Dict, Iterator, List +from typing import Any, Dict, Iterator, List DERIVED_METRICS = [MetricType.DERIVED, MetricType.RATIO] @@ -24,15 +24,15 @@ class ResolvedMetricReference(MetricReference): for working with metrics (ie. __str__ and templating functions) """ - def __init__(self, node: Metric, manifest: Manifest): + def __init__(self, node: Metric, manifest: Manifest) -> None: super().__init__(node.name, node.package_name) self.node = node self.manifest = manifest - 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 From f80b4e1d58bc55c3873f33cb65c6d90dadf15dee Mon Sep 17 00:00:00 2001 From: Quigley Malcolm Date: Thu, 7 Sep 2023 12:45:05 -0700 Subject: [PATCH 16/16] 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. --- core/dbt/contracts/graph/metrics.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/core/dbt/contracts/graph/metrics.py b/core/dbt/contracts/graph/metrics.py index d7e65540dec..c95604cb976 100644 --- a/core/dbt/contracts/graph/metrics.py +++ b/core/dbt/contracts/graph/metrics.py @@ -41,9 +41,9 @@ def parent_metrics(cls, metric_node: Metric, manifest: Manifest) -> Iterator[Met yield metric_node for parent_unique_id in metric_node.depends_on.nodes: - metric = manifest.metrics.get(parent_unique_id) - if metric is not None: - yield from cls.parent_metrics(metric, manifest) + 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: Metric, manifest: Manifest) -> Iterator[str]: @@ -63,9 +63,9 @@ def reverse_dag_parsing( yield {metric_node.name: metric_depth_count} for parent_unique_id in metric_node.depends_on.nodes: - metric = manifest.metrics.get(parent_unique_id) - if metric is not None: - yield from cls.reverse_dag_parsing(metric, manifest, metric_depth_count + 1) + 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."""