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

Handle no-metric queries with sub-daily metric_time #1359

Merged
merged 9 commits into from
Jul 30, 2024
Merged

Handle no-metric queries with sub-daily metric_time #1359

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
1b4bdf7
Update invalid granularity assumptions
courtneyholcomb Jul 27, 2024
1c772f1
Handle no-metric queries with sub-daily metric_time
courtneyholcomb Jul 27, 2024
6df42d9
Changelog
courtneyholcomb Jul 27, 2024
2c440f2
Add new test dimensions with sub-daily granularities
courtneyholcomb Jul 29, 2024
de20f4f
Determine if plan should be distinct_values from params
courtneyholcomb Jul 29, 2024
16c999b
Add SQL rendering tests for no-metric queries
courtneyholcomb Jul 29, 2024
bf490d1
Add check query tests for no-metric queries
courtneyholcomb Jul 29, 2024
e984404
Update SQL engine snapshots
courtneyholcomb Jul 29, 2024
763db74
Fix issue causing inconsistent ordering of specs in snapshots
courtneyholcomb Jul 29, 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
24 changes: 16 additions & 8 deletions metricflow-semantics/metricflow_semantics/model/semantics/linkable_spec_resolver.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
from metricflow_semantics.model.semantics.linkable_element_set import LinkableElementSet
from metricflow_semantics.model.semantics.semantic_model_join_evaluator import SemanticModelJoinEvaluator
from metricflow_semantics.specs.time_dimension_spec import DEFAULT_TIME_GRANULARITY
from metricflow_semantics.time.time_spine_source import TimeSpineSource

