Skip to content

Commit

Permalink
Default granularity bug fix: handle time dimensions with the same name (
Browse files Browse the repository at this point in the history
#305)

### Description
Found another bug in SetDefaultGranularityRule. If there are multiple
`agg_time_dimensions` with the same name across semantic models, and
they have different granularities, we'll only check one. Fixed here!

### Checklist

- [x] I have read [the contributing
guide](https://github.com/dbt-labs/dbt-semantic-interfaces/blob/main/CONTRIBUTING.md)
and understand what's expected of me
- [x] I have signed the
[CLA](https://docs.getdbt.com/docs/contributor-license-agreements)
- [x] This PR includes tests, or tests are not required/relevant for
this PR
- [ ] I have run `changie new` to [create a changelog
entry](https://github.com/dbt-labs/dbt-semantic-interfaces/blob/main/CONTRIBUTING.md#adding-a-changelog-entry)
  • Loading branch information
courtneyholcomb authored Jul 3, 2024
1 parent 7e65636 commit 49a0347
Show file tree
Hide file tree
Showing 5 changed files with 85 additions and 23 deletions.
18 changes: 14 additions & 4 deletions dbt_semantic_interfaces/transformations/default_granularity.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
from typing import Dict, Set
import logging
from typing import Dict, Set, Tuple

from typing_extensions import override

Expand All @@ -11,13 +12,16 @@
from dbt_semantic_interfaces.references import (
DimensionReference,
MetricReference,
SemanticModelReference,
TimeDimensionReference,
)
from dbt_semantic_interfaces.transformations.transform_rule import (
SemanticManifestTransformRule,
)
from dbt_semantic_interfaces.type_enums.time_granularity import TimeGranularity

logger = logging.getLogger(__name__)


class SetDefaultGranularityRule(ProtocolHint[SemanticManifestTransformRule[PydanticSemanticManifest]]):
"""If default_granularity is not set for a metric, set it to DAY if available, else the smallest available grain."""
Expand All @@ -34,7 +38,7 @@ def transform_model(semantic_manifest: PydanticSemanticManifest) -> PydanticSema
continue

default_granularity = TimeGranularity.DAY
seen_agg_time_dimensions: Set[TimeDimensionReference] = set()
seen_agg_time_dimensions: Set[Tuple[SemanticModelReference, TimeDimensionReference]] = set()

metric_index: Dict[MetricReference, Metric] = {
MetricReference(metric.name): metric for metric in semantic_manifest.metrics
Expand All @@ -49,10 +53,16 @@ def transform_model(semantic_manifest: PydanticSemanticManifest) -> PydanticSema
except AssertionError:
# This indicates the agg_time_dimension is misconfigured, which will fail elsewhere.
# Do nothing here to avoid disrupting the validation process.
logger.warning(
f"Agg time dimension '{agg_time_dimension_ref.element_name}' not found in model."
"This should raise a validation error elsewhere."
)
continue
if agg_time_dimension_ref in seen_agg_time_dimensions:
# Time dims might have the same names across semantic models, so check model/name combo.
semantic_model_with_agg_time_dimension = (semantic_model.reference, agg_time_dimension_ref)
if semantic_model_with_agg_time_dimension in seen_agg_time_dimensions:
continue
seen_agg_time_dimensions.add(agg_time_dimension_ref)
seen_agg_time_dimensions.add(semantic_model_with_agg_time_dimension)
dimension = semantic_model.get_dimension(DimensionReference(agg_time_dimension_ref.element_name))
if (
dimension.type_params
Expand Down
2 changes: 1 addition & 1 deletion pyproject.toml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[project]
name = "dbt-semantic-interfaces"
version = "0.6.3"
version = "0.6.4"
description = 'The shared semantic layer definitions that dbt-core and MetricFlow use'
readme = "README.md"
requires-python = ">=3.8"
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
---
semantic_model:
name: bookings_monthly
description: bookings_monthly

node_relation:
schema_name: $source_schema
alias: fct_bookings

defaults:
agg_time_dimension: ds

measures:
- name: monthly_bookings
expr: "1"
agg: sum

dimensions:
- name: ds
type: time
type_params:
time_granularity: month

primary_entity: monthly_booking

entities:
- name: listing
type: foreign
expr: listing_id
- name: guest
type: foreign
expr: guest_id
- name: host
type: foreign
expr: host_id
Original file line number Diff line number Diff line change
Expand Up @@ -65,14 +65,6 @@ semantic_model:
percentile: 0.99
use_discrete_percentile: true
use_approximate_percentile: true
- name: monthly_bookings
expr: "1"
agg: sum
agg_time_dimension: ds_monthly
- name: yearly_bookings
expr: "1"
agg: sum
agg_time_dimension: ds_yearly

dimensions:
- name: is_instant
Expand All @@ -90,16 +82,6 @@ semantic_model:
type: time
type_params:
time_granularity: day
- name: ds_monthly
type: time
expr: ds
type_params:
time_granularity: month
- name: ds_yearly
type: time
expr: ds
type_params:
time_granularity: year

primary_entity: booking

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
---
semantic_model:
name: bookings_yearly
description: bookings_yearly

node_relation:
schema_name: $source_schema
alias: fct_bookings

defaults:
agg_time_dimension: ds

measures:
- name: yearly_bookings
expr: "1"
agg: sum

dimensions:
- name: ds
type: time
type_params:
time_granularity: year

primary_entity: yearly_booking

entities:
- name: listing
type: foreign
expr: listing_id
- name: guest
type: foreign
expr: guest_id
- name: host
type: foreign
expr: host_id

0 comments on commit 49a0347

Please sign in to comment.