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

Bug fix: non-additive dimension with non-default grain #1384

Merged
merged 6 commits into from
Aug 28, 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/Fixes-20240827-130128.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
kind: Fixes
body: 'Bug fix: ensure that granularity requested for non-additive dimension is respected.'
time: 2024-08-27T13:01:28.743199-07:00
custom:
Author: courtneyholcomb
Issue: "1383"
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
from typing import Any, Sequence, Tuple

from dbt_semantic_interfaces.dataclass_serialization import SerializableDataclass
from dbt_semantic_interfaces.type_enums import AggregationType
from dbt_semantic_interfaces.type_enums import AggregationType, TimeGranularity

from metricflow_semantics.specs.entity_spec import LinklessEntitySpec
from metricflow_semantics.specs.instance_spec import LinkableInstanceSpec
Expand Down Expand Up @@ -40,9 +40,10 @@ def bucket_hash(self) -> str:
values.extend(sorted(self.window_groupings))
return hash_items(values)

@property
def linkable_specs(self) -> Sequence[LinkableInstanceSpec]: # noqa: D102
return (TimeDimensionSpec.from_name(self.name),) + tuple(
def linkable_specs( # noqa: D102
self, non_additive_dimension_grain: TimeGranularity
) -> Sequence[LinkableInstanceSpec]:
return (TimeDimensionSpec.from_name(self.name).with_grain(non_additive_dimension_grain),) + tuple(
LinklessEntitySpec.from_element_name(entity_name) for entity_name in self.window_groupings
)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,11 +30,25 @@ semantic_model:
window_groupings:
- user

- name: total_account_balance_first_day_of_month
agg: sum
expr: account_balance
agg_time_dimension: ds_month
non_additive_dimension:
name: ds_month
window_choice: min
create_metric: true

dimensions:
- name: ds
type: time
type_params:
time_granularity: day
- name: ds_month
type: time
expr: ds_month
type_params:
time_granularity: month
- name: account_type
type: categorical

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,7 @@
'user__account__user__current_account_balance_by_user',
'user__account__user__regional_starting_balance_ratios',
'user__account__user__total_account_balance_first_day',
'user__account__user__total_account_balance_first_day_of_month',
'user__active_listings',
'user__archived_at__day',
'user__archived_at__extract_day',
Expand Down Expand Up @@ -461,6 +462,7 @@
'user__smallest_listing',
'user__subdaily_join_to_time_spine_metric',
'user__total_account_balance_first_day',
'user__total_account_balance_first_day_of_month',
'user__verification__user__identity_verifications',
'user__view__user__views',
'user__views',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
'created_at',
'ds',
'ds_latest',
'ds_month',
'ds_partitioned',
'home_state',
'home_state_latest',
Expand Down Expand Up @@ -46,6 +47,7 @@
'referred_bookings',
'smallest_listing',
'total_account_balance_first_day',
'total_account_balance_first_day_of_month',
'txn_revenue',
'views',
'visitors',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,7 @@ Model Join-Path Entity Links
('listings_latest',) ("('user',)", "('account', 'user')") current_account_balance_by_user ['JOINED', 'METRIC']
('listings_latest',) ("('user',)", "('account', 'user')") regional_starting_balance_ratios ['JOINED', 'METRIC']
('listings_latest',) ("('user',)", "('account', 'user')") total_account_balance_first_day ['JOINED', 'METRIC']
('listings_latest',) ("('user',)", "('account', 'user')") total_account_balance_first_day_of_month ['JOINED', 'METRIC']
('listings_latest',) ("('user',)", "('listing', 'user')") active_listings ['JOINED', 'METRIC']
('listings_latest',) ("('user',)", "('listing', 'user')") approximate_continuous_booking_value_p99 ['JOINED', 'METRIC']
('listings_latest',) ("('user',)", "('listing', 'user')") approximate_discrete_booking_value_p99 ['JOINED', 'METRIC']
Expand Down Expand Up @@ -193,6 +194,7 @@ Model Join-Path Entity Links
('listings_latest',) ("('user',)", "('user',)") smallest_listing ['JOINED', 'METRIC']
('listings_latest',) ("('user',)", "('user',)") subdaily_join_to_time_spine_metric ['JOINED', 'METRIC']
('listings_latest',) ("('user',)", "('user',)") total_account_balance_first_day ['JOINED', 'METRIC']
('listings_latest',) ("('user',)", "('user',)") total_account_balance_first_day_of_month ['JOINED', 'METRIC']
('listings_latest',) ("('user',)", "('user',)") views ['JOINED', 'METRIC']
('listings_latest',) ("('user',)", "('user',)") visit_buy_conversion_rate ['JOINED', 'METRIC']
('listings_latest',) ("('user',)", "('user',)") visit_buy_conversion_rate_7days ['JOINED', 'METRIC']
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,10 @@
'account__ds__extract_month',
'account__ds__extract_quarter',
'account__ds__extract_year',
'account__ds_month__extract_month',
'account__ds_month__extract_quarter',
'account__ds_month__extract_year',
'account__ds_month__month',
'account__user',
'booking__ds__day',
'booking__ds__extract_day',
Expand Down Expand Up @@ -104,6 +108,7 @@
'company__user__company__smallest_listing',
'company__user__company__subdaily_join_to_time_spine_metric',
'company__user__company__total_account_balance_first_day',
'company__user__company__total_account_balance_first_day_of_month',
'company__user__company__views',
'company__user__company__visit_buy_conversion_rate',
'company__user__company__visit_buy_conversion_rate_7days',
Expand Down Expand Up @@ -448,6 +453,7 @@
'user__account__user__current_account_balance_by_user',
'user__account__user__regional_starting_balance_ratios',
'user__account__user__total_account_balance_first_day',
'user__account__user__total_account_balance_first_day_of_month',
'user__active_listings',
'user__archived_at__extract_day',
'user__archived_at__extract_dow',
Expand Down Expand Up @@ -577,6 +583,7 @@
'user__smallest_listing',
'user__subdaily_join_to_time_spine_metric',
'user__total_account_balance_first_day',
'user__total_account_balance_first_day_of_month',
'user__verification__user__identity_verifications',
'user__view__user__views',
'user__views',
Expand Down
14 changes: 12 additions & 2 deletions metricflow/dataflow/builder/dataflow_plan_builder.py
Original file line number Diff line number Diff line change
Expand Up @@ -1348,8 +1348,13 @@ def __get_required_and_extraneous_linkable_specs(
for filter_spec in filter_specs:
linkable_spec_sets_to_merge.append(LinkableSpecSet.create_from_specs(filter_spec.linkable_specs))
if non_additive_dimension_spec:
non_additive_dimension_grain = self._semantic_model_lookup.get_defined_time_granularity(
TimeDimensionReference(non_additive_dimension_spec.name)
)
linkable_spec_sets_to_merge.append(
LinkableSpecSet.create_from_specs(non_additive_dimension_spec.linkable_specs)
LinkableSpecSet.create_from_specs(
non_additive_dimension_spec.linkable_specs(non_additive_dimension_grain)
)
)

extraneous_linkable_specs = LinkableSpecSet.merge_iterable(linkable_spec_sets_to_merge).dedupe()
Expand Down Expand Up @@ -1543,14 +1548,19 @@ def _build_aggregated_measure_from_measure_source_node(
if non_additive_dimension_spec is not None:
# Apply semi additive join on the node
agg_time_dimension = measure_properties.agg_time_dimension
non_additive_dimension_grain = self._semantic_model_lookup.get_defined_time_granularity(
TimeDimensionReference(non_additive_dimension_spec.name)
)
queried_time_dimension_spec: Optional[
TimeDimensionSpec
] = self._find_non_additive_dimension_in_linkable_specs(
agg_time_dimension=agg_time_dimension,
linkable_specs=queried_linkable_specs.as_tuple,
non_additive_dimension_spec=non_additive_dimension_spec,
)
time_dimension_spec = TimeDimensionSpec.from_name(non_additive_dimension_spec.name)
time_dimension_spec = TimeDimensionSpec.from_name(non_additive_dimension_spec.name).with_grain(
time_granularity=non_additive_dimension_grain
)
window_groupings = tuple(
LinklessEntitySpec.from_element_name(name) for name in non_additive_dimension_spec.window_groupings
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,25 +3,27 @@ table_snapshot:
column_definitions:
- name: ds
type: TIME
- name: ds_month
type: TIME
- name: user_id
type: STRING
- name: account_balance
type: INT
- name: account_type
type: STRING
rows:
- ["2020-01-01", "u0004114", "2135", "checking"]
- ["2020-01-01", "u0004114", "1234", "savings"]
- ["2020-01-01", "u0003141", "24634", "checking"]
- ["2020-01-01", "u0003154", "23452", "checking"]
- ["2020-01-02", "u1612112", "5123", "checking"]
- ["2020-01-02", "u0005432", "8456", "savings"]
- ["2020-01-02", "u0003452", "6939", "checking"]
- ["2020-01-03", "u0005432", "5234", "checking"]
- ["2020-01-03", "u0005432", "234", "savings"]
- ["2020-01-04", "u0005414", "5582", "savings"]
- ["2020-01-06", "u0004114", "1213", "savings"]
- ["2020-01-07", "u0004114", "523", "checking"]
- ["2020-01-10", "u0004114", "7434", "checkings"]
- ["2020-01-12", "u0003141", "12939", "checking"]
- ["2020-01-12", "u0003452", "35", "checking"]
- ["2020-01-01", "2020-01-01", "u0004114", "2135", "checking"]
- ["2020-01-01", "2020-01-01", "u0004114", "1234", "savings"]
- ["2020-01-01", "2020-01-01", "u0003141", "24634", "checking"]
- ["2020-01-01", "2020-01-01", "u0003154", "23452", "checking"]
- ["2020-01-02", "2020-01-01", "u1612112", "5123", "checking"]
- ["2020-01-02", "2020-01-01", "u0005432", "8456", "savings"]
- ["2020-01-02", "2020-01-01", "u0003452", "6939", "checking"]
- ["2020-01-03", "2020-01-01", "u0005432", "5234", "checking"]
- ["2020-01-03", "2020-01-01", "u0005432", "234", "savings"]
- ["2020-01-04", "2020-01-01", "u0005414", "5582", "savings"]
- ["2020-01-06", "2020-01-01", "u0004114", "1213", "savings"]
- ["2020-01-07", "2020-01-01", "u0004114", "523", "checking"]
- ["2020-01-10", "2020-01-01", "u0004114", "7434", "checkings"]
- ["2020-01-12", "2020-01-01", "u0003141", "12939", "checking"]
- ["2020-01-12", "2020-01-01", "u0003452", "35", "checking"]
Original file line number Diff line number Diff line change
Expand Up @@ -259,3 +259,24 @@ integration_test:
b.ds = d.ds__complete
GROUP BY
b.account_type
---
integration_test:
name: non_default_grain
description: Tests selecting a semi-additive measure with an agg_time_dimension that has non-default grain
model: SIMPLE_MODEL
metrics: ["total_account_balance_first_day_of_month"]
check_query: |
SELECT
SUM(subq_2.total_account_balance_first_day_of_month) AS total_account_balance_first_day_of_month
FROM (
SELECT
{{ render_date_trunc("ds", TimeGranularity.MONTH) }} AS ds_month__month
, account_balance AS total_account_balance_first_day_of_month
FROM {{ source_schema }}.fct_accounts
) subq_2
INNER JOIN (
SELECT
MIN({{ render_date_trunc("ds", TimeGranularity.MONTH) }}) AS ds_month__month__complete
FROM {{ source_schema }}.fct_accounts
) subq_4
ON subq_2.ds_month__month = subq_4.ds_month__month__complete
23 changes: 23 additions & 0 deletions tests_metricflow/query_rendering/test_query_rendering.py
Original file line number Diff line number Diff line change
Expand Up @@ -565,3 +565,26 @@ def test_min_max_metric_time_week(
dataflow_plan_builder=dataflow_plan_builder,
query_spec=query_spec,
)


@pytest.mark.sql_engine_snapshot
def test_non_additive_dimension_with_non_default_grain(
request: FixtureRequest,
mf_test_configuration: MetricFlowTestConfiguration,
dataflow_plan_builder: DataflowPlanBuilder,
sql_client: SqlClient,
dataflow_to_sql_converter: DataflowToSqlQueryPlanConverter,
) -> None:
"""Tests querying a metric with a non-additive agg_time_dimension that has non-default granularity."""
query_spec = MetricFlowQuerySpec(
metric_specs=(MetricSpec(element_name="total_account_balance_first_day_of_month"),)
)

render_and_check(
request=request,
mf_test_configuration=mf_test_configuration,
dataflow_to_sql_converter=dataflow_to_sql_converter,
sql_client=sql_client,
dataflow_plan_builder=dataflow_plan_builder,
query_spec=query_spec,
)
Loading
Loading