Unskip and rename test_expression_metric #8578
Conversation
…` and update functions to work on 1.6+ metrics
…tric_dependnecy`
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.
…e_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.
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #8578 +/- ##
==========================================
+ Coverage 86.36% 86.56% +0.19%
==========================================
Files 174 174
Lines 25587 25590 +3
==========================================
+ Hits 22099 22151 +52
+ Misses 3488 3439 -49
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. 📢 Have feedback on the report? Share it here. |
core/dbt/contracts/graph/metrics.py
Outdated
| 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: |
There was a problem hiding this comment.
is there a valid scenario where it'd be valid for a unique_id to exist in metric_node.depends_on.nodes but not be in manifest.metric? If not, we could handle this less gracefully by getting metric from metric = manifest.expect(parent_unique_id) to ensure an issue is not silently handled.
There was a problem hiding this comment.
depends_on contains metrics and things that aren't metrics (like semantic models). So if we were to use manifest.expect(...) we'd have to bring back checking if the returned node is a metric.
There was a problem hiding this comment.
I see! but once we've got what we know is a metric -- should we expect it to ever be None? If not, we could assert/raise an internal error here to make future investigation easier
There was a problem hiding this comment.
Oooo expect does a look up against all the manifest.x objects. I though this would be a bad runtime cost, but these are dictionaries not lists, so doing an in lookup wouldn't be too costly. I'll switch to that as you suggested. That function handles the None case, and thus all we need to do on return is check if it's a metric node.
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.
QMalcolm
deleted the
qmalcolm--8134-unskip-and-rename-test-expression-metric
branch
resolves #8134
Problem
Previously we were skipping
TestMetricHelperFunctions.test_expression_metricleaving all the helper metric helper functions untested. Additionally because this test was being skipped, we didn't realize all the helper functions were broken 😬Solution
Correct, simplify, and add typing to the metric helper functions. Unskip
TestMetricHelperFunctions.test_expression_metricand rename it toTestMetricHelperFunctions.test_derived_metric.Note
I'm not a huge fan of the
parent_metricsfunctions and dependency tree functions returning the originating metric. That seems unnecessary. It'd also simplify the code to not return the originating function. However, as I don't know who depends on these functions, I didn't want to alter their behavior unexpectedly.Checklist