Consolidate Measure Time Spine Join Conditional Behavior Into MetricInputMeasureSpec
#879
Conversation
plypaul
force-pushed
the
plypaul--58.5--consolidate-into-measure-input-spec
branch
from
8bb0049 to
4eb5355
Compare
plypaul
force-pushed
the
plypaul--58.4--refactor-dfpb
branch
from
0361b0e to
4c6ecb8
Compare
plypaul
force-pushed
the
plypaul--58.5--consolidate-into-measure-input-spec
branch
from
4eb5355 to
524e795
Compare
plypaul
force-pushed
the
plypaul--58.5--consolidate-into-measure-input-spec
branch
from
524e795 to
4b2604d
Compare
| @@ -521,6 +532,20 @@ class LinkableSpecSet(Mergeable, SerializableDataclass): | |||
| time_dimension_specs: Tuple[TimeDimensionSpec, ...] = () | |||
| entity_specs: Tuple[EntitySpec, ...] = () | |||
|
|
|||
| @property | |||
| def contains_metric_time(self) -> bool: | |||
There was a problem hiding this comment.
Helpful thank you!!
metricflow/specs/specs.py
Outdated
| @@ -469,25 +472,33 @@ def reference(self) -> MetricReference: | |||
| return MetricReference(element_name=self.element_name) | |||
|
|
|||
|
|
|||
| @dataclass(frozen=True) | |||
| class MeasureCulminationDescription: | |||
There was a problem hiding this comment.
Did you mean Cumulation instead of Culmination? If not, why Culmination? 🤔
There was a problem hiding this comment.
I think I need to update the dictionary for my IDE:
But yeah, that's off.
| query_contains_metric_time: bool, | ||
| culmination_description: Optional[MeasureCulminationDescription], | ||
| ) -> MetricInputMeasureSpec: | ||
| """Return the measure specs required to compute the base metric.""" |
There was a problem hiding this comment.
Nit: update to singular measure spec
| @@ -645,54 +677,87 @@ def build_computed_metrics_node( | |||
| metric_specs=[metric_spec], | |||
| ) | |||
|
|
|||
| def _measures_for_metric( | |||
| def _build_metric_input_measure_for_base_metric( | |||
There was a problem hiding this comment.
Note that we're about to add conversion measures, which will have multiple input metrics - so this might need to be rewritten by @WilliamDee
There was a problem hiding this comment.
+1 I need this, so we should sync to see whats the best path forward
There was a problem hiding this comment.
we mean multiple input measures, not metrics, but yes.
There was a problem hiding this comment.
im not gonna lie, i always get so confused visually and acoustically. Having to say/see metric input measure instead of just input measure messes with my eyes and ears thinking we're talking about input metrics
There was a problem hiding this comment.
Updated to input_measure.
There was a problem hiding this comment.
There's what appears to be a bad conditional which may or may not matter if you coordinate merging with @courtneyholcomb's work on the nested offset metric fix, and the word culmination does not mean what it's referring to in the code.
| if (metric_spec.offset_to_grain is not None or metric_spec.offset_to_grain is not None) and ( | ||
| metric_input_spec.offset_window is not None or metric_input_spec.offset_to_grain is not None |
There was a problem hiding this comment.
This looks incorrect. Maybe this?
| if (metric_spec.offset_to_grain is not None or metric_spec.offset_to_grain is not None) and ( | |
| metric_input_spec.offset_window is not None or metric_input_spec.offset_to_grain is not None | |
| if (metric_spec.offset_window is not None or metric_spec.offset_to_grain is not None) and ( | |
| metric_input_spec.offset_window is not None or metric_input_spec.offset_to_grain is not None |
I'm not sure it matters if @courtneyholcomb will remove this and fix up immediately afterwards but we might as well fix it in case something has to get reverted or deployed quickly.
| @@ -645,54 +677,87 @@ def build_computed_metrics_node( | |||
| metric_specs=[metric_spec], | |||
| ) | |||
|
|
|||
| def _measures_for_metric( | |||
| def _build_metric_input_measure_for_base_metric( | |||
There was a problem hiding this comment.
we mean multiple input measures, not metrics, but yes.
| query_contains_metric_time=queried_linkable_specs.contains_metric_time, | ||
| child_metric_offset_window=metric_spec.offset_window, | ||
| child_metric_offset_to_grain=metric_spec.offset_to_grain, | ||
| culmination_description=MeasureCulminationDescription( |
There was a problem hiding this comment.
Agree with @courtneyholcomb, this name is odd. I'd go with:
cumulative_description=CumulativeMeasureDescription
Or else
accumulation_description=MeasureAccumulationDescription
plypaul
force-pushed
the
plypaul--58.5--consolidate-into-measure-input-spec
branch
2 times, most recently
from
98444f3 to
3995915
Compare
plypaul
force-pushed
the
plypaul--58.5--consolidate-into-measure-input-spec
branch
from
3995915 to
274d987
Compare
plypaul
deleted the
plypaul--58.5--consolidate-into-measure-input-spec
branch
Description
I found the
DataflowPlanBuilderhard to follow in regards to when the measure would be joined to the time spine. There was conditional logic that depended on several parameters scattered throughout methods. This PR encapsulates the join condition inMetricInputMeasureSpec, and the consolidates filling out those fields when that spec is constructed. Once the behavior is encapsulated, the signature for methods_build_aggregated_measure_from_measure_source_node()can be simplified.There's additional consolidation that can be done for cumulative measure parameters, but putting that off for now as consolidating this logic has been complex as it is. Also, there's a potential edge case with nested derived offset metrics that was found, but it is not addressed here.