-
Notifications
You must be signed in to change notification settings - Fork 23
Validations for when measures with create=true have name overlaps with metrics #446
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
base: graphite-base/446
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| kind: Under the Hood | ||
| body: 'Add validation that measures with create_metric: True don''t have name overlaps with metrics.' | ||
| time: 2025-10-14T16:51:42.9386-07:00 | ||
| custom: | ||
| Author: theyostalservice | ||
| Issue: "387" |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,6 +9,7 @@ | |
| SemanticManifestT, | ||
| ) | ||
| from dbt_semantic_interfaces.references import MeasureReference, MetricModelReference | ||
| from dbt_semantic_interfaces.type_enums.metric_type import MetricType | ||
| from dbt_semantic_interfaces.validations.shared_measure_and_metric_helpers import ( | ||
| SharedMeasureAndMetricHelpers, | ||
| ) | ||
|
|
@@ -37,6 +38,7 @@ class SemanticModelMeasuresUniqueRule(SemanticManifestValidationRule[SemanticMan | |
| def validate_manifest(semantic_manifest: SemanticManifestT) -> Sequence[ValidationIssue]: # noqa: D | ||
| issues: List[ValidationIssue] = [] | ||
|
|
||
| metrics_by_name = {metric.name: metric for metric in getattr(semantic_manifest, "metrics", [])} | ||
| measure_references_to_semantic_models: Dict[MeasureReference, List] = defaultdict(list) | ||
| for semantic_model in semantic_manifest.semantic_models: | ||
| for measure in semantic_model.measures: | ||
|
|
@@ -56,6 +58,50 @@ def validate_manifest(semantic_manifest: SemanticManifestT) -> Sequence[Validati | |
| ) | ||
| measure_references_to_semantic_models[measure.reference].append(semantic_model.name) | ||
|
|
||
| # As we iterate over all measures, check for name clash with metrics | ||
| metric = metrics_by_name.get(measure.name) | ||
| if metric is not None: | ||
| file_context = FileContext.from_metadata(metadata=semantic_model.metadata) | ||
| element_context = SemanticModelElementContext( | ||
| file_context=file_context, | ||
| semantic_model_element=SemanticModelElementReference( | ||
| semantic_model_name=semantic_model.name, | ||
| element_name=measure.name, | ||
| ), | ||
| element_type=SemanticModelElementType.MEASURE, | ||
| ) | ||
| metric_type = getattr(metric, "type", None) | ||
|
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.
|
||
| if metric_type == MetricType.SIMPLE: | ||
| issues.append( | ||
|
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. Am I missing the part where we check if there was |
||
| ValidationWarning( | ||
| context=element_context, | ||
| message=( | ||
| f"A measure with name '{measure.name}' exists in semantic model " | ||
| f"'{semantic_model.name}', and a simple metric with the same name exists in " | ||
| "the project. This may result in ambiguous behavior. The existing metric will " | ||
| "take precedence over the proxy metric that would be auto-generated from this " | ||
| "measure." | ||
| ), | ||
| ) | ||
| ) | ||
| else: | ||
| issues.append( | ||
| ValidationError( | ||
|
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. Going back to the thread on the other PR:
We actually need to make both of these warnings instead of errors, unfortunately! Reason being that it will be a breaking change if we make either of them errors since the error they would have hit wouldn't have been until query time in MF. This error would unexpectedly break their jobs and cause failures for things totally unrelated to SL. But that means we don't need to check the type and we can consolidate to one validation. |
||
| context=element_context, | ||
| message=( | ||
| ( | ||
| f"Cannot auto-generate a proxy metric for measure " | ||
| f"'{measure.name}' in semantic model '{semantic_model.name}' " | ||
| "because a metric with the same name already exists in the project. " | ||
| "This is elevated to an error instead of a warning because the metric " | ||
| "is not a simple metric " | ||
| f"(found type: '{metric_type}'). Rename the measure or the metric " | ||
| "to resolve the collision." | ||
| ) | ||
| ), | ||
| ) | ||
| ) | ||
|
|
||
| return issues | ||
|
|
||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,89 @@ | ||
| from dbt_semantic_interfaces.implementations.elements.measure import PydanticMeasure | ||
| from dbt_semantic_interfaces.implementations.metric import ( | ||
| PydanticMetricInputMeasure, | ||
| PydanticMetricTypeParams, | ||
| ) | ||
| from dbt_semantic_interfaces.implementations.semantic_manifest import ( | ||
| PydanticSemanticManifest, | ||
| ) | ||
| from dbt_semantic_interfaces.test_utils import ( | ||
| metric_with_guaranteed_meta, | ||
| semantic_model_with_guaranteed_meta, | ||
| ) | ||
| from dbt_semantic_interfaces.type_enums import AggregationType, MetricType | ||
| from dbt_semantic_interfaces.validations.measures import SemanticModelMeasuresUniqueRule | ||
| from dbt_semantic_interfaces.validations.semantic_manifest_validator import ( | ||
| SemanticManifestValidator, | ||
| ) | ||
| from tests.example_project_configuration import EXAMPLE_PROJECT_CONFIGURATION | ||
| from tests.validations.validation_test_utils import check_error_in_issues | ||
|
|
||
|
|
||
| def test_measure_and_simple_metric_same_name_warns() -> None: | ||
| """If a metric shares a name with a measure and is SIMPLE, return a validation warning.""" | ||
| measure_name = "num_sample_rows" | ||
| semantic_model_name = "sample_semantic_model" | ||
|
|
||
| semantic_model = semantic_model_with_guaranteed_meta( | ||
| name=semantic_model_name, | ||
| measures=[PydanticMeasure(name=measure_name, agg=AggregationType.SUM, expr="1")], | ||
| ) | ||
| semantic_model.measures[0].create_metric = True | ||
|
|
||
| simple_metric = metric_with_guaranteed_meta( | ||
| name=measure_name, | ||
| type=MetricType.SIMPLE, | ||
| type_params=PydanticMetricTypeParams(measure=PydanticMetricInputMeasure(name=measure_name)), | ||
| ) | ||
|
|
||
| manifest = PydanticSemanticManifest( | ||
| semantic_models=[semantic_model], | ||
| metrics=[simple_metric], | ||
| project_configuration=EXAMPLE_PROJECT_CONFIGURATION, | ||
| ) | ||
|
|
||
| validation_results = SemanticManifestValidator[PydanticSemanticManifest]( | ||
| [SemanticModelMeasuresUniqueRule()] | ||
| ).validate_semantic_manifest(manifest) | ||
|
|
||
| expected_warning = ( | ||
| f"A measure with name '{measure_name}' exists in semantic model '{semantic_model_name}', and a simple metric" | ||
| ) | ||
| check_error_in_issues(error_substrings=[expected_warning], issues=validation_results.warnings) | ||
|
|
||
|
|
||
| def test_measure_and_non_simple_metric_same_name_errors() -> None: | ||
| """If a metric shares a name with a measure and is NOT SIMPLE, return a validation error.""" | ||
| measure_name = "num_sample_rows" | ||
| semantic_model_name = "sample_semantic_model" | ||
|
|
||
| semantic_model = semantic_model_with_guaranteed_meta( | ||
| name=semantic_model_name, | ||
| measures=[PydanticMeasure(name=measure_name, agg=AggregationType.SUM, expr="1")], | ||
| ) | ||
| semantic_model.measures[0].create_metric = True | ||
|
|
||
| non_simple_metric = metric_with_guaranteed_meta( | ||
| name=measure_name, | ||
| type=MetricType.DERIVED, | ||
| type_params=PydanticMetricTypeParams(expr=measure_name), | ||
| ) | ||
|
|
||
| manifest = PydanticSemanticManifest( | ||
| semantic_models=[semantic_model], | ||
| metrics=[non_simple_metric], | ||
| project_configuration=EXAMPLE_PROJECT_CONFIGURATION, | ||
| ) | ||
|
|
||
| validation_results = SemanticManifestValidator[PydanticSemanticManifest]( | ||
| [SemanticModelMeasuresUniqueRule()] | ||
| ).validate_semantic_manifest(manifest) | ||
|
|
||
| expected_error = ( | ||
| f"Cannot auto-generate a proxy metric for measure '{measure_name}' in semantic " | ||
| f"model '{semantic_model_name}' because a metric with the same name already exists " | ||
| f"in the project. This is elevated to an error instead of a warning because the " | ||
| f"metric is not a simple metric (found type: '{MetricType.DERIVED}'). Rename the " | ||
| f"measure or the metric to resolve the collision." | ||
| ) | ||
| check_error_in_issues(error_substrings=[expected_error], issues=validation_results.errors) | ||
|
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. Let's also test that these validations don't get thrown if there is a name conflict but the measure doesn't use |
||
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.
what's the reason for using
getattrhere instead of accessing the property directly? seems like we get better type safety if we access the property?