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

Update core to support DSI 0.8.3 #10990

Merged
merged 8 commits into from
Dec 5, 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/Dependencies-20241112-163815.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
kind: Dependencies
body: Upgrading dbt-semantic-interfaces to 0.8.3 for custom grain support in offset windows
time: 2024-11-12T16:38:15.351519-05:00
custom:
Author: WilliamDee
Issue: None
20 changes: 15 additions & 5 deletions core/dbt/artifacts/resources/v1/metric.py
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,15 @@ def post_aggregation_measure_reference(self) -> MeasureReference:
@dataclass
class MetricTimeWindow(dbtClassMixin):
count: int
granularity: TimeGranularity
granularity: str

@property
def window_string(self) -> str: # noqa: D
return f"{self.count} {self.granularity}"

@property
def is_standard_granularity(self) -> bool: # noqa: D
return self.granularity.casefold() in {item.value.casefold() for item in TimeGranularity}


@dataclass
Expand All @@ -55,7 +63,7 @@ class MetricInput(dbtClassMixin):
filter: Optional[WhereFilterIntersection] = None
alias: Optional[str] = None
offset_window: Optional[MetricTimeWindow] = None
offset_to_grain: Optional[TimeGranularity] = None
offset_to_grain: Optional[str] = None
QMalcolm marked this conversation as resolved.
Show resolved Hide resolved

def as_reference(self) -> MetricReference:
return MetricReference(element_name=self.name)
Expand Down Expand Up @@ -83,7 +91,7 @@ class ConversionTypeParams(dbtClassMixin):
@dataclass
class CumulativeTypeParams(dbtClassMixin):
window: Optional[MetricTimeWindow] = None
grain_to_date: Optional[TimeGranularity] = None
grain_to_date: Optional[str] = None
period_agg: PeriodAggregation = PeriodAggregation.FIRST


Expand All @@ -95,7 +103,9 @@ class MetricTypeParams(dbtClassMixin):
denominator: Optional[MetricInput] = None
expr: Optional[str] = None
window: Optional[MetricTimeWindow] = None
grain_to_date: Optional[TimeGranularity] = None
grain_to_date: Optional[TimeGranularity] = (
QMalcolm marked this conversation as resolved.
Show resolved Hide resolved
None # legacy, use cumulative_type_params.grain_to_date
)
metrics: Optional[List[MetricInput]] = None
conversion_type_params: Optional[ConversionTypeParams] = None
cumulative_type_params: Optional[CumulativeTypeParams] = None
Expand All @@ -121,7 +131,7 @@ class Metric(GraphResource):
type_params: MetricTypeParams
filter: Optional[WhereFilterIntersection] = None
metadata: Optional[SourceFileMetadata] = None
time_granularity: Optional[TimeGranularity] = None
time_granularity: Optional[str] = None
resource_type: Literal[NodeType.Metric]
meta: Dict[str, Any] = field(default_factory=dict, metadata=MergeBehavior.Update.meta())
tags: List[str] = field(default_factory=list)
Expand Down
14 changes: 9 additions & 5 deletions core/dbt/artifacts/resources/v1/semantic_layer_components.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,17 +12,21 @@
class WhereFilter(dbtClassMixin):
where_sql_template: str

@property
def call_parameter_sets(self) -> FilterCallParameterSets:
return WhereFilterParser.parse_call_parameter_sets(self.where_sql_template)
def call_parameter_sets(
self, custom_granularity_names: Sequence[str]
) -> FilterCallParameterSets:
return WhereFilterParser.parse_call_parameter_sets(

Check warning on line 18 in core/dbt/artifacts/resources/v1/semantic_layer_components.py

View check run for this annotation

Codecov / codecov/patch

core/dbt/artifacts/resources/v1/semantic_layer_components.py#L18

Added line #L18 was not covered by tests
self.where_sql_template, custom_granularity_names=custom_granularity_names
)


@dataclass
class WhereFilterIntersection(dbtClassMixin):
where_filters: List[WhereFilter]

@property
def filter_expression_parameter_sets(self) -> Sequence[Tuple[str, FilterCallParameterSets]]:
def filter_expression_parameter_sets(
self, custom_granularity_names: Sequence[str]
) -> Sequence[Tuple[str, FilterCallParameterSets]]:
raise NotImplementedError


