Skip to content

Commit

Permalink
Remove granularity restriction for cumulative metrics (#1282)
Browse files Browse the repository at this point in the history
Remove the `BaseTimeGrainPattern` filter that was in place for
cumulative metrics. This restricted users from querying cumulative
metrics with `metric_time` using non-default granularities, which will
be supported shortly.
Also a small cleanup item in related code.
Only thing left in the stack after this is tests!
  • Loading branch information
courtneyholcomb authored Jun 19, 2024
1 parent 5fc18b7 commit 7c58282
Show file tree
Hide file tree
Showing 3 changed files with 10 additions and 14 deletions.
6 changes: 6 additions & 0 deletions .changes/unreleased/Features-20240614-071108.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
kind: Features
body: Remove restriction on querying non-default granularities with cumulative metrics.
time: 2024-06-14T07:11:08.765605-07:00
custom:
Author: courtneyholcomb
Issue: "1282"
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,6 @@
MetricFlowQueryResolutionIssueSet,
)
from metricflow_semantics.query.suggestion_generator import QueryItemSuggestionGenerator
from metricflow_semantics.specs.patterns.base_time_grain import BaseTimeGrainPattern
from metricflow_semantics.specs.patterns.none_date_part import NoneDatePartPattern
from metricflow_semantics.specs.patterns.spec_pattern import SpecPattern

Expand Down Expand Up @@ -171,19 +170,8 @@ def visit_measure_node(self, node: MeasureGroupByItemSourceNode) -> PushDownResu
elif metric.type is MetricType.RATIO or metric.type is MetricType.DERIVED:
assert False, f"A measure should have a simple or cumulative metric as a child, but got {metric.type}"
elif metric.type is MetricType.CUMULATIVE:
# To handle the restriction that cumulative metrics can only be queried at the base grain, it's
# easiest to handle that by applying the pattern to remove non-base grain time dimension specs at the
# measure node and generate the issue here if there's nothing that matches. Generating the issue here
# allows for creation of a more specific issue (i.e. include the measure) vs. generating the issue
# at a higher level. This can be more cleanly handled once we add additional context to the
# LinkableInstanceSpec.
patterns_to_apply = (
# From comment in ValidLinkableSpecResolver:
# It's possible to aggregate measures to coarser time granularities
# (except with cumulative metrics).
BaseTimeGrainPattern(only_apply_for_metric_time=True),
# From comment in previous query parser:
# Cannot extract date part for cumulative metrics.
# Date part doesn't make clear sense with cumulative metrics, so we don't allow it.
NoneDatePartPattern(),
)
else:
Expand All @@ -198,7 +186,7 @@ def visit_measure_node(self, node: MeasureGroupByItemSourceNode) -> PushDownResu
f"For {node.ui_description}:\n"
+ indent(
"After applying patterns:\n"
+ indent(mf_pformat(patterns_to_apply))
+ indent(mf_pformat(patterns_to_apply + self._source_spec_patterns))
+ "\n"
+ "to inputs, matches are:\n"
+ indent(mf_pformat(matching_items.specs))
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
[
"TimeDimension('metric_time', 'month')",
"TimeDimension('metric_time', 'quarter')",
"TimeDimension('metric_time', 'year')",
"TimeDimension('monthly_measure_entity__creation_time', 'month')",
"TimeDimension('monthly_measure_entity__creation_time', 'quarter')",
"TimeDimension('monthly_measure_entity__creation_time', 'year')",
Expand Down

0 comments on commit 7c58282

Please sign in to comment.