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.

Sign up for GitHub

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

Default granularity prep #1321

Closed
wants to merge 4 commits into from
+22 −1
Closed

Default granularity prep #1321

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
66dd77c
Use SetDefaultGranularityRule in CLI rule set
courtneyholcomb Jul 11, 2024
6e68ad6
Add default_granularity to MetricSpec
courtneyholcomb Jul 11, 2024
fd57e01
Add MetricLookup method to get_default_granularity_for_metrics
courtneyholcomb Jul 11, 2024
7435bee
Remove default_granularity from MetricSpec
courtneyholcomb Jul 12, 2024
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
2 changes: 2 additions & 0 deletions metricflow-semantics/metricflow_semantics/model/dbt_manifest_parser.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
ConvertMedianToPercentileRule,
)
from dbt_semantic_interfaces.transformations.cumulative_type_params import SetCumulativeTypeParamsRule
from dbt_semantic_interfaces.transformations.default_granularity import SetDefaultGranularityRule
from dbt_semantic_interfaces.transformations.names import LowerCaseNamesRule
from dbt_semantic_interfaces.transformations.proxy_measure import CreateProxyMeasureRule
from dbt_semantic_interfaces.transformations.semantic_manifest_transformer import (
Expand Down Expand Up @@ -38,6 +39,7 @@ def parse_manifest_from_dbt_generated_manifest(manifest_json_string: str) -> Pyd
ConvertMedianToPercentileRule(),
DedupeMetricInputMeasuresRule(), # Remove once fix is in core
SetCumulativeTypeParamsRule(),
SetDefaultGranularityRule(),
),
)
model = PydanticSemanticManifestTransformer.transform(raw_model, rules)
Expand Down
15 changes: 15 additions & 0 deletions metricflow-semantics/metricflow_semantics/model/semantics/metric_lookup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,3 +210,18 @@ def get_min_queryable_time_granularity(self, metric_reference: MetricReference)
minimum_queryable_granularity = defined_time_granularity

return minimum_queryable_granularity

def get_default_granularity_for_metrics(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To confirm - what's the expected behavior when you have a derived metric that depends on two other metrics that have different default time granularities? Based on SetDefaultGranularityRule, it seems that the default granularity for parent metrics are not intended to be considered.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's correct. But now that you bring it up, I'm thinking I should confirm that behavior with product, so I'll post about it in #dev-metricflow and see what people think.
Thinking I can probably change this behavior on top of the stack if we decide to go that route.

self, metric_references: Sequence[MetricReference]
) -> Optional[TimeGranularity]:
"""When querying a group of metrics, the default granularity will be the largest of the metrics' default granularities."""
max_default_granularity: Optional[TimeGranularity] = None
for metric_reference in metric_references:
default_granularity = self.get_metric(metric_reference).default_granularity
assert (
default_granularity
), f"No default_granularity set for {metric_reference}. Something has been misconfigured."
if not max_default_granularity or default_granularity.to_int() > max_default_granularity.to_int():
max_default_granularity = default_granularity

return max_default_granularity
8 changes: 7 additions & 1 deletion metricflow-semantics/metricflow_semantics/specs/spec_classes.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -570,6 +570,7 @@ class MetricSpec(InstanceSpec): # noqa: D101
alias: Optional[str] = None
offset_window: Optional[PydanticMetricTimeWindow] = None
offset_to_grain: Optional[TimeGranularity] = None
default_granularity: Optional[TimeGranularity] = None
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fields in the spec classes should be values that can't be inferred from the semantic manifest. From get_default_granularity_for_metrics, it seems like it is?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated!


@staticmethod
def from_element_name(element_name: str) -> MetricSpec: # noqa: D102
Expand Down Expand Up @@ -598,7 +599,12 @@ def has_time_offset(self) -> bool: # noqa: D102

def without_offset(self) -> MetricSpec:
"""Represents the metric spec with any time offsets removed."""
return MetricSpec(element_name=self.element_name, filter_specs=self.filter_specs, alias=self.alias)
return MetricSpec(
element_name=self.element_name,
filter_specs=self.filter_specs,
alias=self.alias,
default_granularity=self.default_granularity,
)


@dataclass(frozen=True)
Expand Down
Loading