From e8a3d58603a0888012bc69a997b5b1fc1ceb7baf Mon Sep 17 00:00:00 2001 From: Courtney Holcomb Date: Thu, 16 Jan 2025 15:17:24 -0800 Subject: [PATCH] PR feedback re: custom vs. standard offset_window --- .../specs/measure_spec.py | 7 ++++++ .../metricflow_semantics/specs/metric_spec.py | 8 ++++++ .../dataflow/builder/dataflow_plan_builder.py | 18 +++---------- .../dataflow/nodes/join_to_time_spine.py | 25 +++++++++++-------- .../plan_conversion/sql_join_builder.py | 6 ++--- ...st_derived_metric_offset_window__dfp_0.xml | 2 +- ..._metric_offset_with_granularity__dfp_0.xml | 2 +- ...erived_offset_cumulative_metric__dfp_0.xml | 2 +- ...in_to_time_spine_derived_metric__dfp_0.xml | 2 +- ...erived_metric_with_outer_offset__dfp_0.xml | 4 +-- ...ry_have_different_granularities__dfp_0.xml | 2 +- 11 files changed, 43 insertions(+), 35 deletions(-) diff --git a/metricflow-semantics/metricflow_semantics/specs/measure_spec.py b/metricflow-semantics/metricflow_semantics/specs/measure_spec.py index 8c4994a2f1..03277c0399 100644 --- a/metricflow-semantics/metricflow_semantics/specs/measure_spec.py +++ b/metricflow-semantics/metricflow_semantics/specs/measure_spec.py @@ -91,3 +91,10 @@ class JoinToTimeSpineDescription: join_type: SqlJoinType offset_window: Optional[MetricTimeWindow] offset_to_grain: Optional[TimeGranularity] + + @property + def standard_offset_window(self) -> Optional[MetricTimeWindow]: + """Return the standard offset window if it is a standard granularity.""" + if self.offset_window and self.offset_window.is_standard_granularity: + return self.offset_window + return None diff --git a/metricflow-semantics/metricflow_semantics/specs/metric_spec.py b/metricflow-semantics/metricflow_semantics/specs/metric_spec.py index 73b81e6b9d..78e5639431 100644 --- a/metricflow-semantics/metricflow_semantics/specs/metric_spec.py +++ b/metricflow-semantics/metricflow_semantics/specs/metric_spec.py @@ -4,6 +4,7 @@ from typing import Optional from dbt_semantic_interfaces.implementations.metric import PydanticMetricTimeWindow +from dbt_semantic_interfaces.protocols.metric import MetricTimeWindow from dbt_semantic_interfaces.references import MetricReference from dbt_semantic_interfaces.type_enums import TimeGranularity @@ -67,3 +68,10 @@ def without_filter_specs(self) -> MetricSpec: # noqa: D102 offset_window=self.offset_window, offset_to_grain=self.offset_to_grain, ) + + @property + def standard_offset_window(self) -> Optional[MetricTimeWindow]: + """Return the offset window if it exists and uses a standard granularity.""" + if self.offset_window and self.offset_window.is_standard_granularity: + return self.offset_window + return None diff --git a/metricflow/dataflow/builder/dataflow_plan_builder.py b/metricflow/dataflow/builder/dataflow_plan_builder.py index 6cabf11f27..7582603519 100644 --- a/metricflow/dataflow/builder/dataflow_plan_builder.py +++ b/metricflow/dataflow/builder/dataflow_plan_builder.py @@ -671,9 +671,7 @@ def _build_derived_metric_output_node( time_spine_node=time_spine_node, requested_agg_time_dimension_specs=queried_agg_time_dimension_specs, join_on_time_dimension_spec=self._sort_by_base_granularity(queried_agg_time_dimension_specs)[0], - offset_window=( - metric_spec.offset_window if not self._offset_window_is_custom(metric_spec.offset_window) else None - ), + standard_offset_window=metric_spec.standard_offset_window, offset_to_grain=metric_spec.offset_to_grain, join_type=SqlJoinType.INNER, ) @@ -1683,11 +1681,7 @@ def _build_aggregated_measure_from_measure_source_node( time_spine_node=time_spine_node, requested_agg_time_dimension_specs=base_queried_agg_time_dimension_specs, join_on_time_dimension_spec=join_on_time_dimension_spec, - offset_window=( - before_aggregation_time_spine_join_description.offset_window - if not self._offset_window_is_custom(before_aggregation_time_spine_join_description.offset_window) - else None - ), + standard_offset_window=(before_aggregation_time_spine_join_description.standard_offset_window), offset_to_grain=before_aggregation_time_spine_join_description.offset_to_grain, join_type=before_aggregation_time_spine_join_description.join_type, ) @@ -1905,7 +1899,7 @@ def _build_time_spine_node( should_dedupe = False filter_to_specs = tuple(queried_time_spine_specs) - if offset_window and self._offset_window_is_custom(offset_window): + if offset_window and not offset_window.is_standard_granularity: time_spine_node = self._build_custom_offset_time_spine_node( offset_window=offset_window, required_time_spine_specs=required_time_spine_specs ) @@ -1989,9 +1983,3 @@ def _determine_time_spine_join_spec( sample_agg_time_dimension_spec = required_time_spine_specs[0] join_on_time_dimension_spec = sample_agg_time_dimension_spec.with_grain(time_granularity=join_spec_grain) return join_on_time_dimension_spec - - def _offset_window_is_custom(self, offset_window: Optional[MetricTimeWindow]) -> bool: - return ( - offset_window is not None - and offset_window.granularity in self._semantic_model_lookup.custom_granularity_names - ) diff --git a/metricflow/dataflow/nodes/join_to_time_spine.py b/metricflow/dataflow/nodes/join_to_time_spine.py index 27557bf36c..0e8d0a799e 100644 --- a/metricflow/dataflow/nodes/join_to_time_spine.py +++ b/metricflow/dataflow/nodes/join_to_time_spine.py @@ -24,7 +24,8 @@ class JoinToTimeSpineNode(DataflowPlanNode, ABC): requested_agg_time_dimension_specs: Time dimensions requested in the query. join_type: Join type to use when joining to time spine. join_on_time_dimension_spec: The time dimension to use in the join ON condition. - offset_window: Time window to offset the parent dataset by when joining to time spine. + standard_offset_window: Time window to offset the parent dataset by when joining to time spine. + Only standard granularities are accepted for standard_offset_window in this node. offset_to_grain: Granularity period to offset the parent dataset to when joining to time spine. """ @@ -33,18 +34,22 @@ class JoinToTimeSpineNode(DataflowPlanNode, ABC): requested_agg_time_dimension_specs: Sequence[TimeDimensionSpec] join_on_time_dimension_spec: TimeDimensionSpec join_type: SqlJoinType - offset_window: Optional[MetricTimeWindow] + standard_offset_window: Optional[MetricTimeWindow] offset_to_grain: Optional[TimeGranularity] def __post_init__(self) -> None: # noqa: D105 super().__post_init__() assert not ( - self.offset_window and self.offset_to_grain - ), "Can't set both offset_window and offset_to_grain when joining to time spine. Choose one or the other." + self.standard_offset_window and self.offset_to_grain + ), "Can't set both standard_offset_window and offset_to_grain when joining to time spine. Choose one or the other." assert ( len(self.requested_agg_time_dimension_specs) > 0 ), "Must have at least one value in requested_agg_time_dimension_specs for JoinToTimeSpineNode." + if self.standard_offset_window and not self.standard_offset_window.is_standard_granularity: + raise RuntimeError( + f"JoinToTimeSpineNode should not accept a custom standard_offset_window. Got: {self.standard_offset_window}" + ) @staticmethod def create( # noqa: D102 @@ -53,7 +58,7 @@ def create( # noqa: D102 requested_agg_time_dimension_specs: Sequence[TimeDimensionSpec], join_on_time_dimension_spec: TimeDimensionSpec, join_type: SqlJoinType, - offset_window: Optional[MetricTimeWindow] = None, + standard_offset_window: Optional[MetricTimeWindow] = None, offset_to_grain: Optional[TimeGranularity] = None, ) -> JoinToTimeSpineNode: return JoinToTimeSpineNode( @@ -63,7 +68,7 @@ def create( # noqa: D102 requested_agg_time_dimension_specs=tuple(requested_agg_time_dimension_specs), join_on_time_dimension_spec=join_on_time_dimension_spec, join_type=join_type, - offset_window=offset_window, + standard_offset_window=standard_offset_window, offset_to_grain=offset_to_grain, ) @@ -85,8 +90,8 @@ def displayed_properties(self) -> Sequence[DisplayedProperty]: # noqa: D102 DisplayedProperty("join_on_time_dimension_spec", self.join_on_time_dimension_spec), DisplayedProperty("join_type", self.join_type), ) - if self.offset_window: - props += (DisplayedProperty("offset_window", self.offset_window),) + if self.standard_offset_window: + props += (DisplayedProperty("standard_offset_window", self.standard_offset_window),) if self.offset_to_grain: props += (DisplayedProperty("offset_to_grain", self.offset_to_grain),) return props @@ -94,7 +99,7 @@ def displayed_properties(self) -> Sequence[DisplayedProperty]: # noqa: D102 def functionally_identical(self, other_node: DataflowPlanNode) -> bool: # noqa: D102 return ( isinstance(other_node, self.__class__) - and other_node.offset_window == self.offset_window + and other_node.standard_offset_window == self.standard_offset_window and other_node.offset_to_grain == self.offset_to_grain and other_node.requested_agg_time_dimension_specs == self.requested_agg_time_dimension_specs and other_node.join_on_time_dimension_spec == self.join_on_time_dimension_spec @@ -107,7 +112,7 @@ def with_new_parents(self, new_parent_nodes: Sequence[DataflowPlanNode]) -> Join metric_source_node=self.metric_source_node, time_spine_node=self.time_spine_node, requested_agg_time_dimension_specs=self.requested_agg_time_dimension_specs, - offset_window=self.offset_window, + standard_offset_window=self.standard_offset_window, offset_to_grain=self.offset_to_grain, join_type=self.join_type, join_on_time_dimension_spec=self.join_on_time_dimension_spec, diff --git a/metricflow/plan_conversion/sql_join_builder.py b/metricflow/plan_conversion/sql_join_builder.py index 5b28945609..b25b1453a4 100644 --- a/metricflow/plan_conversion/sql_join_builder.py +++ b/metricflow/plan_conversion/sql_join_builder.py @@ -536,11 +536,11 @@ def make_join_to_time_spine_join_description( left_expr: SqlExpressionNode = SqlColumnReferenceExpression.create( col_ref=SqlColumnReference(table_alias=time_spine_alias, column_name=time_spine_column_name) ) - if node.offset_window: + if node.standard_offset_window: left_expr = SqlSubtractTimeIntervalExpression.create( arg=left_expr, - count=node.offset_window.count, - granularity=error_if_not_standard_grain(input_granularity=node.offset_window.granularity), + count=node.standard_offset_window.count, + granularity=error_if_not_standard_grain(input_granularity=node.standard_offset_window.granularity), ) elif node.offset_to_grain: left_expr = SqlDateTruncExpression.create(time_granularity=node.offset_to_grain, arg=left_expr) diff --git a/tests_metricflow/snapshots/test_dataflow_plan_builder.py/DataflowPlan/test_derived_metric_offset_window__dfp_0.xml b/tests_metricflow/snapshots/test_dataflow_plan_builder.py/DataflowPlan/test_derived_metric_offset_window__dfp_0.xml index f7436df3c7..8add9b8347 100644 --- a/tests_metricflow/snapshots/test_dataflow_plan_builder.py/DataflowPlan/test_derived_metric_offset_window__dfp_0.xml +++ b/tests_metricflow/snapshots/test_dataflow_plan_builder.py/DataflowPlan/test_derived_metric_offset_window__dfp_0.xml @@ -50,7 +50,7 @@ docstring: - + diff --git a/tests_metricflow/snapshots/test_dataflow_plan_builder.py/DataflowPlan/test_derived_metric_offset_with_granularity__dfp_0.xml b/tests_metricflow/snapshots/test_dataflow_plan_builder.py/DataflowPlan/test_derived_metric_offset_with_granularity__dfp_0.xml index 1f2f626d45..2b37db8f77 100644 --- a/tests_metricflow/snapshots/test_dataflow_plan_builder.py/DataflowPlan/test_derived_metric_offset_with_granularity__dfp_0.xml +++ b/tests_metricflow/snapshots/test_dataflow_plan_builder.py/DataflowPlan/test_derived_metric_offset_with_granularity__dfp_0.xml @@ -48,7 +48,7 @@ test_filename: test_dataflow_plan_builder.py - + diff --git a/tests_metricflow/snapshots/test_dataflow_plan_builder.py/DataflowPlan/test_derived_offset_cumulative_metric__dfp_0.xml b/tests_metricflow/snapshots/test_dataflow_plan_builder.py/DataflowPlan/test_derived_offset_cumulative_metric__dfp_0.xml index d26968121f..7dba963ead 100644 --- a/tests_metricflow/snapshots/test_dataflow_plan_builder.py/DataflowPlan/test_derived_offset_cumulative_metric__dfp_0.xml +++ b/tests_metricflow/snapshots/test_dataflow_plan_builder.py/DataflowPlan/test_derived_offset_cumulative_metric__dfp_0.xml @@ -49,7 +49,7 @@ test_filename: test_dataflow_plan_builder.py - + diff --git a/tests_metricflow/snapshots/test_dataflow_plan_builder.py/DataflowPlan/test_join_to_time_spine_derived_metric__dfp_0.xml b/tests_metricflow/snapshots/test_dataflow_plan_builder.py/DataflowPlan/test_join_to_time_spine_derived_metric__dfp_0.xml index 50bcb8aec1..42ffd68148 100644 --- a/tests_metricflow/snapshots/test_dataflow_plan_builder.py/DataflowPlan/test_join_to_time_spine_derived_metric__dfp_0.xml +++ b/tests_metricflow/snapshots/test_dataflow_plan_builder.py/DataflowPlan/test_join_to_time_spine_derived_metric__dfp_0.xml @@ -157,7 +157,7 @@ test_filename: test_dataflow_plan_builder.py - + diff --git a/tests_metricflow/snapshots/test_dataflow_plan_builder.py/DataflowPlan/test_nested_derived_metric_with_outer_offset__dfp_0.xml b/tests_metricflow/snapshots/test_dataflow_plan_builder.py/DataflowPlan/test_nested_derived_metric_with_outer_offset__dfp_0.xml index 5bd7132c39..10cff4d0d8 100644 --- a/tests_metricflow/snapshots/test_dataflow_plan_builder.py/DataflowPlan/test_nested_derived_metric_with_outer_offset__dfp_0.xml +++ b/tests_metricflow/snapshots/test_dataflow_plan_builder.py/DataflowPlan/test_nested_derived_metric_with_outer_offset__dfp_0.xml @@ -25,7 +25,7 @@ test_filename: test_dataflow_plan_builder.py - + @@ -73,7 +73,7 @@ test_filename: test_dataflow_plan_builder.py - + diff --git a/tests_metricflow/snapshots/test_dataflow_plan_builder.py/DataflowPlan/test_offset_window_metric_filter_and_query_have_different_granularities__dfp_0.xml b/tests_metricflow/snapshots/test_dataflow_plan_builder.py/DataflowPlan/test_offset_window_metric_filter_and_query_have_different_granularities__dfp_0.xml index fe934c12c6..2139b25564 100644 --- a/tests_metricflow/snapshots/test_dataflow_plan_builder.py/DataflowPlan/test_offset_window_metric_filter_and_query_have_different_granularities__dfp_0.xml +++ b/tests_metricflow/snapshots/test_dataflow_plan_builder.py/DataflowPlan/test_offset_window_metric_filter_and_query_have_different_granularities__dfp_0.xml @@ -181,7 +181,7 @@ docstring: - +