Support custom grain in DSI callsites #363
Conversation
WilliamDee
requested review from
QMalcolm,
graciegoheen and
a team
as code owners
…WhereFilterIntersection.filter_expression_parameter_sets from property to a method that takes in the valid custom grain names
…nticWhereFilterIntersection.filter_expression_parameter_sets
WilliamDee
force-pushed
the
will/support-custom-grain
branch
from
dc47c31
to
e20b045
Compare
There was a problem hiding this comment.
Left a couple comments but otherwise looks good!
Since this is a breaking change, I would wait to merge it until you're ready to bump the version (otherwise can risk someone else doing a patch version bump that includes your change). This should be a minor version bump.
@DevonFulcher also has a breaking change PR up. Maybe you guys can coordinate releasing & updating core. Make sure you guys sync with me before you update core because there are some extra steps you'll need to take.
| @@ -91,52 +91,3 @@ def entity_prefix(self) -> Optional[str]: | |||
| return DUNDER.join(tuple(entity_reference.element_name for entity_reference in self.entity_links)) | |||
|
|
|||
| return None | |||
|
|
|||
|
|
|||
| class DunderedNameFormatter: | |||
There was a problem hiding this comment.
I'm assuming you checked that this isn't used in any dependent repos?
There was a problem hiding this comment.
Same for all the other deleted objects!
There was a problem hiding this comment.
yup! now i'm paranoid, gonna make another pass to see
There was a problem hiding this comment.
so DunderedNameFormatter is used in MF, but I thought it should be fine since this is release a minor version upgrade, which shouldn't be picked up by MF right away right? And when I upgrade, i'll update those in MF
There was a problem hiding this comment.
Sounds good! I would double check that metricflow-semantics won't pick it up (it's not always pinned to a patch version)
There was a problem hiding this comment.
Wait ignore me you're right. Minor is not the same is patch. Sorry my brain is busted rn 🙃
| @@ -49,6 +49,7 @@ def create_time_dimension( | |||
| time_granularity_name: Optional[str] = None, | |||
| entity_path: Sequence[str] = (), | |||
| date_part_name: Optional[str] = None, | |||
| custom_granularity_names: Sequence[str] = (), | |||
There was a problem hiding this comment.
Should this not be optional?
There was a problem hiding this comment.
felt better for this to be default to a sequence, so we don't need to handle this at the bottom where we do
StructuredDunderedName.parse_name(
name=time_dimension_name, custom_granularity_names=custom_granularity_names or ()
)There was a problem hiding this comment.
Yeah I guess I just want to make sure people don't skip it without realizing that it's necessary. Like sometimes I like to leave params like this as required so that people are forced to actively pass in the empty value and hopefully in the process they check out what it's used for and make sure they're doing the right thing.
There was a problem hiding this comment.
Ooo yea, ill drop the default
Description
Given that custom granularity is available, we need to start supporting it when parsing through names and filters.
Non-breaking changes
WhereFilterTimeDimensionFactoryas it's not used anywhere (MetricFlow has it's own in metricflow-semantics)Breaking changes
DunderedNameFormatterin favour ofStructuredDunderedNameas it's duplicate logiccustom_granularity_namestoStructuredDunderedName.parse_nameas a parameter to parse out any custom grainPydanticWhereFilter.call_parameter_setsandPydanticWhereFilterIntersection.filter_expression_parameter_setsfrom a property to a method that takes in the valid custom grain namesChecklist
changie newto create a changelog entryResolves SL-2989