Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change OrderBySpec to Use a Single Spec #789

Merged
merged 2 commits into from
Oct 5, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions metricflow/dataflow/builder/dataflow_plan_builder.py
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
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
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
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
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
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
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
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
Original file line number Diff line number Diff line change
Expand Up @@ -5,16 +5,13 @@
<OrderByLimitNode>
<!-- description = Order By ['listing__country_latest'] Limit 100 -->
<!-- node_id = obl_0 -->
<!-- order_by_spec = -->
<!-- {'class': 'OrderBySpec', -->
<!-- 'descending': False, -->
<!-- 'metric_spec': None, -->
<!-- 'dimension_spec': {'class': 'DimensionSpec', -->
<!-- 'element_name': 'country_latest', -->
<!-- 'entity_links': ({'class': 'EntityReference', -->
<!-- 'element_name': 'listing'},)}, -->
<!-- 'time_dimension_spec': None, -->
<!-- 'entity_spec': None} -->
<!-- order_by_spec = -->
<!-- {'class': 'OrderBySpec', -->
<!-- 'instance_spec': {'class': 'DimensionSpec', -->
<!-- 'element_name': 'country_latest', -->
<!-- 'entity_links': ({'class': 'EntityReference', -->
<!-- 'element_name': 'listing'},)}, -->
<!-- 'descending': False} -->
<!-- limit = 100 -->
<FilterElementsNode>
<!-- description = -->
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,30 +5,24 @@
<OrderByLimitNode>
<!-- description = Order By ['metric_time__day', 'bookings'] -->
<!-- node_id = obl_0 -->
<!-- order_by_spec = -->
<!-- {'class': 'OrderBySpec', -->
<!-- 'descending': False, -->
<!-- 'metric_spec': None, -->
<!-- 'dimension_spec': None, -->
<!-- 'time_dimension_spec': {'class': 'TimeDimensionSpec', -->
<!-- 'element_name': 'metric_time', -->
<!-- 'entity_links': (), -->
<!-- 'time_granularity': TimeGranularity.DAY, -->
<!-- 'date_part': None, -->
<!-- 'aggregation_state': None}, -->
<!-- 'entity_spec': None} -->
<!-- order_by_spec = -->
<!-- {'class': 'OrderBySpec', -->
<!-- 'descending': True, -->
<!-- 'metric_spec': {'class': 'MetricSpec', -->
<!-- 'element_name': 'bookings', -->
<!-- 'constraint': None, -->
<!-- 'alias': None, -->
<!-- 'offset_window': None, -->
<!-- 'offset_to_grain': None}, -->
<!-- 'dimension_spec': None, -->
<!-- 'time_dimension_spec': None, -->
<!-- 'entity_spec': None} -->
<!-- order_by_spec = -->
<!-- {'class': 'OrderBySpec', -->
<!-- 'instance_spec': {'class': 'TimeDimensionSpec', -->
<!-- 'element_name': 'metric_time', -->
<!-- 'entity_links': (), -->
<!-- 'time_granularity': TimeGranularity.DAY, -->
<!-- 'date_part': None, -->
<!-- 'aggregation_state': None}, -->
<!-- 'descending': False} -->
<!-- order_by_spec = -->
<!-- {'class': 'OrderBySpec', -->
<!-- 'instance_spec': {'class': 'MetricSpec', -->
<!-- 'element_name': 'bookings', -->
<!-- 'constraint': None, -->
<!-- 'alias': None, -->
<!-- 'offset_window': None, -->
<!-- 'offset_to_grain': None}, -->
<!-- 'descending': True} -->
<!-- limit = None -->
<ComputeMetricsNode>
<!-- description = Compute Metrics via Expressions -->
Expand Down