Skip to content

Commit

Permalink
Move from manifest.metrics.get to manifest.expect in metric helpers
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
QMalcolm committed Sep 7, 2023
1 parent da99e59 commit f80b4e1
Showing 1 changed file with 6 additions and 6 deletions.
12 changes: 6 additions & 6 deletions core/dbt/contracts/graph/metrics.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):

Check warning on line 45 in core/dbt/contracts/graph/metrics.py

View check run for this annotation

Codecov / codecov/patch

core/dbt/contracts/graph/metrics.py#L44-L45

Added lines #L44 - L45 were not covered by tests
yield from cls.parent_metrics(node, manifest)

@classmethod
def parent_metrics_names(cls, metric_node: Metric, manifest: Manifest) -> Iterator[str]:
Expand All @@ -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)

Check warning on line 68 in core/dbt/contracts/graph/metrics.py

View check run for this annotation

Codecov / codecov/patch

core/dbt/contracts/graph/metrics.py#L65-L68

Added lines #L65 - L68 were not covered by tests

def full_metric_dependency(self):
"""Returns a unique list of all upstream metric names."""
Expand Down

0 comments on commit f80b4e1

Please sign in to comment.