-
Notifications
You must be signed in to change notification settings - Fork 96
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
Support for Dimension(...).grain(...)
in where filter
#785
Changes from all commits
dae5d4c
268c0fb
04c877d
3cdea34
6470b08
d8640fc
38069c5
d37ec16
3c97f45
86f14bf
ce529c5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
kind: Features | ||
body: Support for the Dimension(...).grain(...) syntax for the where parameter | ||
time: 2023-10-04T10:22:55.730467-05:00 | ||
custom: | ||
Author: DevonFulcher | ||
Issue: None |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,54 @@ | ||
from __future__ import annotations | ||
|
||
from typing import Sequence | ||
|
||
from dbt_semantic_interfaces.call_parameter_sets import ( | ||
DimensionCallParameterSet, | ||
FilterCallParameterSets, | ||
TimeDimensionCallParameterSet, | ||
) | ||
from dbt_semantic_interfaces.naming.dundered import DunderedNameFormatter | ||
from dbt_semantic_interfaces.references import DimensionReference, EntityReference, TimeDimensionReference | ||
from dbt_semantic_interfaces.type_enums import TimeGranularity | ||
|
||
from metricflow.specs.specs import DimensionSpec, TimeDimensionSpec | ||
|
||
|
||
class DimensionSpecResolver: | ||
"""Resolves specs for Dimension & TimeDimension given name, grain, & entity path. Utilized in where clause in Jinja syntax.""" | ||
|
||
def __init__(self, call_parameter_sets: FilterCallParameterSets): # noqa | ||
self._call_parameter_sets = call_parameter_sets | ||
|
||
def resolve_dimension_spec(self, name: str, entity_path: Sequence[str]) -> DimensionSpec: | ||
"""Resolve Dimension spec with the call_parameter_sets.""" | ||
structured_name = DunderedNameFormatter.parse_name(name) | ||
call_parameter_set = DimensionCallParameterSet( | ||
dimension_reference=DimensionReference(element_name=structured_name.element_name), | ||
entity_path=( | ||
tuple(EntityReference(element_name=arg) for arg in entity_path) + structured_name.entity_links | ||
), | ||
) | ||
return DimensionSpec( | ||
element_name=call_parameter_set.dimension_reference.element_name, | ||
entity_links=call_parameter_set.entity_path, | ||
) | ||
|
||
def resolve_time_dimension_spec( | ||
self, name: str, time_granularity_name: TimeGranularity, entity_path: Sequence[str] | ||
) -> TimeDimensionSpec: | ||
"""Resolve TimeDimension spec with the call_parameter_sets.""" | ||
structured_name = DunderedNameFormatter.parse_name(name) | ||
call_parameter_set = TimeDimensionCallParameterSet( | ||
time_dimension_reference=TimeDimensionReference(element_name=structured_name.element_name), | ||
time_granularity=time_granularity_name, | ||
entity_path=( | ||
tuple(EntityReference(element_name=arg) for arg in entity_path) + structured_name.entity_links | ||
), | ||
) | ||
assert call_parameter_set in self._call_parameter_sets.time_dimension_call_parameter_sets | ||
return TimeDimensionSpec( | ||
element_name=call_parameter_set.time_dimension_reference.element_name, | ||
entity_links=call_parameter_set.entity_path, | ||
time_granularity=call_parameter_set.time_granularity, | ||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,17 +1,12 @@ | ||
from __future__ import annotations | ||
|
||
from typing import List, Sequence | ||
from typing import List, Optional, Sequence | ||
|
||
from dbt_semantic_interfaces.call_parameter_sets import ( | ||
DimensionCallParameterSet, | ||
FilterCallParameterSets, | ||
) | ||
from dbt_semantic_interfaces.naming.dundered import DunderedNameFormatter | ||
from dbt_semantic_interfaces.protocols.protocol_hint import ProtocolHint | ||
from dbt_semantic_interfaces.references import ( | ||
DimensionReference, | ||
EntityReference, | ||
) | ||
from dbt_semantic_interfaces.type_enums import TimeGranularity | ||
from typing_extensions import override | ||
|
||
from metricflow.errors.errors import InvalidQuerySyntax | ||
|
@@ -20,7 +15,8 @@ | |
QueryInterfaceDimensionFactory, | ||
) | ||
from metricflow.specs.column_assoc import ColumnAssociationResolver | ||
from metricflow.specs.specs import DimensionSpec | ||
from metricflow.specs.dimension_spec_resolver import DimensionSpecResolver | ||
from metricflow.specs.specs import TimeDimensionSpec | ||
|
||
|
||
class WhereFilterDimension(ProtocolHint[QueryInterfaceDimension]): | ||
|
@@ -30,39 +26,49 @@ class WhereFilterDimension(ProtocolHint[QueryInterfaceDimension]): | |
def _implements_protocol(self) -> QueryInterfaceDimension: | ||
return self | ||
|
||
def __init__(self, column_name: str) -> None: # noqa | ||
self.column_name = column_name | ||
def __init__( # noqa | ||
self, | ||
name: str, | ||
entity_path: Sequence[str], | ||
call_parameter_sets: FilterCallParameterSets, | ||
column_association_resolver: ColumnAssociationResolver, | ||
) -> None: | ||
self._dimension_spec_resolver = DimensionSpecResolver(call_parameter_sets) | ||
self._column_association_resolver = column_association_resolver | ||
self._name = name | ||
self._entity_path = entity_path | ||
self.dimension_spec = self._dimension_spec_resolver.resolve_dimension_spec(name, entity_path) | ||
self.time_dimension_spec: Optional[TimeDimensionSpec] = None | ||
Comment on lines
+40
to
+41
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we be using the type system to enforce that exactly one of these is set? Like we could make this a union type and keep the property itself internal, and then we get some protection against someone using a time dimension as a non-time dimension, which could get odd. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Do you mean something like:
I had started with that, but I think that would require the use of
Do you mean specifying grain on a non-time dimension? Or not specifying grain on a time dimension? I had thought that is validated after the specs are returned from |
||
|
||
def grain(self, _grain: str) -> QueryInterfaceDimension: | ||
def grain(self, time_granularity_name: str) -> QueryInterfaceDimension: | ||
"""The time granularity.""" | ||
raise NotImplementedError | ||
self.time_dimension_spec = self._dimension_spec_resolver.resolve_time_dimension_spec( | ||
self._name, TimeGranularity(time_granularity_name), self._entity_path | ||
) | ||
return self | ||
Comment on lines
+41
to
+48
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, right, this has to be here because it's the input protocol. The output QueryParameter or whatever it's called has these organized a bit better. Do I have that right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you clarify what you mean by |
||
|
||
def date_part(self, _date_part: str) -> QueryInterfaceDimension: | ||
"""The date_part requested to extract.""" | ||
raise NotImplementedError | ||
|
||
def alias(self, _alias: str) -> QueryInterfaceDimension: | ||
"""Renaming the column.""" | ||
raise NotImplementedError | ||
raise InvalidQuerySyntax("date_part isn't currently supported in the where parameter") | ||
|
||
def descending(self, _is_descending: bool) -> QueryInterfaceDimension: | ||
"""Set the sort order for order-by.""" | ||
raise InvalidQuerySyntax( | ||
"Can't set descending in the where clause. Try setting descending in the order_by clause instead" | ||
) | ||
raise InvalidQuerySyntax("descending is invalid in the where parameter") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hm. Seems like the protocol specs need more refinement, but this'll have to do for now. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the spec is fine. We don't have an LSP for the Jinja syntax, so I think providing a more specific error message will help our users more than a generic type error. |
||
|
||
def __str__(self) -> str: | ||
"""Returns the column name. | ||
|
||
Important in the Jinja sandbox. | ||
""" | ||
return self.column_name | ||
return self._column_association_resolver.resolve_spec( | ||
self.time_dimension_spec or self.dimension_spec | ||
).column_name | ||
|
||
|
||
class WhereFilterDimensionFactory(ProtocolHint[QueryInterfaceDimensionFactory]): | ||
"""Creates a WhereFilterDimension. | ||
|
||
Each call to `create` adds a DimensionSpec to dimension_specs. | ||
Each call to `create` adds a WhereFilterDimension to created. | ||
""" | ||
|
||
@override | ||
|
@@ -76,29 +82,12 @@ def __init__( # noqa | |
): | ||
self._call_parameter_sets = call_parameter_sets | ||
self._column_association_resolver = column_association_resolver | ||
self.dimension_specs: List[DimensionSpec] = [] | ||
self.created: List[WhereFilterDimension] = [] | ||
|
||
def create(self, name: str, entity_path: Sequence[str] = ()) -> WhereFilterDimension: | ||
"""Create a WhereFilterDimension.""" | ||
structured_name = DunderedNameFormatter.parse_name(name) | ||
call_parameter_set = DimensionCallParameterSet( | ||
dimension_reference=DimensionReference(element_name=structured_name.element_name), | ||
entity_path=( | ||
tuple(EntityReference(element_name=arg) for arg in entity_path) + structured_name.entity_links | ||
), | ||
) | ||
assert call_parameter_set in self._call_parameter_sets.dimension_call_parameter_sets | ||
|
||
dimension_spec = self._convert_to_dimension_spec(call_parameter_set) | ||
self.dimension_specs.append(dimension_spec) | ||
column_name = self._column_association_resolver.resolve_spec(dimension_spec).column_name | ||
return WhereFilterDimension(column_name) | ||
|
||
def _convert_to_dimension_spec( | ||
self, | ||
parameter_set: DimensionCallParameterSet, | ||
) -> DimensionSpec: # noqa: D | ||
return DimensionSpec( | ||
element_name=parameter_set.dimension_reference.element_name, | ||
entity_links=parameter_set.entity_path, | ||
dimension = WhereFilterDimension( | ||
name, entity_path, self._call_parameter_sets, self._column_association_resolver | ||
) | ||
self.created.append(dimension) | ||
return dimension |
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't this logic already exist somewhere in MetricFlow? Or am I confusing that with this logic existing in some other repo?
I might be thinking of the deleted code below, too. 🤷
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure! This code was moved here from elsewhere in this PR, which was also moved there from somewhere that @plypaul put it. So, Paul is the original author, and I just moved it here. He may know if this is duplicative of somewhere else in MF.