Expand Down
11 changes: 11 additions & 0 deletions core/dbt/artifacts/resources/v1/semantic_model.py
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,14 @@
"""


@dataclass
class SemanticLayerElementConfig(dbtClassMixin):
meta: Dict[str, Any] = field(
default_factory=dict,
metadata=MergeBehavior.Update.meta(),
)


@dataclass
class Defaults(dbtClassMixin):
agg_time_dimension: Optional[str] = None
Expand Down Expand Up @@ -72,6 +80,7 @@ class Dimension(dbtClassMixin):
type_params: Optional[DimensionTypeParams] = None
expr: Optional[str] = None
metadata: Optional[SourceFileMetadata] = None
config: Optional[SemanticLayerElementConfig] = None
QMalcolm marked this conversation as resolved.
Show resolved Hide resolved

@property
def reference(self) -> DimensionReference:
Expand Down Expand Up @@ -106,6 +115,7 @@ class Entity(dbtClassMixin):
label: Optional[str] = None
role: Optional[str] = None
expr: Optional[str] = None
config: Optional[SemanticLayerElementConfig] = None

@property
def reference(self) -> EntityReference:
Expand Down Expand Up @@ -147,6 +157,7 @@ class Measure(dbtClassMixin):
agg_params: Optional[MeasureAggregationParameters] = None
non_additive_dimension: Optional[NonAdditiveDimension] = None
agg_time_dimension: Optional[str] = None
config: Optional[SemanticLayerElementConfig] = None

@property
def reference(self) -> MeasureReference:
Expand Down
2 changes: 1 addition & 1 deletion core/dbt/contracts/graph/unparsed.py
Original file line number Diff line number Diff line change
Expand Up @@ -564,7 +564,7 @@ class UnparsedMetricInput(dbtClassMixin):
filter: Union[str, List[str], None] = None
alias: Optional[str] = None
offset_window: Optional[str] = None
offset_to_grain: Optional[str] = None # str is really a TimeGranularity Enum
offset_to_grain: Optional[str] = None


@dataclass
Expand Down
43 changes: 13 additions & 30 deletions core/dbt/parser/schema_yaml_readers.py
Original file line number Diff line number Diff line change
Expand Up @@ -228,19 +228,11 @@ def _get_input_measures(
def _get_period_agg(self, unparsed_period_agg: str) -> PeriodAggregation:
return PeriodAggregation(unparsed_period_agg)

def _get_optional_grain_to_date(
self, unparsed_grain_to_date: Optional[str]
) -> Optional[TimeGranularity]:
if not unparsed_grain_to_date:
return None

return TimeGranularity(unparsed_grain_to_date)

def _get_optional_time_window(
self, unparsed_window: Optional[str]
) -> Optional[MetricTimeWindow]:
if unparsed_window is not None:
parts = unparsed_window.split(" ")
parts = unparsed_window.lower().split(" ")
Copy link
Contributor

@QMalcolm QMalcolm Nov 14, 2024

Choose a reason for hiding this comment

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

Does DSI/Metricflow require these be lowercase now? If so, this may be breaking as for already parsed manifests 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

Nope! It should not be case sensitive. This is lowercasing it before we get it so that the user doesn't need to worry about lowercasing it but we can expect lowercase on our side. Will added tests for case sensitivity in DSI, too.

Copy link
Contributor

@QMalcolm QMalcolm Nov 15, 2024

Choose a reason for hiding this comment

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

The words that concern me are

but we can expect lowercase on our side

I'm not sure you can unfortunately, but I could be wrong about that. Could you point me to the tests around case sensitivity in DSI?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Let me go back and make sure all other fields have this covered, too

Copy link
Contributor

Choose a reason for hiding this comment

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

Alright @QMalcolm - sorry for the delay on this, had some other high prios and then didn't want to mess with code freeze! 🥶

We bumped the version of DSI one more time with some changes to fix the issues with case sensitivity. This is the PR with those changes, but these commits are most relevant: 1, 2.
Let us know if you think there are any other issues here! The thorough review was much appreciated 🙏
Once approved, we shouldn't merge quite yet - just because we want to coordinate the merge timing so that we will have time to update mantle between when this merges and the next sync to mantle.

cc @WilliamDee

Copy link
Contributor

Choose a reason for hiding this comment

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

I think github screwed up those links somehow. They point to https://github.com/dbt-labs/dbt-core/pull/2083a249889252823b097098cb03471f2db0d00e and https://github.com/dbt-labs/dbt-core/pull/00abab69a8b4537abcc0ea9e544547e99a3c27a2. Which are commits, but on DSI, not core.

Copy link
Contributor

Choose a reason for hiding this comment

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

if len(parts) != 2:
raise YamlParseDictError(
self.yaml.path,
Expand All @@ -252,16 +244,11 @@ def _get_optional_time_window(

granularity = parts[1]
# once we drop python 3.8 this could just be `granularity = parts[0].removesuffix('s')
if granularity.endswith("s"):
# months -> month
if granularity.endswith("s") and granularity[:-1] in [
item.value for item in TimeGranularity
]:
# Can only remove the `s` if it's a standard grain, months -> month
granularity = granularity[:-1]
if granularity not in [item.value for item in TimeGranularity]:
raise YamlParseDictError(
self.yaml.path,
"window",
{"window": unparsed_window},
f"Invalid time granularity {granularity} in cumulative/conversion metric window string: ({unparsed_window})",
)

count = parts[0]
if not count.isdigit():
Expand All @@ -274,7 +261,7 @@ def _get_optional_time_window(

return MetricTimeWindow(
count=int(count),
granularity=TimeGranularity(granularity),
granularity=granularity,
)
else:
return None
Expand All @@ -283,16 +270,12 @@ def _get_metric_input(self, unparsed: Union[UnparsedMetricInput, str]) -> Metric
if isinstance(unparsed, str):
return MetricInput(name=unparsed)
else:
offset_to_grain: Optional[TimeGranularity] = None
if unparsed.offset_to_grain is not None:
offset_to_grain = TimeGranularity(unparsed.offset_to_grain)

return MetricInput(
name=unparsed.name,
filter=parse_where_filter(unparsed.filter),
alias=unparsed.alias,
offset_window=self._get_optional_time_window(unparsed.offset_window),
offset_to_grain=offset_to_grain,
offset_to_grain=unparsed.offset_to_grain,
)

def _get_optional_metric_input(
Expand Down Expand Up @@ -354,9 +337,7 @@ def _get_optional_cumulative_type_params(
window=self._get_optional_time_window(
unparsed_type_params.cumulative_type_params.window
),
grain_to_date=self._get_optional_grain_to_date(
unparsed_type_params.cumulative_type_params.grain_to_date
),
grain_to_date=unparsed_type_params.cumulative_type_params.grain_to_date,
period_agg=self._get_period_agg(
unparsed_type_params.cumulative_type_params.period_agg
),
Expand All @@ -369,6 +350,10 @@ def _get_metric_type_params(self, unparsed_metric: UnparsedMetric) -> MetricType

grain_to_date: Optional[TimeGranularity] = None
if type_params.grain_to_date is not None:
# This should've been changed to a string (to support custom grain), but since this
# is a legacy field waiting to be deprecated, we will not support custom grain here
# in order to force customers off of using this field. The field to use should be
# `cumulative_type_params.grain_to_date`
grain_to_date = TimeGranularity(type_params.grain_to_date)

return MetricTypeParams(
Expand Down Expand Up @@ -435,9 +420,7 @@ def parse_metric(self, unparsed: UnparsedMetric, generated_from: Optional[str] =
label=unparsed.label,
type=MetricType(unparsed.type),
type_params=self._get_metric_type_params(unparsed),
time_granularity=(
TimeGranularity(unparsed.time_granularity) if unparsed.time_granularity else None
),
time_granularity=unparsed.time_granularity,
filter=parse_where_filter(unparsed.filter),
meta=unparsed.meta,
tags=unparsed.tags,
Expand Down
2 changes: 1 addition & 1 deletion core/setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@
# These are major-version-0 packages also maintained by dbt-labs.
# Accept patches but avoid automatically updating past a set minor version range.
"dbt-extractor>=0.5.0,<=0.6",
"dbt-semantic-interfaces>=0.7.4,<0.8",
"dbt-semantic-interfaces>=0.8.3,<0.9",
# Minor versions for these are expected to be backwards-compatible
"dbt-common>=1.13.0,<2.0",
"dbt-adapters>=1.10.1,<2.0",
Expand Down
Loading
Loading