/* PR_START p--short-term-perf 25 */ Separate cases for different met…

…rics into methods.
dbt-labs · Oct 28, 2024 · 71b72b0 · 71b72b0
1 parent 29e2a10
commit 71b72b0
Showing 1 changed file with 75 additions and 33 deletions.
diff --git a/metricflow-semantics/metricflow_semantics/query/validation_rules/metric_time_requirements.py b/metricflow-semantics/metricflow_semantics/query/validation_rules/metric_time_requirements.py
@@ -5,7 +5,7 @@
 
 from dbt_semantic_interfaces.enum_extension import assert_values_exhausted
 from dbt_semantic_interfaces.naming.keywords import METRIC_TIME_ELEMENT_NAME
-from dbt_semantic_interfaces.protocols import WhereFilterIntersection
+from dbt_semantic_interfaces.protocols import Metric, WhereFilterIntersection
 from dbt_semantic_interfaces.references import (
     MetricReference,
     TimeDimensionReference,
@@ -16,7 +16,10 @@
 from metricflow_semantics.collection_helpers.lru_cache import LruCache
 from metricflow_semantics.model.semantic_manifest_lookup import SemanticManifestLookup
 from metricflow_semantics.query.group_by_item.resolution_path import MetricFlowQueryResolutionPath
-from metricflow_semantics.query.issues.issues_base import MetricFlowQueryResolutionIssueSet
+from metricflow_semantics.query.issues.issues_base import (
+    MetricFlowQueryResolutionIssue,
+    MetricFlowQueryResolutionIssueSet,
+)
 from metricflow_semantics.query.issues.parsing.cumulative_metric_requires_metric_time import (
     CumulativeMetricRequiresMetricTimeIssue,
 )
@@ -109,6 +112,54 @@ def _uncached_query_items_analysis(
             has_agg_time_dimension=has_agg_time_dimension,
         )
 
+    def _validate_cumulative_metric(
+        self,
+        metric_reference: MetricReference,
+        metric: Metric,
+        query_items_analysis: QueryItemsAnalysis,
+        resolution_path: MetricFlowQueryResolutionPath,
+    ) -> Sequence[MetricFlowQueryResolutionIssue]:
+        if (
+            metric.type_params is not None
+            and metric.type_params.cumulative_type_params is not None
+            and (
+                metric.type_params.cumulative_type_params.window is not None
+                or metric.type_params.cumulative_type_params.grain_to_date is not None
+            )
+            and not (query_items_analysis.has_metric_time or query_items_analysis.has_agg_time_dimension)
+        ):
+            return (
+                CumulativeMetricRequiresMetricTimeIssue.from_parameters(
+                    metric_reference=metric_reference,
+                    query_resolution_path=resolution_path,
+                ),
+            )
+        return ()
+
+    def _validate_derived_metric(
+        self,
+        metric_reference: MetricReference,
+        metric: Metric,
+        resolution_path: MetricFlowQueryResolutionPath,
+        query_items_analysis: QueryItemsAnalysis,
+    ) -> Sequence[MetricFlowQueryResolutionIssue]:
+        has_time_offset = any(
+            input_metric.offset_window is not None or input_metric.offset_to_grain is not None
+            for input_metric in metric.input_metrics
+        )
+
+        if has_time_offset and not (
+            query_items_analysis.has_metric_time or query_items_analysis.has_agg_time_dimension
+        ):
+            return (
+                OffsetMetricRequiresMetricTimeIssue.from_parameters(
+                    metric_reference=metric_reference,
+                    input_metrics=metric.input_metrics,
+                    query_resolution_path=resolution_path,
+                ),
+            )
+        return ()
+
     @override
     def validate_metric_in_resolution_dag(
         self,
@@ -119,54 +170,45 @@ def validate_metric_in_resolution_dag(
 
         query_items_analysis = self._get_query_items_analysis(self._resolver_input_for_query, metric_reference)
 
-        issues = MetricFlowQueryResolutionIssueSet.empty_instance()
+        issues: List[MetricFlowQueryResolutionIssue] = []
 
         # Queries that join to an SCD don't support direct references to agg_time_dimension, so we
         # only check for metric_time. If we decide to support agg_time_dimension, we should add a check
         if len(query_items_analysis.scds) > 0 and not query_items_analysis.has_metric_time:
-            issues = issues.add_issue(
+            issues.append(
                 ScdRequiresMetricTimeIssue.from_parameters(
                     scds_in_query=query_items_analysis.scds,
                     query_resolution_path=resolution_path,
                 )
             )
 
         if metric.type is MetricType.CUMULATIVE:
-            if (
-                metric.type_params is not None
-                and metric.type_params.cumulative_type_params is not None
-                and (
-                    metric.type_params.cumulative_type_params.window is not None
-                    or metric.type_params.cumulative_type_params.grain_to_date is not None
-                )
-                and not (query_items_analysis.has_metric_time or query_items_analysis.has_agg_time_dimension)
-            ):
-                issues = issues.add_issue(
-                    CumulativeMetricRequiresMetricTimeIssue.from_parameters(
-                        metric_reference=metric_reference,
-                        query_resolution_path=resolution_path,
-                    )
+            issues.extend(
+                self._validate_cumulative_metric(
+                    metric_reference=metric_reference,
+                    metric=metric,
+                    query_items_analysis=query_items_analysis,
+                    resolution_path=resolution_path,
                 )
-        elif metric.type is MetricType.RATIO or metric.type is MetricType.DERIVED:
-            has_time_offset = any(
-                input_metric.offset_window is not None or input_metric.offset_to_grain is not None
-                for input_metric in metric.input_metrics
             )
 
-            if has_time_offset and not (
-                query_items_analysis.has_metric_time or query_items_analysis.has_agg_time_dimension
-            ):
-                issues = issues.add_issue(
-                    OffsetMetricRequiresMetricTimeIssue.from_parameters(
-                        metric_reference=metric_reference,
-                        input_metrics=metric.input_metrics,
-                        query_resolution_path=resolution_path,
-                    )
+        elif metric.type is MetricType.RATIO or metric.type is MetricType.DERIVED:
+            issues.extend(
+                self._validate_derived_metric(
+                    metric_reference=metric_reference,
+                    metric=metric,
+                    query_items_analysis=query_items_analysis,
+                    resolution_path=resolution_path,
                 )
-        elif metric.type is not MetricType.SIMPLE and metric.type is not MetricType.CONVERSION:
+            )
+        elif metric.type is MetricType.SIMPLE:
+            pass
+        elif metric.type is MetricType.CONVERSION:
+            pass
+        else:
             assert_values_exhausted(metric.type)
 
-        return issues
+        return MetricFlowQueryResolutionIssueSet(issues=tuple(issues))
 
     @override
     def validate_query_in_resolution_dag(