if TYPE_CHECKING:
from metricflow_semantics.model.semantics.semantic_model_lookup import SemanticModelLookup
Expand Down Expand Up @@ -124,6 +125,7 @@ def __init__(
# Sort semantic models by name for consistency in building derived objects.
self._semantic_models = sorted(self._semantic_manifest.semantic_models, key=lambda x: x.name)
self._join_evaluator = SemanticModelJoinEvaluator(semantic_model_lookup)
self._time_spine_sources = TimeSpineSource.create_from_manifest(self._semantic_manifest)

assert max_entity_links >= 0
self._max_entity_links = max_entity_links
Expand Down Expand Up @@ -454,6 +456,7 @@ def _get_metric_time_elements(self, measure_reference: Optional[MeasureReference
on what aggregation time dimension was used to define the measure.
"""
measure_semantic_model: Optional[SemanticModel] = None
defined_granularity: Optional[TimeGranularity] = None
if measure_reference:
measure_semantic_model = self._get_semantic_model_for_measure(measure_reference)
measure_agg_time_dimension_reference = measure_semantic_model.checked_agg_time_dimension_for_measure(
Expand All @@ -463,15 +466,20 @@ def _get_metric_time_elements(self, measure_reference: Optional[MeasureReference
semantic_model=measure_semantic_model,
time_dimension_reference=measure_agg_time_dimension_reference,
)
possible_metric_time_granularities = tuple(
time_granularity
for time_granularity in TimeGranularity
if defined_granularity.is_smaller_than_or_equal(time_granularity)
)
else:
defined_granularity = DEFAULT_TIME_GRANULARITY

# It's possible to aggregate measures to coarser time granularities (except with cumulative metrics).
possible_metric_time_granularities = tuple(
time_granularity
for time_granularity in TimeGranularity
if defined_granularity.is_smaller_than_or_equal(time_granularity)
)
# If querying metric_time without metrics, will query from time spines.
# Defaults to DAY granularity if available in time spines, else smallest available granularity.
min_time_spine_granularity = min(self._time_spine_sources.keys())
Copy link
Contributor

Choose a reason for hiding this comment

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

This default is because the time_spine_sources automatically include the DAY granularity (via TimeSpineSource.create_from_manifest()) if no custom time spines are found, correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This default is set so that if you query ambiguous metric_time without metrics, we don't return nanoseconds if you have that time spine configured. Since metric_time defaults to DAY in all other scenarios (unless overridden by the metric's default granularity), this seemed like the expected behavior.

possible_metric_time_granularities = tuple(
time_granularity
for time_granularity in TimeGranularity
if min_time_spine_granularity.is_smaller_than_or_equal(time_granularity)
)

# For each of the possible time granularities, create a LinkableDimension.
path_key_to_linkable_dimensions: Dict[ElementPathKey, List[LinkableDimension]] = defaultdict(list)
Expand Down
14 changes: 8 additions & 6 deletions metricflow-semantics/metricflow_semantics/specs/patterns/metric_time_default_granularity.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
from metricflow_semantics.specs.patterns.spec_pattern import SpecPattern
from metricflow_semantics.specs.spec_set import group_specs_by_type
from metricflow_semantics.specs.time_dimension_spec import (
DEFAULT_TIME_GRANULARITY,
TimeDimensionSpec,
TimeDimensionSpecComparisonKey,
TimeDimensionSpecField,
Expand Down Expand Up @@ -47,10 +48,13 @@ def __init__(self, max_metric_default_time_granularity: Optional[TimeGranularity
def match(self, candidate_specs: Sequence[InstanceSpec]) -> Sequence[InstanceSpec]:
spec_set = group_specs_by_type(candidate_specs)

# If there are no metrics or metric_time specs in the query, skip this filter.
if not (self._max_metric_default_time_granularity and spec_set.metric_time_specs):
# If there are no metric_time specs in the query, skip this filter.
if not spec_set.metric_time_specs:
return candidate_specs

# If there are metrics in the query, use max metric default. For no-metric queries, use standard default.
default_granularity = self._max_metric_default_time_granularity or DEFAULT_TIME_GRANULARITY

spec_key_to_grains: Dict[TimeDimensionSpecComparisonKey, Set[TimeGranularity]] = defaultdict(set)
spec_key_to_specs: Dict[TimeDimensionSpecComparisonKey, Tuple[TimeDimensionSpec, ...]] = defaultdict(tuple)
for metric_time_spec in spec_set.metric_time_specs:
Expand All @@ -60,10 +64,8 @@ def match(self, candidate_specs: Sequence[InstanceSpec]) -> Sequence[InstanceSpe

matched_metric_time_specs: Tuple[TimeDimensionSpec, ...] = ()
for spec_key, time_grains in spec_key_to_grains.items():
if self._max_metric_default_time_granularity in time_grains:
matched_metric_time_specs += (
spec_key_to_specs[spec_key][0].with_grain(self._max_metric_default_time_granularity),
)
if default_granularity in time_grains:
matched_metric_time_specs += (spec_key_to_specs[spec_key][0].with_grain(default_granularity),)
else:
# If default_granularity is not in the available options, then time granularity was specified in the request
# and a default is not needed here. Pass all options through for this spec key.
Expand Down
42 changes: 42 additions & 0 deletions ...ble_group_by_items.py/LinkableSpecSet/test_available_group_by_items__no_metrics__set0.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,55 @@
"TimeDimension('metric_time', 'day', date_part_name='month')",
"TimeDimension('metric_time', 'day', date_part_name='quarter')",
"TimeDimension('metric_time', 'day', date_part_name='year')",
"TimeDimension('metric_time', 'hour')",
"TimeDimension('metric_time', 'hour', date_part_name='day')",
"TimeDimension('metric_time', 'hour', date_part_name='dow')",
"TimeDimension('metric_time', 'hour', date_part_name='doy')",
"TimeDimension('metric_time', 'hour', date_part_name='month')",
"TimeDimension('metric_time', 'hour', date_part_name='quarter')",
"TimeDimension('metric_time', 'hour', date_part_name='year')",
"TimeDimension('metric_time', 'microsecond')",
"TimeDimension('metric_time', 'microsecond', date_part_name='day')",
"TimeDimension('metric_time', 'microsecond', date_part_name='dow')",
"TimeDimension('metric_time', 'microsecond', date_part_name='doy')",
"TimeDimension('metric_time', 'microsecond', date_part_name='month')",
"TimeDimension('metric_time', 'microsecond', date_part_name='quarter')",
"TimeDimension('metric_time', 'microsecond', date_part_name='year')",
"TimeDimension('metric_time', 'millisecond')",
"TimeDimension('metric_time', 'millisecond', date_part_name='day')",
"TimeDimension('metric_time', 'millisecond', date_part_name='dow')",
"TimeDimension('metric_time', 'millisecond', date_part_name='doy')",
"TimeDimension('metric_time', 'millisecond', date_part_name='month')",
"TimeDimension('metric_time', 'millisecond', date_part_name='quarter')",
"TimeDimension('metric_time', 'millisecond', date_part_name='year')",
"TimeDimension('metric_time', 'minute')",
"TimeDimension('metric_time', 'minute', date_part_name='day')",
"TimeDimension('metric_time', 'minute', date_part_name='dow')",
"TimeDimension('metric_time', 'minute', date_part_name='doy')",
"TimeDimension('metric_time', 'minute', date_part_name='month')",
"TimeDimension('metric_time', 'minute', date_part_name='quarter')",
"TimeDimension('metric_time', 'minute', date_part_name='year')",
"TimeDimension('metric_time', 'month')",
"TimeDimension('metric_time', 'month', date_part_name='month')",
"TimeDimension('metric_time', 'month', date_part_name='quarter')",
"TimeDimension('metric_time', 'month', date_part_name='year')",
"TimeDimension('metric_time', 'nanosecond')",
"TimeDimension('metric_time', 'nanosecond', date_part_name='day')",
"TimeDimension('metric_time', 'nanosecond', date_part_name='dow')",
"TimeDimension('metric_time', 'nanosecond', date_part_name='doy')",
"TimeDimension('metric_time', 'nanosecond', date_part_name='month')",
"TimeDimension('metric_time', 'nanosecond', date_part_name='quarter')",
"TimeDimension('metric_time', 'nanosecond', date_part_name='year')",
"TimeDimension('metric_time', 'quarter')",
"TimeDimension('metric_time', 'quarter', date_part_name='quarter')",
"TimeDimension('metric_time', 'quarter', date_part_name='year')",
"TimeDimension('metric_time', 'second')",
"TimeDimension('metric_time', 'second', date_part_name='day')",
"TimeDimension('metric_time', 'second', date_part_name='dow')",
"TimeDimension('metric_time', 'second', date_part_name='doy')",
"TimeDimension('metric_time', 'second', date_part_name='month')",
"TimeDimension('metric_time', 'second', date_part_name='quarter')",
"TimeDimension('metric_time', 'second', date_part_name='year')",
"TimeDimension('metric_time', 'week')",
"TimeDimension('metric_time', 'week', date_part_name='month')",
"TimeDimension('metric_time', 'week', date_part_name='quarter')",
Expand Down
2 changes: 1 addition & 1 deletion .../GroupByItemResolution/test_ambiguous_metric_time_in_query_filter__no_metrics__result.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ GroupByItemResolution(
time_granularity=DAY,
): (
LinkableDimension(
properties=(METRIC_TIME,),
properties=(DERIVED_TIME_GRANULARITY, METRIC_TIME),
element_name='metric_time',
dimension_type=TIME,
join_path=SemanticModelJoinPath(
Expand Down
1 change: 0 additions & 1 deletion ...emantic_model_container.py/tuple/test_linkable_elements_for_no_metrics_query__result0.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -419,7 +419,6 @@
'lux_listing__listing__lux_listing__twice_bookings_fill_nulls_with_0_without_time_spine',
'lux_listing__listing__lux_listing__views',
'lux_listing__listing__lux_listing__views_times_booking_value',
'metric_time__day',
Copy link
Contributor

Choose a reason for hiding this comment

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

Wait, is this expected?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes! I had the same question and meant to leave a comment explaining but I forgot.
This test is specifically filtering out specs with the property DERIVED_TIME_GRANULARITY. I'm not sure why. But I made a change in this PR that makes all metric_time granularities derived for no-metric queries.
Debatable which behavior would be expected for that property here, but doesn't seem super important since it's only internal.

'revenue_instance__ds__day',
'revenue_instance__ds__extract_day',
'revenue_instance__ds__extract_dow',
Expand Down