Skip to content

Commit

Permalink
Use singular instance_spec field for OrderBySpec.
Browse files Browse the repository at this point in the history
  • Loading branch information
@plypaul
plypaul committed Sep 25, 2023
1 parent afb5681 commit 5a631f0
Show file tree
Hide file tree
Showing 8 changed files with 25 additions and 53 deletions.
4 changes: 1 addition & 3 deletions metricflow/dataflow/builder/dataflow_plan_builder.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
),
),
Expand Down
2 changes: 1 addition & 1 deletion metricflow/dataflow/dataflow_plan.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 ""
)

Expand Down
4 changes: 3 additions & 1 deletion metricflow/plan_conversion/dataflow_to_sql.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
18 changes: 9 additions & 9 deletions metricflow/query/query_parser.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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")

Expand Down Expand Up @@ -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,
)
)
Expand All @@ -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),
),
Expand All @@ -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,
Expand All @@ -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,
)
)
Expand All @@ -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),
),
Expand Down
22 changes: 1 addition & 21 deletions metricflow/specs/specs.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down
4 changes: 2 additions & 2 deletions metricflow/test/dataflow/builder/test_dataflow_plan_builder.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
),
),
Expand Down
4 changes: 2 additions & 2 deletions metricflow/test/plan_conversion/test_dataflow_to_sql_plan.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
),
],
Expand Down
20 changes: 6 additions & 14 deletions metricflow/test/query/test_query_parser.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
),
)
Expand Down Expand Up @@ -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,
),
)
Expand All @@ -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
Expand All @@ -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
Expand Down

0 comments on commit 5a631f0

Please sign in to comment.