-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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 spec MetricNode for dbt x MetricFlow integration #7812
Merged
Merged
Changes from 26 commits
Commits
Show all changes
28 commits
Select commit
Hold shift + click to select a range
ce53ad4
Add dbt-semantic-interfaces as a dependency
QMalcolm 0bcbcec
Add implementations of DSI Metadata protocol to nodes.py
QMalcolm 343b948
First pass at refactoring MetricNode definition to satisfy DSI Metric…
QMalcolm b4c3fac
Fix new optional Metric properties to default to None
QMalcolm 091b68b
Fix tests involving metrics to have updated properties
QMalcolm 8690ab3
Update UnparsedMetricNode to match new metric yaml spec
QMalcolm b0fcad8
Update MetricParser for new unparsed and parsed MetricNodes
QMalcolm 6223efd
Remove `rename_metric_attr`
QMalcolm 1e98f5c
First pass fixing tests for Metrics (1.6) changes
QMalcolm 128a830
Regenerated v10 manifest schema and associated functional test artifa…
QMalcolm e50f0b4
Mark manifest v10 as incompatible with previous version of manifest
QMalcolm 310b220
Remove no longer needed tests
QMalcolm c05c2bc
Skip / comment out tests for metrics functionality that we'll be impl…
QMalcolm 57f7b79
Revert "Mark manifest v10 as incompatible with previous version of ma…
QMalcolm 30a30f4
Begin outputting semantic manifest artifact on every run
QMalcolm 1aaaf29
Drop metrics during upgrade_manifest_json if manifest is v9 or before
QMalcolm 64d39fd
Update properties of `minimal_parsed_metric_dict` to match new metric…
QMalcolm 4132c95
Remove `Replacable` inherited class from metric node objects
QMalcolm 616c836
Move from `Sequence` to `List` for Metric node typing
QMalcolm cefc9a7
Fill out Metric Node `reference` helper method
QMalcolm 797c196
Add changie entry for metric node breaking changes
QMalcolm 4e4690f
Unnest metric filter specification for yaml parsing
QMalcolm 6ffc3b8
Enable easy one line measure, measures, nominator, denominator, and m…
QMalcolm 81e0779
Improve easy of specifying `window` on metrics
QMalcolm cdab99f
Merge branch 'main' into qmalcolm--metric-nodes-match-dsi-spec
QMalcolm 8cf0f3c
Add semantic model nodes to semantic manifest
QMalcolm 6099d00
Update `manifest.pydantic_semantic_manifest` typing to non-optional a…
QMalcolm cc4cd6d
Fix naming 'SourceFileMetadata' post merging in main
QMalcolm File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
kind: Breaking Changes | ||
body: Switch from dbt-metrics to dbt-semantic-interfaces for MetricNode definitions | ||
time: 2023-06-07T19:03:09.680189-07:00 | ||
custom: | ||
Author: QMalcolm | ||
Issue: 7500 7404 |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,8 +23,6 @@ | |
MacroArgument, | ||
MaturityType, | ||
Measure, | ||
MetricFilter, | ||
MetricTime, | ||
Owner, | ||
Quoting, | ||
TestDef, | ||
|
@@ -45,6 +43,10 @@ | |
from dbt.events.contextvars import set_contextvars | ||
from dbt.flags import get_flags | ||
from dbt.node_types import ModelLanguage, NodeType, AccessType | ||
from dbt_semantic_interfaces.references import MeasureReference | ||
from dbt_semantic_interfaces.references import MetricReference as DSIMetricReference | ||
from dbt_semantic_interfaces.type_enums.metric_type import MetricType | ||
from dbt_semantic_interfaces.type_enums.time_granularity import TimeGranularity | ||
|
||
from .model_config import ( | ||
NodeConfig, | ||
|
@@ -566,7 +568,7 @@ class FileSlice(dbtClassMixin, Replaceable): | |
|
||
|
||
@dataclass | ||
class SourceFileMetadata(dbtClassMixin, Replaceable): | ||
class Metadata(dbtClassMixin, Replaceable): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is this rename intentional? re: #7769 (comment) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nope! looks like my merge of |
||
"""Provides file context about what something was created from. | ||
|
||
Implementation of the dbt-semantic-interfaces `Metadata` protocol | ||
|
@@ -1309,27 +1311,76 @@ def same_contents(self, old: Optional["Exposure"]) -> bool: | |
# ==================================== | ||
|
||
|
||
@dataclass | ||
class WhereFilter(dbtClassMixin): | ||
where_sql_template: str | ||
|
||
|
||
@dataclass | ||
class MetricInputMeasure(dbtClassMixin): | ||
name: str | ||
filter: Optional[WhereFilter] = None | ||
alias: Optional[str] = None | ||
|
||
def measure_reference(self) -> MeasureReference: | ||
return MeasureReference(element_name=self.name) | ||
|
||
def post_aggregation_measure_referenc(self) -> MeasureReference: | ||
return MeasureReference(element_name=self.alias or self.name) | ||
|
||
|
||
@dataclass | ||
class MetricTimeWindow(dbtClassMixin): | ||
count: int | ||
granularity: TimeGranularity | ||
|
||
|
||
@dataclass | ||
class MetricInput(dbtClassMixin): | ||
name: str | ||
filter: Optional[WhereFilter] = None | ||
alias: Optional[str] = None | ||
offset_window: Optional[MetricTimeWindow] = None | ||
offset_to_grain: Optional[TimeGranularity] = None | ||
|
||
def as_reference(self) -> DSIMetricReference: | ||
return DSIMetricReference(element_name=self.name) | ||
|
||
|
||
@dataclass | ||
class MetricTypeParams(dbtClassMixin): | ||
measure: Optional[MetricInputMeasure] = None | ||
measures: Optional[List[MetricInputMeasure]] = None | ||
numerator: Optional[MetricInputMeasure] = None | ||
denominator: Optional[MetricInputMeasure] = None | ||
expr: Optional[str] = None | ||
window: Optional[MetricTimeWindow] = None | ||
grain_to_date: Optional[TimeGranularity] = None | ||
metrics: Optional[List[MetricInput]] = None | ||
|
||
def numerator_measure_reference(self) -> Optional[MeasureReference]: | ||
return self.numerator.measure_reference() if self.numerator else None | ||
|
||
def denominator_measure_reference(self) -> Optional[MeasureReference]: | ||
return self.denominator.measure_reference() if self.denominator else None | ||
|
||
|
||
@dataclass | ||
class MetricReference(dbtClassMixin, Replaceable): | ||
sql: Optional[Union[str, int]] | ||
unique_id: Optional[str] | ||
sql: Optional[Union[str, int]] = None | ||
unique_id: Optional[str] = None | ||
|
||
|
||
@dataclass | ||
class Metric(GraphNode): | ||
name: str | ||
description: str | ||
label: str | ||
calculation_method: str | ||
expression: str | ||
filters: List[MetricFilter] | ||
time_grains: List[str] | ||
dimensions: List[str] | ||
type: MetricType | ||
type_params: MetricTypeParams | ||
filter: Optional[WhereFilter] = None | ||
metadata: Optional[Metadata] = None | ||
resource_type: NodeType = field(metadata={"restrict": [NodeType.Metric]}) | ||
timestamp: Optional[str] = None | ||
window: Optional[MetricTime] = None | ||
model: Optional[str] = None | ||
model_unique_id: Optional[str] = None | ||
meta: Dict[str, Any] = field(default_factory=dict) | ||
tags: List[str] = field(default_factory=list) | ||
config: MetricConfig = field(default_factory=MetricConfig) | ||
|
@@ -1353,59 +1404,64 @@ def depends_on_public_nodes(self): | |
def search_name(self): | ||
return self.name | ||
|
||
def same_model(self, old: "Metric") -> bool: | ||
return self.model == old.model | ||
@property | ||
def input_measures(self) -> List[MetricInputMeasure]: | ||
tp = self.type_params | ||
res = tp.measures or [] | ||
if tp.measure: | ||
res.append(tp.measure) | ||
if tp.numerator: | ||
res.append(tp.numerator) | ||
if tp.denominator: | ||
res.append(tp.denominator) | ||
|
||
def same_window(self, old: "Metric") -> bool: | ||
return self.window == old.window | ||
return res | ||
|
||
def same_dimensions(self, old: "Metric") -> bool: | ||
return self.dimensions == old.dimensions | ||
@property | ||
def measure_references(self) -> List[MeasureReference]: | ||
return [x.measure_reference() for x in self.input_measures] | ||
|
||
def same_filters(self, old: "Metric") -> bool: | ||
return self.filters == old.filters | ||
@property | ||
def input_metrics(self) -> List[MetricInput]: | ||
return self.type_params.metrics or [] | ||
|
||
def same_description(self, old: "Metric") -> bool: | ||
return self.description == old.description | ||
|
||
def same_label(self, old: "Metric") -> bool: | ||
return self.label == old.label | ||
|
||
def same_calculation_method(self, old: "Metric") -> bool: | ||
return self.calculation_method == old.calculation_method | ||
|
||
def same_expression(self, old: "Metric") -> bool: | ||
return self.expression == old.expression | ||
|
||
def same_timestamp(self, old: "Metric") -> bool: | ||
return self.timestamp == old.timestamp | ||
|
||
def same_time_grains(self, old: "Metric") -> bool: | ||
return self.time_grains == old.time_grains | ||
|
||
def same_config(self, old: "Metric") -> bool: | ||
return self.config.same_contents( | ||
self.unrendered_config, | ||
old.unrendered_config, | ||
) | ||
|
||
def same_filter(self, old: "Metric") -> bool: | ||
return True # TODO | ||
|
||
def same_metadata(self, old: "Metric") -> bool: | ||
return True # TODO | ||
|
||
def same_type(self, old: "Metric") -> bool: | ||
return self.type == old.type | ||
|
||
def same_type_params(self, old: "Metric") -> bool: | ||
return True # TODO | ||
|
||
def same_contents(self, old: Optional["Metric"]) -> bool: | ||
# existing when it didn't before is a change! | ||
# metadata/tags changes are not "changes" | ||
if old is None: | ||
return True | ||
|
||
return ( | ||
self.same_model(old) | ||
and self.same_window(old) | ||
and self.same_dimensions(old) | ||
and self.same_filters(old) | ||
self.same_filter(old) | ||
and self.same_metadata(old) | ||
and self.same_type(old) | ||
and self.same_type_params(old) | ||
and self.same_description(old) | ||
and self.same_label(old) | ||
and self.same_calculation_method(old) | ||
and self.same_expression(old) | ||
and self.same_timestamp(old) | ||
and self.same_time_grains(old) | ||
and self.same_config(old) | ||
and True | ||
) | ||
|
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Borderline nitpicky, the return type is actually a non-Optional PydanticSemanticManifest, which is the more attractive return type anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh right, good catch! I had planned on having it return
None
if there were not any SemanticModel or Metric nodes 🤦 I think returning an empty semantic model is a pretty good default. I'll get the typing updated, and then also updatewrite_semantic_manifest
to not have to anticipate it ever beingNone