Skip to content

Commit

Permalink
Renaming Attributes In Metric Spec (#5775)
Browse files Browse the repository at this point in the history
* making updates - see what fails

* updating tests

* adding timestamp to ok_metric_no_model

* adding changie and fixing description error

* test fixes

* updating schema renderer

* fixing test_yaml_render

* file cleaning and window tests
  • Loading branch information
callum-mcdata authored Sep 9, 2022
1 parent 79da002 commit cfece2c
Show file tree
Hide file tree
Showing 17 changed files with 133 additions and 118 deletions.
7 changes: 7 additions & 0 deletions .changes/unreleased/Breaking Changes-20220906-154521.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
kind: Breaking Changes
body: Renaming Metric Spec Attributes
time: 2022-09-06T15:45:21.2769-05:00
custom:
Author: callum-mcdata
Issue: "5774"
PR: "5775"
12 changes: 8 additions & 4 deletions core/dbt/contracts/graph/metrics.py
Original file line number Diff line number Diff line change
Expand Up @@ -49,13 +49,17 @@ def parent_metrics_names(cls, metric_node, manifest):

@classmethod
def reverse_dag_parsing(cls, metric_node, manifest, metric_depth_count):
if metric_node.type == "expression":
if metric_node.calculation_method == "derived":
yield {metric_node.name: metric_depth_count}
metric_depth_count = metric_depth_count + 1

for parent_unique_id in metric_node.depends_on.nodes:
node = manifest.metrics.get(parent_unique_id)
if node and node.resource_type == NodeType.Metric and node.type == "expression":
if (
node
and node.resource_type == NodeType.Metric
and node.calculation_method == "derived"
):
yield from cls.reverse_dag_parsing(node, manifest, metric_depth_count)

def full_metric_dependency(self):
Expand All @@ -67,7 +71,7 @@ def base_metric_dependency(self):

to_return = []
for metric in in_scope_metrics:
if metric.type != "expression" and metric.name not in to_return:
if metric.calculation_method != "derived" and metric.name not in to_return:
to_return.append(metric.name)

return to_return
Expand All @@ -77,7 +81,7 @@ def derived_metric_dependency(self):

to_return = []
for metric in in_scope_metrics:
if metric.type == "expression" and metric.name not in to_return:
if metric.calculation_method == "derived" and metric.name not in to_return:
to_return.append(metric.name)

return to_return
Expand Down
16 changes: 8 additions & 8 deletions core/dbt/contracts/graph/parsed.py
Original file line number Diff line number Diff line change
Expand Up @@ -806,8 +806,8 @@ class ParsedMetric(UnparsedBaseNode, HasUniqueID, HasFqn):
name: str
description: str
label: str
type: str
sql: str
calculation_method: str
expression: str
timestamp: Optional[str]
filters: List[MetricFilter]
time_grains: List[str]
Expand Down Expand Up @@ -850,11 +850,11 @@ def same_description(self, old: "ParsedMetric") -> bool:
def same_label(self, old: "ParsedMetric") -> bool:
return self.label == old.label

def same_type(self, old: "ParsedMetric") -> bool:
return self.type == old.type
def same_calculation_method(self, old: "ParsedMetric") -> bool:
return self.calculation_method == old.calculation_method

def same_sql(self, old: "ParsedMetric") -> bool:
return self.sql == old.sql
def same_expression(self, old: "ParsedMetric") -> bool:
return self.expression == old.expression

def same_timestamp(self, old: "ParsedMetric") -> bool:
return self.timestamp == old.timestamp
Expand All @@ -875,8 +875,8 @@ def same_contents(self, old: Optional["ParsedMetric"]) -> bool:
and self.same_filters(old)
and self.same_description(old)
and self.same_label(old)
and self.same_type(old)
and self.same_sql(old)
and self.same_calculation_method(old)
and self.same_expression(old)
and self.same_timestamp(old)
and self.same_time_grains(old)
and True
Expand Down
20 changes: 12 additions & 8 deletions core/dbt/contracts/graph/unparsed.py
Original file line number Diff line number Diff line change
Expand Up @@ -451,13 +451,13 @@ class UnparsedMetric(dbtClassMixin, Replaceable):
# name: Identifier
name: str
label: str
type: str
calculation_method: str
timestamp: str
model: Optional[str] = None
description: str = ""
sql: Union[str, int] = ""
expression: Union[str, int] = ""
time_grains: List[str] = field(default_factory=list)
dimensions: List[str] = field(default_factory=list)
model: Optional[str] = None
window: Optional[str] = None
filters: List[MetricFilter] = field(default_factory=list)
meta: Dict[str, Any] = field(default_factory=dict)
Expand All @@ -471,9 +471,13 @@ def validate(cls, data):
if "name" in data and " " in data["name"]:
raise ParsingException(f"Metrics name '{data['name']}' cannot contain spaces")

# TODO: Expressions _cannot_ have `model` properties
if data.get("model") is None and data.get("type") != "expression":
raise ValidationError("Non-expression metrics require a 'model' property")
if data.get("calculation_method") == "expression":
raise ValidationError(
"The metric calculation method expression has been deprecated and renamed to derived. Please update"
)

if data.get("model") is None and data.get("calculation_method") != "derived":
raise ValidationError("Non-derived metrics require a 'model' property")

if data.get("model") is not None and data.get("type") == "expression":
raise ValidationError("Expression metrics cannot have a 'model' property")
if data.get("model") is not None and data.get("calculation_method") == "derived":
raise ValidationError("Derived metrics cannot have a 'model' property")
2 changes: 1 addition & 1 deletion core/dbt/parser/schema_renderer.py
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ def should_render_keypath(self, keypath: Keypath) -> bool:
elif self._is_norender_key(keypath[0:]):
return False
elif self.key == "metrics":
if keypath[0] == "sql":
if keypath[0] == "expression":
return False
elif self._is_norender_key(keypath[0:]):
return False
Expand Down
8 changes: 4 additions & 4 deletions core/dbt/parser/schemas.py
Original file line number Diff line number Diff line change
Expand Up @@ -1046,8 +1046,8 @@ def parse_metric(self, unparsed: UnparsedMetric) -> ParsedMetric:
name=unparsed.name,
description=unparsed.description,
label=unparsed.label,
type=unparsed.type,
sql=str(unparsed.sql),
calculation_method=unparsed.calculation_method,
expression=str(unparsed.expression),
timestamp=unparsed.timestamp,
dimensions=unparsed.dimensions,
window=unparsed.window,
Expand All @@ -1068,8 +1068,8 @@ def parse_metric(self, unparsed: UnparsedMetric) -> ParsedMetric:
model_ref = "{{ " + parsed.model + " }}"
get_rendered(model_ref, ctx, parsed)

parsed.sql = get_rendered(
parsed.sql,
parsed.expression = get_rendered(
parsed.expression,
ctx,
node=parsed,
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@ metrics:
name: number_of_people
description: Total count of people
label: "Number of people"
type: count
sql: "*"
calculation_method: count
expression: "*"
timestamp: created_at
time_grains: [day, week, month]
dimensions:
Expand All @@ -20,8 +20,8 @@ metrics:
name: collective_tenure
description: Total number of years of team experience
label: "Collective tenure"
type: sum
sql: tenure
calculation_method: sum
expression: tenure
timestamp: created_at
time_grains: [day]
filters:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@ metrics:
label: New Customers
model: customers
description: "The number of paid customers who are using the product"
type: count
sql: user_id
calculation_method: count
expression: user_id
timestamp: signup_date
time_grains: [day, week, month]
dimensions:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@ metrics:
name: number_of_people
description: Total count of people
label: "Number of people"
type: count
sql: "*"
calculation_method: count
expression: "*"
timestamp: created_at
time_grains: [day, week, month]
dimensions:
Expand All @@ -20,8 +20,8 @@ metrics:
name: collective_tenure
description: Total number of years of team experience
label: "Collective tenure"
type: sum
sql: tenure
calculation_method: sum
expression: tenure
timestamp: created_at
time_grains: [day]
filters:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@ metrics:
name: number_of_people
description: Total count of people
label: "Number of people"
type: count
sql: "*"
calculation_method: count
expression: "*"
timestamp: created_at
time_grains: [day, week, month]
dimensions:
Expand All @@ -20,8 +20,8 @@ metrics:
name: collective_tenure
description: Total number of years of team experience
label: "Collective tenure"
type: sum
sql: tenure
calculation_method: sum
expression: tenure
timestamp: created_at
time_grains: [day]
filters:
Expand Down
6 changes: 3 additions & 3 deletions test/unit/test_contracts_graph_parsed.py
Original file line number Diff line number Diff line change
Expand Up @@ -2318,8 +2318,8 @@ def basic_parsed_metric_dict():
'name': 'new_customers',
'label': 'New Customers',
'model': 'ref("dim_customers")',
'type': 'count',
'sql': 'user_id',
'calculation_method': 'count',
'expression': 'user_id',
'timestamp': 'signup_date',
'time_grains': ['day', 'week', 'month'],
'dimensions': ['plan', 'country'],
Expand Down Expand Up @@ -2355,7 +2355,7 @@ def basic_parsed_metric_dict():
def basic_parsed_metric_object():
return ParsedMetric(
name='my_metric',
type='count',
calculation_method='count',
fqn=['test', 'metrics', 'my_metric'],
unique_id='metric.test.my_metric',
package_name='test',
Expand Down
32 changes: 16 additions & 16 deletions test/unit/test_contracts_graph_unparsed.py
Original file line number Diff line number Diff line change
Expand Up @@ -685,8 +685,8 @@ def get_ok_dict(self):
'label': 'New Customers',
'model': 'ref("dim_customers")',
'description': 'New customers',
'type': 'count',
'sql': 'user_id',
'calculation_method': 'count',
'expression': 'user_id',
'timestamp': 'signup_date',
'time_grains': ['day', 'week', 'month'],
'dimensions': ['plan', 'country'],
Expand All @@ -704,15 +704,15 @@ def get_ok_dict(self):
},
}

def get_ok_expression_dict(self):
def get_ok_derived_dict(self):
return {
'name': 'arpc',
'label': 'revenue per customer',
'description': '',
'type': 'expression',
'sql': "{{ metric('revenue') }} / {{ metric('customers') }}",
'timestamp': "signup_date",
'calculation_method': 'derived',
'expression': "{{ metric('revenue') }} / {{ metric('customers') }}",
'time_grains': ['day', 'week', 'month'],
'timestamp': 'signup_date',
'dimensions': [],
'filters': [],
'window': '',
Expand All @@ -728,8 +728,8 @@ def test_ok(self):
label='New Customers',
model='ref("dim_customers")',
description="New customers",
type='count',
sql="user_id",
calculation_method='count',
expression="user_id",
timestamp="signup_date",
time_grains=['day', 'week', 'month'],
dimensions=['plan', 'country'],
Expand All @@ -746,34 +746,34 @@ def test_ok(self):
pickle.loads(pickle.dumps(metric))

def test_ok_metric_no_model(self):
# Expression metrics do not have model properties
# Derived metrics do not have model properties
metric = self.ContractType(
name='arpc',
label='revenue per customer',
model=None,
description="",
type='expression',
sql="{{ metric('revenue') }} / {{ metric('customers') }}",
calculation_method='derived',
expression="{{ metric('revenue') }} / {{ metric('customers') }}",
timestamp="signup_date",
time_grains=['day', 'week', 'month'],
dimensions=[],
window='',
meta={'is_okr': True},
)
dct = self.get_ok_expression_dict()
dct = self.get_ok_derived_dict()
self.assert_symmetric(metric, dct)
pickle.loads(pickle.dumps(metric))

def test_bad_metric_no_type(self):
def test_bad_metric_no_calculation_method(self):
tst = self.get_ok_dict()
del tst['type']
del tst['calculation_method']
self.assert_fails_validation(tst)

def test_bad_metric_no_model(self):
tst = self.get_ok_dict()
# Metrics with type='expression' do not have model props
# Metrics with calculation_type='derived' do not have model props
tst['model'] = None
tst['type'] = 'sum'
tst['calculation_method'] = 'sum'
self.assert_fails_validation(tst)

def test_bad_filter_missing_things(self):
Expand Down
4 changes: 2 additions & 2 deletions test/unit/test_graph_selector_methods.py
Original file line number Diff line number Diff line change
Expand Up @@ -370,8 +370,8 @@ def make_metric(pkg, name, path=None):
label='New Customers',
model='ref("multi")',
description="New customers",
type='count',
sql="user_id",
calculation_method='count',
expression="user_id",
timestamp="signup_date",
time_grains=['day', 'week', 'month'],
dimensions=['plan', 'country'],
Expand Down
4 changes: 2 additions & 2 deletions test/unit/test_manifest.py
Original file line number Diff line number Diff line change
Expand Up @@ -103,8 +103,8 @@ def setUp(self):
label='New Customers',
model='ref("multi")',
description="New customers",
type='count',
sql="user_id",
calculation_method='count',
expression="user_id",
timestamp="signup_date",
time_grains=['day', 'week', 'month'],
dimensions=['plan', 'country'],
Expand Down
6 changes: 3 additions & 3 deletions test/unit/test_yaml_renderer.py
Original file line number Diff line number Diff line change
Expand Up @@ -110,15 +110,15 @@ def test__metrics(self):
dct = {
"name": "my_source",
"description": "{{ docs('my_doc') }}",
"sql": "select {{ var('my_var') }} from my_table",
"expression": "select {{ var('my_var') }} from my_table",
"time_grains": "{{ my_time_grains }}",
}
# We expect the sql and description will not be rendered, but
# We expect the expression and description will not be rendered, but
# other fields will be
expected = {
"name": "my_source",
"description": "{{ docs('my_doc') }}",
"sql": "select {{ var('my_var') }} from my_table",
"expression": "select {{ var('my_var') }} from my_table",
"time_grains": "[day]"
}
dct = renderer.render_data(dct)
Expand Down
Loading

0 comments on commit cfece2c

Please sign in to comment.