From 5a631f0000f63bdbe19421cea1db5b72ce5503c4 Mon Sep 17 00:00:00 2001 From: Paul Yang Date: Sun, 20 Aug 2023 19:05:30 -0700 Subject: [PATCH 1/2] Use singular instance_spec field for OrderBySpec. --- .../dataflow/builder/dataflow_plan_builder.py | 4 +--- metricflow/dataflow/dataflow_plan.py | 2 +- metricflow/plan_conversion/dataflow_to_sql.py | 4 +++- metricflow/query/query_parser.py | 18 +++++++-------- metricflow/specs/specs.py | 22 +------------------ .../builder/test_dataflow_plan_builder.py | 4 ++-- .../test_dataflow_to_sql_plan.py | 4 ++-- metricflow/test/query/test_query_parser.py | 20 +++++------------ 8 files changed, 25 insertions(+), 53 deletions(-) diff --git a/metricflow/dataflow/builder/dataflow_plan_builder.py b/metricflow/dataflow/builder/dataflow_plan_builder.py index 004283db1e..8235a87c8b 100644 --- a/metricflow/dataflow/builder/dataflow_plan_builder.py +++ b/metricflow/dataflow/builder/dataflow_plan_builder.py @@ -303,9 +303,7 @@ def build_plan_for_distinct_values( computed_metrics_output=distinct_values_node, order_by_specs=( OrderBySpec( - dimension_spec=dimension_spec, - time_dimension_spec=time_dimension_spec, - entity_spec=entity_spec, + instance_spec=linkable_spec, descending=False, ), ), diff --git a/metricflow/dataflow/dataflow_plan.py b/metricflow/dataflow/dataflow_plan.py index 89aef6c18c..93fa93f2a1 100644 --- a/metricflow/dataflow/dataflow_plan.py +++ b/metricflow/dataflow/dataflow_plan.py @@ -887,7 +887,7 @@ def accept(self, visitor: DataflowPlanNodeVisitor[VisitorOutputT]) -> VisitorOut @property def description(self) -> str: # noqa: D - return f"Order By {[x.item.qualified_name for x in self._order_by_specs]}" + ( + return f"Order By {[order_by_spec.instance_spec.qualified_name for order_by_spec in self._order_by_specs]}" + ( f" Limit {self._limit}" if self.limit else "" ) diff --git a/metricflow/plan_conversion/dataflow_to_sql.py b/metricflow/plan_conversion/dataflow_to_sql.py index 4391ad2254..ff5b0f9359 100644 --- a/metricflow/plan_conversion/dataflow_to_sql.py +++ b/metricflow/plan_conversion/dataflow_to_sql.py @@ -749,7 +749,9 @@ def visit_order_by_limit_node(self, node: OrderByLimitNode) -> SqlDataSet: # no expr=SqlColumnReferenceExpression( col_ref=SqlColumnReference( table_alias=from_data_set_alias, - column_name=self._column_association_resolver.resolve_spec(order_by_spec.item).column_name, + column_name=self._column_association_resolver.resolve_spec( + order_by_spec.instance_spec + ).column_name, ) ), desc=order_by_spec.descending, diff --git a/metricflow/query/query_parser.py b/metricflow/query/query_parser.py index 4312b44764..e2e0edbece 100644 --- a/metricflow/query/query_parser.py +++ b/metricflow/query/query_parser.py @@ -511,10 +511,10 @@ def _validate_order_by_specs( metric_specs = [MetricSpec.from_reference(metric_reference) for metric_reference in metric_references] for order_by_spec in order_by_specs: if not ( - order_by_spec.item in metric_specs - or order_by_spec.item in linkable_specs.dimension_specs - or order_by_spec.item in linkable_specs.time_dimension_specs - or order_by_spec.item in linkable_specs.entity_specs + order_by_spec.instance_spec in metric_specs + or order_by_spec.instance_spec in linkable_specs.dimension_specs + or order_by_spec.instance_spec in linkable_specs.time_dimension_specs + or order_by_spec.instance_spec in linkable_specs.entity_specs ): raise InvalidQueryException(f"Order by item {order_by_spec} not in the query") @@ -855,7 +855,7 @@ def _parse_order_by( raise InvalidQueryException(f"Order by item '{order}' references a metric but has entity links") order_by_specs.append( OrderBySpec( - metric_spec=MetricSpec(element_name=parsed_name.element_name), + instance_spec=MetricSpec(element_name=parsed_name.element_name), descending=descending, ) ) @@ -866,7 +866,7 @@ def _parse_order_by( ) order_by_specs.append( OrderBySpec( - dimension_spec=DimensionSpec( + instance_spec=DimensionSpec( element_name=parsed_name.element_name, entity_links=tuple(EntityReference(element_name=x) for x in parsed_name.entity_link_names), ), @@ -881,7 +881,7 @@ def _parse_order_by( if time_granularity: order_by_specs.append( OrderBySpec( - time_dimension_spec=TimeDimensionSpec( + instance_spec=TimeDimensionSpec( element_name=parsed_name.element_name, entity_links=entity_links, time_granularity=time_granularity, @@ -902,7 +902,7 @@ def _parse_order_by( if partial_time_dimension_spec in time_dimension_spec_replacements: order_by_specs.append( OrderBySpec( - time_dimension_spec=time_dimension_spec_replacements[partial_time_dimension_spec], + instance_spec=time_dimension_spec_replacements[partial_time_dimension_spec], descending=descending, ) ) @@ -919,7 +919,7 @@ def _parse_order_by( ) order_by_specs.append( OrderBySpec( - entity_spec=EntitySpec( + instance_spec=EntitySpec( element_name=parsed_name.element_name, entity_links=tuple(EntityReference(element_name=x) for x in parsed_name.entity_link_names), ), diff --git a/metricflow/specs/specs.py b/metricflow/specs/specs.py index 81e1f68e23..a9c1905460 100644 --- a/metricflow/specs/specs.py +++ b/metricflow/specs/specs.py @@ -29,7 +29,6 @@ from dbt_semantic_interfaces.type_enums.time_granularity import TimeGranularity from metricflow.aggregation_properties import AggregationState -from metricflow.assert_one_arg import assert_exactly_one_arg_set from metricflow.filters.time_constraint import TimeRangeConstraint from metricflow.naming.linkable_spec_name import StructuredLinkableSpecName from metricflow.sql.sql_bind_parameters import SqlBindParameters @@ -479,27 +478,8 @@ def post_aggregation_spec(self) -> MeasureSpec: @dataclass(frozen=True) class OrderBySpec(SerializableDataclass): # noqa: D + instance_spec: InstanceSpec descending: bool - metric_spec: Optional[MetricSpec] = None - dimension_spec: Optional[DimensionSpec] = None - time_dimension_spec: Optional[TimeDimensionSpec] = None - entity_spec: Optional[EntitySpec] = None - - def __post_init__(self) -> None: # noqa: D - assert_exactly_one_arg_set( - metric_spec=self.metric_spec, - dimension_spec=self.dimension_spec, - time_dimension_spec=self.time_dimension_spec, - entity_spec=self.entity_spec, - ) - - @property - def item(self) -> InstanceSpec: # noqa: D - result: Optional[InstanceSpec] = ( - self.metric_spec or self.dimension_spec or self.time_dimension_spec or self.entity_spec - ) - assert result - return result @dataclass(frozen=True) diff --git a/metricflow/test/dataflow/builder/test_dataflow_plan_builder.py b/metricflow/test/dataflow/builder/test_dataflow_plan_builder.py index 625226cddf..cb97cee1e2 100644 --- a/metricflow/test/dataflow/builder/test_dataflow_plan_builder.py +++ b/metricflow/test/dataflow/builder/test_dataflow_plan_builder.py @@ -141,11 +141,11 @@ def test_order_by_plan( # noqa: D time_dimension_specs=(MTD_SPEC_DAY,), order_by_specs=( OrderBySpec( - time_dimension_spec=MTD_SPEC_DAY, + instance_spec=MTD_SPEC_DAY, descending=False, ), OrderBySpec( - metric_spec=MetricSpec(element_name="bookings"), + instance_spec=MetricSpec(element_name="bookings"), descending=True, ), ), diff --git a/metricflow/test/plan_conversion/test_dataflow_to_sql_plan.py b/metricflow/test/plan_conversion/test_dataflow_to_sql_plan.py index 782fa1caf3..71d78e45fe 100644 --- a/metricflow/test/plan_conversion/test_dataflow_to_sql_plan.py +++ b/metricflow/test/plan_conversion/test_dataflow_to_sql_plan.py @@ -856,11 +856,11 @@ def test_order_by_node( order_by_node = OrderByLimitNode( order_by_specs=[ OrderBySpec( - time_dimension_spec=time_dimension_spec, + instance_spec=time_dimension_spec, descending=False, ), OrderBySpec( - metric_spec=metric_spec, + instance_spec=metric_spec, descending=True, ), ], diff --git a/metricflow/test/query/test_query_parser.py b/metricflow/test/query/test_query_parser.py index 51f272879c..e362fd8ed2 100644 --- a/metricflow/test/query/test_query_parser.py +++ b/metricflow/test/query/test_query_parser.py @@ -180,13 +180,11 @@ def test_query_parser(bookings_query_parser: MetricFlowQueryParser) -> None: # assert query_spec.entity_specs == (EntitySpec(element_name="listing", entity_links=()),) assert query_spec.order_by_specs == ( OrderBySpec( - time_dimension_spec=TimeDimensionSpec( - element_name=MTD, entity_links=(), time_granularity=TimeGranularity.DAY - ), + instance_spec=TimeDimensionSpec(element_name=MTD, entity_links=(), time_granularity=TimeGranularity.DAY), descending=False, ), OrderBySpec( - metric_spec=MetricSpec(element_name="bookings"), + instance_spec=MetricSpec(element_name="bookings"), descending=True, ), ) @@ -215,13 +213,11 @@ def test_query_parser_with_object_params(bookings_query_parser: MetricFlowQueryP assert query_spec.entity_specs == (EntitySpec(element_name="listing", entity_links=()),) assert query_spec.order_by_specs == ( OrderBySpec( - time_dimension_spec=TimeDimensionSpec( - element_name=MTD, entity_links=(), time_granularity=TimeGranularity.DAY - ), + instance_spec=TimeDimensionSpec(element_name=MTD, entity_links=(), time_granularity=TimeGranularity.DAY), descending=False, ), OrderBySpec( - metric_spec=MetricSpec(element_name="bookings"), + instance_spec=MetricSpec(element_name="bookings"), descending=True, ), ) @@ -247,9 +243,7 @@ def test_order_by_granularity_conversion() -> None: # The lowest common granularity is MONTH, so we expect the PTD in the order by to have that granularity. assert ( OrderBySpec( - time_dimension_spec=TimeDimensionSpec( - element_name=MTD, entity_links=(), time_granularity=TimeGranularity.MONTH - ), + instance_spec=TimeDimensionSpec(element_name=MTD, entity_links=(), time_granularity=TimeGranularity.MONTH), descending=True, ), ) == query_spec.order_by_specs @@ -263,9 +257,7 @@ def test_order_by_granularity_no_conversion(bookings_query_parser: MetricFlowQue # The only granularity is DAY, so we expect the PTD in the order by to have that granularity. assert ( OrderBySpec( - time_dimension_spec=TimeDimensionSpec( - element_name=MTD, entity_links=(), time_granularity=TimeGranularity.DAY - ), + instance_spec=TimeDimensionSpec(element_name=MTD, entity_links=(), time_granularity=TimeGranularity.DAY), descending=False, ), ) == query_spec.order_by_specs From 94365bd85e81d28a7220b23fc1c6324fe0ee8c52 Mon Sep 17 00:00:00 2001 From: Paul Yang Date: Sun, 24 Sep 2023 17:37:59 -0700 Subject: [PATCH 2/2] Update snapshots following the change to OrderBySpec. --- .../test_distinct_values_plan__dfp_0.xml | 17 ++++---- .../test_order_by_plan__dfp_0.xml | 42 ++++++++----------- 2 files changed, 25 insertions(+), 34 deletions(-) diff --git a/metricflow/test/snapshots/test_dataflow_plan_builder.py/DataflowPlan/test_distinct_values_plan__dfp_0.xml b/metricflow/test/snapshots/test_dataflow_plan_builder.py/DataflowPlan/test_distinct_values_plan__dfp_0.xml index 387eb97250..29f0eaa0ed 100644 --- a/metricflow/test/snapshots/test_dataflow_plan_builder.py/DataflowPlan/test_distinct_values_plan__dfp_0.xml +++ b/metricflow/test/snapshots/test_dataflow_plan_builder.py/DataflowPlan/test_distinct_values_plan__dfp_0.xml @@ -5,16 +5,13 @@ - - - - - - - - - - + + + + + + + diff --git a/metricflow/test/snapshots/test_dataflow_plan_builder.py/DataflowPlan/test_order_by_plan__dfp_0.xml b/metricflow/test/snapshots/test_dataflow_plan_builder.py/DataflowPlan/test_order_by_plan__dfp_0.xml index ea902f79a7..1298e65b05 100644 --- a/metricflow/test/snapshots/test_dataflow_plan_builder.py/DataflowPlan/test_order_by_plan__dfp_0.xml +++ b/metricflow/test/snapshots/test_dataflow_plan_builder.py/DataflowPlan/test_order_by_plan__dfp_0.xml @@ -5,30 +5,24 @@ - - - - - - - - - - - - - - - - - - - - - - - - + + + + + + + + + + + + + + + + + +