Add SpecPattern Classes to Model Ambiguous Group-By-Item Inputs
#888
Conversation
plypaul
force-pushed
the
plypaul--58.6--time-dimension-spec-comparison
branch
2 times, most recently
from
f4c20db to
86b41cc
Compare
plypaul
force-pushed
the
plypaul--58.7--spec-pattern
branch
2 times, most recently
from
f654067 to
aba93d0
Compare
There was a problem hiding this comment.
This seems reasonable. See inlines for small improvement requests.
Going forward, please do write out brief commit messages and PR descriptions explaining what is changing and why - pointing to issue links is more indirection for readers to wade through when they often just need a local summary.
metricflow/specs/specs.py
Outdated
| def transform( # noqa: D | ||
| self, transform_function: LinkableSpecSetTransform[LinkableSpecSetTransformOutputT] | ||
| ) -> LinkableSpecSetTransformOutputT: | ||
| return transform_function.transform(self) | ||
|
|
||
|
|
||
| class LinkableSpecSetTransform(Generic[LinkableSpecSetTransformOutputT], ABC): | ||
| """Function to use for transforming LinkableSpecSets. | ||
|
|
||
| Similar to InstanceSpecSetTransform, but for LinkableSpecSets. TBD: There's a lot of similarity between | ||
| InstanceSpecSet and LinkableSpecSet, so there's probably a better class structure so that we don't need to duplicate | ||
| this. | ||
| """ | ||
|
|
||
| @abstractmethod | ||
| def transform(self, spec_set: LinkableSpecSet) -> LinkableSpecSetTransformOutputT: # noqa: D | ||
| pass | ||
|
|
There was a problem hiding this comment.
This is fine in this instance, it's a pattern we have in the codebase, etc., but what are we going to use this for, exactly?
We have this habit of producing broadly generic interfaces for doing arbitrary things and then using them for exactly one concrete application. This adds low grade, everyday friction to working in the codebase. It also encourages adding the broadest possible interface for every operation because that's the pattern we've been following, which then further adds to that friction.
In this PR stack we're essentially doing custom string serialization for converting to various naming representations. What other operations will we perform? If that's kind of all we've got in mind here, I recommend making a more specific API for allowing different forms of string serialization and then generalizing it later if we find a need to do so.
More broadly, I think "add an interface to do the specific thing we're trying to do unless we have a clear need for broader generalization" is a default posture that will serve us well going forward. Broad methods with generic return types like transform(...) -> TransformOutpuT and accept(....) -> VisitorOutputT are the enemy of readability, and we use a lot of them to hide the fact that what we really mean is render_name(...) -> str or convert_to_sql_plan(....) -> SqlQueryPlanNode or render_sql(...) -> SqlRenderResult.
There was a problem hiding this comment.
Removing this interface as later in the stack, this turned out to not be necessary.
| @@ -0,0 +1,174 @@ | |||
| from __future__ import annotations | |||
There was a problem hiding this comment.
Nit-picky but personally important thing - as a person who runs git log in a terminal on a regular basis and reads commit messages to see what's going on and why at a given point in the evolution of a codebase, having commit messages that contain only pointers to some github link is super annoying.
I know it feels like duplicated effort, but adding 2 sentences describing what this commit is doing and why it's here would be super handy. That's especially true in the case where this is one commit of many targeted at resolving an issue consisting of 1500 words of explanation complete with diagrams.
Don't get me wrong, I love that issue writeup, I just don't want to have to wade through all of it when I'm looking at the codebase and thinking "wait, why is this like this? Oh it was always this way, let me see if the commit message helps me out at all." Often enough the 2 sentence thing is adequate, and it saves me time.
There was a problem hiding this comment.
Expanded the commit message, but also left the link in there as I thought it would still be useful. It's now:
Add SpecPattern classes.
These pattern classes are used to model user inputs when group-by items are
specified through the query interface or specified in a filter. These patterns
allow for ambiguous user inputs of group-by items e.g. a time dimension with
an unknown grain. For that case, the ambiguous input makes it easier for the
user to author queries as figuring out the time grain requires inspection of
the configs. For more details, please see:
https://github.com/dbt-labs/metricflow/issues/887
| from metricflow.specs.specs import LinkableInstanceSpec | ||
|
|
||
|
|
||
| class SpecPattern(ABC): |
There was a problem hiding this comment.
nit: LinkableSpecPattern? It only works on LinkableInstanceSpecs.
There was a problem hiding this comment.
Actually, this was updated later in the stack to apply for any spec. Moved those changes into this commit.
|
|
||
| for spec in candidate_specs: | ||
| spec_values = tuple( | ||
| (getattr(spec, key_to_check) if hasattr(spec, key_to_check) else None) for key_to_check in keys_to_check |
There was a problem hiding this comment.
getattr allows for a default return:
getattr(spec, key_to_check, default=None) for key_to_check in keys_to_check
| ParameterSetField.DATE_PART, | ||
| ] | ||
|
|
||
| if time_dimension_call_parameter_set.time_granularity is not None: | ||
| fields_to_compare.append(ParameterSetField.TIME_GRANULARITY) |
There was a problem hiding this comment.
It's a little puzzling to me why one of these is using an is not None check but not the other. Is there any reason not to just compare both of these every time? Why do we care if the granularity is set for the purposes of the match call?
It appears the reason is so we can use None as a value for "any time granularity" here. Regardless, adding a docstring to the method explaining this instead of suppressing lint would be great.
There was a problem hiding this comment.
Yeah, that's the case to follow the behavior that's defined for how the time grain / date part are used in the object builder naming scheme. Added a comment.
plypaul
force-pushed
the
plypaul--58.7--spec-pattern
branch
from
aba93d0 to
a12e578
Compare
These pattern classes are used to model user inputs when group-by items are specified through the query interface or specified in a filter. These patterns allow for ambiguous user inputs of group-by items e.g. a time dimension with an unknown grain. For that case, the ambiguous input makes it easier for the user to author queries as figuring out the time grain requires inspection of the configs. For more details, please see: #887
plypaul
force-pushed
the
plypaul--58.7--spec-pattern
branch
from
a12e578 to
1db958e
Compare
Description
This PR implements the pattern classes for ambiguous group-by-items as described in https://github.com/dbt-labs/metricflow/pull/new/plypaul--58.7--spec-pattern.