-
Notifications
You must be signed in to change notification settings - Fork 23
Transform measures with create_metric true to actual metrics #417
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
Merged
theyostalservice
merged 2 commits into
main
from
patricky/create_proxy_metric_makes_independent_metric
Sep 26, 2025
+176
−2
Merged
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| kind: Under the Hood | ||
| body: 'Updated transformation for measures with create_metric: True to create an independent metric.' | ||
| time: 2025-09-10T17:28:13.240814-07:00 | ||
| custom: | ||
| Author: theyostalservice | ||
| Issue: "387" |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -44,7 +44,7 @@ def transform_model(semantic_manifest: PydanticSemanticManifest) -> PydanticSema | |
| if metric.type != MetricType.SIMPLE: | ||
| raise ModelTransformError( | ||
| f"Cannot have metric with the same name as a measure ({measure.name}) that is not a " | ||
| f"proxy for that measure" | ||
| f"created mechanically from that measure using create_metric=True" | ||
| ) | ||
| logger.warning( | ||
| f"Metric already exists with name ({measure.name}). *Not* adding measure proxy metric for " | ||
|
|
@@ -58,9 +58,18 @@ def transform_model(semantic_manifest: PydanticSemanticManifest) -> PydanticSema | |
| name=measure.name, | ||
| type=MetricType.SIMPLE, | ||
| type_params=PydanticMetricTypeParams( | ||
| # Measure is left here for backward compatibility. It will not be | ||
| # used by metricflow. | ||
| measure=PydanticMetricInputMeasure(name=measure.name), | ||
| expr=measure.name, | ||
| metric_aggregation_params=PydanticMetric.get_metric_aggregation_params( | ||
| measure=measure, | ||
| semantic_model_name=semantic_model.name, | ||
| ), | ||
| expr=measure.expr, | ||
| ), | ||
| description=measure.description, | ||
| label=measure.label, | ||
| config=measure.config, | ||
|
Comment on lines
+68
to
+72
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How silly that we were ignoring these before! Thank you! |
||
| ) | ||
| ) | ||
|
|
||
|
|
||
141 changes: 141 additions & 0 deletions
141
tests/transformations/test_proxy_measure_transformation_rule.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,141 @@ | ||
| import textwrap | ||
| from typing import List, Optional | ||
|
|
||
| from dbt_semantic_interfaces.implementations.elements.measure import PydanticMeasure | ||
| from dbt_semantic_interfaces.implementations.metric import ( | ||
| PydanticMetric, | ||
| PydanticMetricAggregationParams, | ||
| PydanticMetricInputMeasure, | ||
| ) | ||
| from dbt_semantic_interfaces.implementations.semantic_manifest import ( | ||
| PydanticSemanticManifest, | ||
| ) | ||
| from dbt_semantic_interfaces.parsing.dir_to_model import ( | ||
| SemanticManifestBuildResult, | ||
| parse_yaml_files_to_validation_ready_semantic_manifest, | ||
| ) | ||
| from dbt_semantic_interfaces.parsing.objects import YamlConfigFile | ||
| from dbt_semantic_interfaces.type_enums.metric_type import MetricType | ||
| from tests.example_project_configuration import ( | ||
| EXAMPLE_PROJECT_CONFIGURATION_YAML_CONFIG_FILE, | ||
| ) | ||
|
|
||
|
|
||
| def test_measure_with_create_metric_generates_metric_with_equivalent_agg_params() -> None: | ||
| """Test that a measure with create_metric: true generates a metric with equivalent agg params.""" | ||
| primary_entity_name = "example_entity" | ||
| measure_name = "my_sum_measure" | ||
| measure_agg = "percentile" | ||
| measure_expr = "this_expr" | ||
| measure_agg_time_dimension = "ds" | ||
| semantic_model_name = "proxy_measure_test_model" | ||
| measure_non_additive_dimension_time_dimension = "ds" | ||
| measure_non_additive_dimension = "is_instant" | ||
| measure_non_additive_dimension_window_choice = "min" | ||
| measure_non_additive_dimension_window_grouping = primary_entity_name | ||
| measure_description = "A beautiful description for a beautiful measure!" | ||
| measure_label = "Read me, human! I'm a label!" | ||
|
Comment on lines
+36
to
+37
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 😂 😂 😂 😂 😂 |
||
|
|
||
| yaml_contents = textwrap.dedent( | ||
| f"""\ | ||
| semantic_model: | ||
| name: {semantic_model_name} | ||
| node_relation: | ||
| schema_name: some_schema | ||
| alias: source_table | ||
| entities: | ||
| - name: {primary_entity_name} | ||
| type: primary | ||
| role: test_role | ||
| expr: example_id | ||
| measures: | ||
| - name: {measure_name} | ||
| description: {measure_description} | ||
| label: {measure_label} | ||
| agg: {measure_agg} | ||
| agg: percentile | ||
| agg_params: | ||
| percentile: 0.99 | ||
| agg_time_dimension: {measure_agg_time_dimension} | ||
| expr: {measure_expr} | ||
| non_additive_dimension: | ||
| name: {measure_non_additive_dimension} | ||
| window_choice: {measure_non_additive_dimension_window_choice} | ||
| window_groupings: | ||
| - {measure_non_additive_dimension_window_grouping} | ||
| config: | ||
| meta: | ||
| text_tag_1: whoot | ||
| text_tag_2: beepboop | ||
| create_metric: true | ||
| dimensions: | ||
| - name: {measure_agg_time_dimension} | ||
| type: time | ||
| type_params: | ||
| time_granularity: day | ||
| - name: {measure_non_additive_dimension_time_dimension} | ||
| type: time | ||
| type_params: | ||
| time_granularity: day | ||
| - name: {measure_non_additive_dimension} | ||
| type: time | ||
| type_params: | ||
| time_granularity: day | ||
| """ | ||
| ) | ||
| yaml_file = YamlConfigFile(filepath="inline_for_test", contents=yaml_contents) | ||
| model_build_result: SemanticManifestBuildResult = parse_yaml_files_to_validation_ready_semantic_manifest( | ||
| [EXAMPLE_PROJECT_CONFIGURATION_YAML_CONFIG_FILE, yaml_file], | ||
| apply_transformations=True, | ||
| raise_issues_as_exceptions=True, | ||
| ) | ||
|
|
||
| # Sanity check - there should exactly 1 measure and 1 metric | ||
| measures: List[PydanticMeasure] = [] | ||
| manifest: PydanticSemanticManifest = model_build_result.semantic_manifest | ||
| for sm in manifest.semantic_models: | ||
| measures.extend(sm.measures) | ||
| metrics: List[PydanticMetric] = manifest.metrics | ||
| assert len(measures) == 1 | ||
| assert len(metrics) == 1 | ||
|
|
||
| # Get the metric we expect with correct metadata | ||
| metric = metrics[0] | ||
| assert metric.name == measure_name, "Metric was constructed with the wrong name." | ||
| assert metric.type == MetricType.SIMPLE, "Constructed metric MUST be a simple metric." | ||
| assert metric.description == measure_description, "Metric was constructed with the wrong description." | ||
| assert metric.label == measure_label, "Metric was constructed with the wrong label." | ||
| assert metric.config is not None, "Metric should have a config, but it is missing." | ||
| meta = metric.config.meta | ||
| assert meta is not None, "Metric should have a 'meta' value in its config." | ||
| assert meta.get("text_tag_1") == "whoot", "Metric was constructed with the wrong config values." | ||
| assert meta.get("text_tag_2") == "beepboop", "Metric was constructed with the wrong config values." | ||
|
|
||
| # Metric fields that specific to metrics that replace measures | ||
| metric_agg_params: Optional[PydanticMetricAggregationParams] = metric.type_params.metric_aggregation_params | ||
| assert metric_agg_params is not None, "Metric aggregation params were not populated but they should have been." | ||
| assert ( | ||
| metric_agg_params.semantic_model == semantic_model_name | ||
| ), "Metric was constructed with the wrong semantic model name." | ||
| assert metric_agg_params.agg.value == measure_agg, "Metric was constructed with the wrong aggregation type." | ||
| assert ( | ||
| metric_agg_params.agg_time_dimension == measure_agg_time_dimension | ||
| ), "Metric was constructed with the wrong agg time dimension." | ||
| non_additive_dimension = metric_agg_params.non_additive_dimension | ||
| assert non_additive_dimension is not None, "Metric should have been constructed with a non-additive dimension." | ||
| assert ( | ||
| non_additive_dimension.name == measure_non_additive_dimension | ||
| ), "Metric was constructed with the wrong non-additive dimension name." | ||
| assert ( | ||
| non_additive_dimension.window_choice.value == measure_non_additive_dimension_window_choice | ||
| ), "Metric was constructed with the wrong non-additive dimension window choice." | ||
| assert non_additive_dimension.window_groupings == [ | ||
| measure_non_additive_dimension_window_grouping | ||
| ], "Metric was constructed with the wrong non-additive dimension window groupings." | ||
|
|
||
| assert metric.type_params.expr == measure_expr, "Metric was constructed with the wrong expr." | ||
|
|
||
| # Finally, make sure it still references the measure | ||
| assert metric.type_params.measure == PydanticMetricInputMeasure( | ||
| name=measure_name | ||
| ), "Metric was constructed with the wrong measure." | ||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Ooo - I wonder if we have a validation for this? i.e., transformations currently run after validations so I don't think we would catch this until query time. Not part of this PR / project, but would be good to put up a ticket for that if we don't have it already
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.
Added as SL-4241
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.
Revisiting this msg with some things I didn't catch - I think its original intent was this:
create_metric=True, but you also have a simple metric with the same name, we'll ignore thecreate_metriccue and log a warning. I don't think the user will ever actually see this warning since it runs in MF.create_metric=True. Otherwise we don't care if there is a name conflict between a measure and metric.So I think we need 2 things here:
create_metric=Truehas the same name as another metric in the model (any metric type).