From 825622c4914bc04651c37678de66f44e9b1dcafe Mon Sep 17 00:00:00 2001 From: Courtney Holcomb Date: Tue, 26 Nov 2024 14:59:07 -0800 Subject: [PATCH] Add validations to temporarily block custom grain in all fields except offset_window We decided to cut scope on custom calendar in the meantime and only enable it for offset_windows. These validations will make that an easy user experience, while still allowing us to easily remove them once we build support. --- .../validations/metrics.py | 47 +++++++++++++- tests/validations/test_metrics.py | 61 +++++++++++++++++-- 2 files changed, 100 insertions(+), 8 deletions(-) diff --git a/dbt_semantic_interfaces/validations/metrics.py b/dbt_semantic_interfaces/validations/metrics.py index c069f742..57d7c7d4 100644 --- a/dbt_semantic_interfaces/validations/metrics.py +++ b/dbt_semantic_interfaces/validations/metrics.py @@ -38,6 +38,8 @@ WhereFiltersAreParseable, # noQa ) +TEMP_CUSTOM_GRAIN_MSG = "Custom granularities are not supported for this field yet." + class CumulativeMetricRule(SemanticManifestValidationRule[SemanticManifestT], Generic[SemanticManifestT]): """Checks that cumulative metrics are configured properly.""" @@ -52,6 +54,7 @@ def validate_manifest(cls, semantic_manifest: SemanticManifestT) -> Sequence[Val for time_spine in semantic_manifest.project_configuration.time_spines for granularity in time_spine.custom_granularities ] + standard_granularities = {item.value.lower() for item in TimeGranularity} for metric in semantic_manifest.metrics or []: if metric.type != MetricType.CUMULATIVE: @@ -96,6 +99,18 @@ def validate_manifest(cls, semantic_manifest: SemanticManifestT) -> Sequence[Val grain_to_date = metric.type_params.grain_to_date.value if metric.type_params.grain_to_date else None if metric.type_params.cumulative_type_params and metric.type_params.cumulative_type_params.grain_to_date: grain_to_date = metric.type_params.cumulative_type_params.grain_to_date + + if grain_to_date and grain_to_date not in standard_granularities: + issues.append( + ValidationError( + context=metric_context, + message=( + f"Invalid time granularity found in `grain_to_date`: '{grain_to_date}'. " + f"{TEMP_CUSTOM_GRAIN_MSG}" + ), + ) + ) + if window and grain_to_date: issues.append( ValidationError( @@ -113,21 +128,34 @@ def validate_manifest(cls, semantic_manifest: SemanticManifestT) -> Sequence[Val @classmethod def validate_metric_time_window( # noqa: D - cls, metric_context: MetricContext, window: MetricTimeWindow, custom_granularities: Sequence[str] + cls, + metric_context: MetricContext, + window: MetricTimeWindow, + custom_granularities: Sequence[str], + allow_custom: bool = False, ) -> Sequence[ValidationIssue]: issues: List[ValidationIssue] = [] - valid_granularities = set(custom_granularities) | {item.value.lower() for item in TimeGranularity} + standard_granularities = {item.value.lower() for item in TimeGranularity} + valid_granularities = set(custom_granularities) | standard_granularities window_granularity = window.granularity if window_granularity.endswith("s") and window_granularity[:-1] in valid_granularities: # months -> month window_granularity = window_granularity[:-1] + msg = f"Invalid time granularity '{window_granularity}' in window: '{window.window_string}'" if window_granularity not in valid_granularities: issues.append( ValidationError( context=metric_context, - message=f"Invalid time granularity {window_granularity} in window: ({window})", + message=msg, + ) + ) + elif not allow_custom and (window_granularity not in standard_granularities): + issues.append( + ValidationError( + context=metric_context, + message=msg + " " + TEMP_CUSTOM_GRAIN_MSG, ) ) @@ -199,6 +227,8 @@ def _validate_input_metrics_exist(semantic_manifest: SemanticManifest) -> List[V def _validate_time_offset_params(metric: Metric, custom_granularities: Sequence[str]) -> List[ValidationIssue]: issues: List[ValidationIssue] = [] + standard_granularities = {item.value.lower() for item in TimeGranularity} + metric_context = MetricContext( file_context=FileContext.from_metadata(metadata=metric.metadata), metric=MetricModelReference(metric_name=metric.name), @@ -209,6 +239,17 @@ def _validate_time_offset_params(metric: Metric, custom_granularities: Sequence[ metric_context=metric_context, window=input_metric.offset_window, custom_granularities=custom_granularities, + allow_custom=True, + ) + if input_metric.offset_to_grain and input_metric.offset_to_grain not in standard_granularities: + issues.append( + ValidationError( + context=metric_context, + message=( + f"Invalid time granularity found in `offset_to_grain`: '{input_metric.offset_to_grain}'. " + f"{TEMP_CUSTOM_GRAIN_MSG}" + ), + ) ) if input_metric.offset_window and input_metric.offset_to_grain: issues.append( diff --git a/tests/validations/test_metrics.py b/tests/validations/test_metrics.py index 4955fb79..4794bd89 100644 --- a/tests/validations/test_metrics.py +++ b/tests/validations/test_metrics.py @@ -328,12 +328,38 @@ def test_derived_metric() -> None: # noqa: D ], ), ), + metric_with_guaranteed_meta( + name="has_custom_grain_offset_window", # this is valid + type=MetricType.DERIVED, + type_params=PydanticMetricTypeParams( + expr="random_metric * 2", + metrics=[ + PydanticMetricInput( + name="random_metric", + offset_window=PydanticMetricTimeWindow.parse("3 martian_weeks"), + ) + ], + ), + ), + metric_with_guaranteed_meta( + name="has_custom_offset_to_grain", + type=MetricType.DERIVED, + type_params=PydanticMetricTypeParams( + expr="random_metric * 2", + metrics=[ + PydanticMetricInput( + name="random_metric", + offset_to_grain="martian_week", + ) + ], + ), + ), ], project_configuration=EXAMPLE_PROJECT_CONFIGURATION, ) ) build_issues = validation_results.all_issues - assert len(build_issues) == 7 + assert len(build_issues) == 8 expected_substrings = [ "is already being used. Please choose another alias", "does not exist as a configured metric in the model", @@ -341,7 +367,9 @@ def test_derived_metric() -> None: # noqa: D "is not used in `expr`", "No input metrics found for derived metric", "No `expr` set for derived metric", - "Invalid time granularity", + "Invalid time granularity 'weekies' in window: '3 weekies'", + "Custom granularities are not supported", + "Invalid time granularity found in `offset_to_grain`: 'martian_week'", ] check_error_in_issues(error_substrings=expected_substrings, issues=build_issues) @@ -518,7 +546,7 @@ def test_conversion_metrics() -> None: # noqa: D ) build_issues = result.all_issues - assert len(result.errors) == 6 + assert len(result.errors) == 8 assert len(result.warnings) == 1 expected_substrings = [ @@ -528,7 +556,9 @@ def test_conversion_metrics() -> None: # noqa: D "The provided constant property: bad_dim, cannot be found", "The provided constant property: bad_dim2, cannot be found", "filtering on a conversion input measure is not fully supported yet", - "Invalid time granularity", + "Invalid time granularity 'moons' in window", + "Invalid time granularity 'martian_week' in window: '7 martian_weeks'", + "Invalid time granularity 'martian_week' in window: '7 martian_week'", ] check_error_in_issues(error_substrings=expected_substrings, issues=build_issues) @@ -661,17 +691,38 @@ def test_cumulative_metrics() -> None: # noqa: D ), ), ), + metric_with_guaranteed_meta( + name="custom_grain_to_date", + type=MetricType.CUMULATIVE, + type_params=PydanticMetricTypeParams( + measure=PydanticMetricInputMeasure(name=measure_name), + cumulative_type_params=PydanticCumulativeTypeParams( + grain_to_date="3 martian_weeks", + ), + ), + ), + metric_with_guaranteed_meta( + name="custom_window_old", + type=MetricType.CUMULATIVE, + type_params=PydanticMetricTypeParams( + measure=PydanticMetricInputMeasure(name=measure_name), + window=PydanticMetricTimeWindow.parse(window="5 martian_week"), + ), + ), ], project_configuration=EXAMPLE_PROJECT_CONFIGURATION, ) ) build_issues = validation_results.all_issues - assert len(build_issues) == 3 + assert len(build_issues) == 7 expected_substrings = [ "Invalid time granularity", "Both window and grain_to_date set for cumulative metric. Please set one or the other.", "Got differing values for `window`", + "Invalid time granularity 'martian_week' in window: '3 martian_weeks'", + "Invalid time granularity 'martian_week' in window: '3 martian_week'", + "Invalid time granularity 'martian_week' in window: '5 martian_week'", ] check_error_in_issues(error_substrings=expected_substrings, issues=build_issues)