Support for Dimension(...).grain(...) in where filter
#785
Conversation
|
Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide. |
DevonFulcher
force-pushed
the
dimension_dot_grain
branch
from
b15f8e5
to
3c97f45
Compare
DevonFulcher
requested review from
plypaul,
tlento,
courtneyholcomb and
QMalcolm
Just as an aside, if you squash it's fine as long as the PR is reviewable in one chunk - I mainly care that every commit to the main source tree is focused and documented, so if squash achieves that it's totally fine. |
There was a problem hiding this comment.
Assuming I still understand this correctly I think this is good for now. Thanks for pushing these down to a more central location!
Once the current push is done we should go over all of this and see if there's an easy way to refine the interfaces further. I know Paul is doing some refactoring of the query parser itself, which I'm sure will have an effect on where we eventually land with these protocol specs.
| 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, |
There was a problem hiding this comment.
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.
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.
| self.time_dimension_spec: Optional[TimeDimensionSpec] = None | ||
|
|
||
| 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 |
There was a problem hiding this comment.
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.
Can you clarify what you mean by this and these 😃?
| 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.
Hm. Seems like the protocol specs need more refinement, but this'll have to do for now.
There was a problem hiding this comment.
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.
| self.dimension_spec = self._dimension_spec_resolver.resolve_dimension_spec(name, entity_path) | ||
| self.time_dimension_spec: Optional[TimeDimensionSpec] = None |
There was a problem hiding this comment.
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.
Should we be using the type system to enforce that exactly one of these is set?
Do you mean something like:
self.spec: Union[DimensionSpec, TimeDimensionSpec] = self._dimension_spec_resolver.resolve_dimension_spec(name, entity_path)
I had started with that, but I think that would require the use of isinstance to disambiguate between the two types later when they are added to the lists. Paul steered me away from the use of isinstance.
protection against someone using a time dimension as a non-time dimension
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 create_from_where_filter. Is that not the case? I agree that should be checked somewhere
Co-authored-by: Thomas Lento <tlento@users.noreply.github.com>
This PR adds support for the Dimension(...).grain(...) syntax for the where parameter and filter spec by using the factory pattern & implementing the Query Interface protocols.
Resolves # SL-631 (internal to dbt labs)
Description
This PR adds support for the
Dimension(...).grain(...)syntax for the where parameter and filter spec by using the factory pattern & implementing the Query Interface protocols.I started on this branch before Tom & I talked about the git norms in this repo. Next PR, I will be sure to make every commit meaningful.