Support for descending & date_part in query interface #154
Conversation
There was a problem hiding this comment.
Makes sense to me! We'll see if adjusting protocols makes sense as we get more of these implemented. For now being able to throw the errors from the relevant class, instead of some central handler, seems like a much bigger advantage than potential type safety, especially given how we expect these to be used.
| def descending(self, _is_descending: bool) -> QueryInterfaceDimension: | ||
| """Set the sort order for order-by.""" | ||
| raise InvalidQuerySyntax("descending is invalid in the where parameter and filter spec") | ||
|
|
||
| def date_part(self, _date_part: str) -> QueryInterfaceDimension: | ||
| """Date part to extract from the dimension.""" | ||
| raise InvalidQuerySyntax("date_part isn't currently supported in the where parameter and filter spec") | ||
|
|
There was a problem hiding this comment.
I saw this in the other implementations as well and I wonder if there's a way for us to structure the protocols so that things are just typed, but I don't know if that's worth the complexity.
There was a problem hiding this comment.
Related question: in practice, will anybody other than us (or another implementer of a MetricFlow query interface) ever initialize one of these things directly? I don't think the WhereFilterDimension is a thing any end user will ever interact with except via our parser, is that correct?
There was a problem hiding this comment.
structure the protocols so that things are just typed
I believe this is typed correctly. Users of the JDBC interface don't get the benefits of an LSP, so they are reliant on error messages to help them write correct code.
will anybody other than us (or another implementer of a MetricFlow query interface) ever initialize one of these things directly?
Right, I think this is just an internal class.
| descending: Optional[bool] = None, | ||
| date_part_name: Optional[str] = None, |
There was a problem hiding this comment.
Should these also be added to the QueryInterfaceDimensionFactory.create() method?
There was a problem hiding this comment.
Oops, I somehow forgot about this between writing it and submitting the review. If this was an oversight please fix before merge!
There was a problem hiding this comment.
Hmm...good thought. To be honest, I'm not sure what the best design approach is. time_granularity_name is already a parameter for QueryInterfaceTimeDimensionFactory, but not QueryInterfaceDimensionFactory. I had assumed we would continue that pattern. But, if we go with that approach, maybe it would be better to remove entity_path from QueryInterfaceDimensionFactory and place it on QueryInterfaceDimension instead too.
This PR adds support for date_part and descending in the query interface. I decided to raise an error for
date_partfor now. We will have to revisit that.Checklist
changie newto create a changelog entry