-
Notifications
You must be signed in to change notification settings - Fork 1.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Propagate the measure label when using create_metric option #10536
Propagate the measure label when using create_metric option #10536
Conversation
Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide. |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #10536 +/- ##
=======================================
Coverage 88.84% 88.84%
=======================================
Files 180 180
Lines 22717 22720 +3
=======================================
+ Hits 20182 20186 +4
+ Misses 2535 2534 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
9342b0a
to
be26928
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me. It would probably be worthwhile to add a test which checks the state of the created metric when the related measure does not have a label set, however this is not blocking.
Context
When _create_metric
, which creates a metric from a measure, was written, measures did not have labels. It seems that when label
was added to the measure class, we did not think to add it here. That is an oversight, but I can see how that happened.
Implications
For metrics created from measures, in order to get the correct label, a full reparse will be necessary. This is because partial parsing only reparses an object is the input for the object has changed. Thus, without a full parsing, the relevant metrics will only get updated if the underlying semantic model is modified somehow.
TL;DR
Looks good. People will need to run dbt parse --no-partial-parse
to ensure all existing measures with labels that create metrics properly propagate the label.
be26928
to
402c4c6
Compare
402c4c6
to
e867392
Compare
@QMalcolm thanks for the context! I updated the test. I don't have permission to merge so I'll leave the merging to you |
Hey @QMalcolm, @WilliamDee measures:
- name: metric_measure
description: This is a measure (originally)
label: This is the label value
expr: order_id
agg: count_distinct
create_metric: true After running both {
...
"metric.vec_sales.metric_measure":
{
"name": "metric_measure",
"resource_type": "metric",
"package_name": "vec_sales",
"path": "vehicle_sales.yml",
"original_file_path": "models/vehicle_sales.yml",
"unique_id": "metric.vec_sales.metric_measure",
"fqn": ["vec_sales", "metric_measure"],
"description": "This is a measure (originally)",
"label": "metric_measure",
...
} The (dbt-latest) ➜ vec_sales pip freeze | grep dbt
dbt-adapters==1.10.3
dbt-common==1.13.0
dbt-core==1.8.9
dbt-extractor==0.5.1
dbt-postgres==1.8.2
dbt-semantic-interfaces==0.5.1 |
@Vitor-Avila howdy! Looks like this never got backported to 1.8.latest and thus never went into a 1.8.x minor release. I can check in with the team if that is something we want to do, it shouldn't be a major lift. However, we also plan to release 1.9.0 very soon. It should be fixed in that version. If you want a sneak peak of that, try installing the latest release candidate via |
Thank you so much for the quick response, @QMalcolm! 🙏 I've just tested using |
Resolves #10538
Problem
When using create_metric on a measure, the label of the measure is not propagated to the metric
Solution
Set metric label to be measure label if it exists
Checklist