Fix multiple semantic models with generated metrics #10451
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #10451 +/- ##
==========================================
- Coverage 88.79% 88.76% -0.03%
==========================================
Files 180 180
Lines 22562 22586 +24
==========================================
+ Hits 20034 20049 +15
- Misses 2528 2537 +9
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
Thanks for reproducing this and adding the fix!
Understanding what's going on here is a bit tricky, can we add a bit more comments? And maybe do a live walk through with the team to pass the knowledge along?
Maybe this is because I am not familiar with how this section of the logic works.
(sorry I meant to comment vs approve, clicked the wrong button).
core/dbt/contracts/files.py
Outdated
| def fix_metrics_from_measures(self): | ||
| # Loop through yaml dictionary looking for matching generated metrics | ||
| generated_metrics = self.generated_metrics | ||
| self.generated_metrics = [] |
There was a problem hiding this comment.
Are we using this manipulation of generated_metrics to avoid fix_metrics_from_measures and fix_metrics_from_measures infinite looping? Can we try to organize the function so it is easier to read?
There was a problem hiding this comment.
Is it possible to add some unittest to test this new logic added? I see the coverage is only 10%-ish.
| self.delete_disabled(unique_id, schema_file.file_id) | ||
| if schema_file.generated_metrics: | ||
| schema_file.fix_metrics_from_measures() | ||
| if semantic_model_name in schema_file.metrics_from_measures: |
There was a problem hiding this comment.
Can we add a bit of comment here?
| @@ -146,3 +147,28 @@ def test_semantic_model_flipping_create_metric_partial_parsing(self, project): | |||
| # Verify the metric originally created by `create_metric: true` was removed | |||
| metric = result.result.metrics[generated_metric] | |||
| assert metric.name == "txn_revenue" | |||
|
|
|||
|
|
|||
| class TestSemanticModelPartialParsingGeneratedMetrics: | |||
| @@ -968,13 +968,17 @@ def delete_schema_semantic_model(self, schema_file, semantic_model_dict): | |||
| elif unique_id in self.saved_manifest.disabled: | |||
| self.delete_disabled(unique_id, schema_file.file_id) | |||
|
|
|||
| metrics = schema_file.generated_metrics.copy() | |||
There was a problem hiding this comment.
So here we were deleting all generated metrics instead of just the one related to a semantic model
resolves #10450
Problem
When partial parsing, all generated metrics in a file were being deleted instead of just for the changed semantic model.
Solution
Use a dictionary in SchemaSourceFile, with the semantic model name for the key and a list containing metric unique ids for the value.
Checklist