Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove granularity restriction for cumulative metrics #1282

Merged
merged 3 commits into from
Jun 19, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
Loading