Skip to content

Commit

Permalink
Add validations to temporarily block custom grain in all fields excep…
Browse files Browse the repository at this point in the history 
…t 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.
  • Loading branch information
@courtneyholcomb
courtneyholcomb committed Nov 26, 2024
1 parent 9151740 commit 825622c
Show file tree
Hide file tree
Showing 2 changed files with 100 additions and 8 deletions.
47 changes: 44 additions & 3 deletions dbt_semantic_interfaces/validations/metrics.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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."""
Expand All @@ -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:
Expand Down Expand Up @@ -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(
Expand All @@ -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,
)
)

Expand Down Expand Up @@ -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),
Expand All @@ -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(
Expand Down
61 changes: 56 additions & 5 deletions tests/validations/test_metrics.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -328,20 +328,48 @@ 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",
"Both offset_window and offset_to_grain set",
"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)

Expand Down Expand Up @@ -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 = [
Expand All @@ -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)

Expand Down Expand Up @@ -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)

Expand Down

0 comments on commit 825622c

Please sign in to comment.