From 10062791a380ce45012a5d49f9ad0f8cd63b878e Mon Sep 17 00:00:00 2001 From: Callum McCann Date: Wed, 3 Aug 2022 10:39:54 -0400 Subject: [PATCH 1/6] Adding helper functions --- .gitignore | 4 ++ core/dbt/contracts/graph/metrics.py | 69 ++++++++++++++++++----------- 2 files changed, 48 insertions(+), 25 deletions(-) diff --git a/.gitignore b/.gitignore index 7216a85b2da..6ed4301d235 100644 --- a/.gitignore +++ b/.gitignore @@ -95,3 +95,7 @@ venv/ # vscode .vscode/ + +# poetry +pyproject.toml +poetry.lock \ No newline at end of file diff --git a/core/dbt/contracts/graph/metrics.py b/core/dbt/contracts/graph/metrics.py index 46d9b8334ff..f161d13a3f3 100644 --- a/core/dbt/contracts/graph/metrics.py +++ b/core/dbt/contracts/graph/metrics.py @@ -38,33 +38,52 @@ def parent_metrics(cls, metric_node, manifest): if node and node.resource_type == NodeType.Metric: yield from cls.parent_metrics(node, manifest) - def parent_models(self): + @classmethod + def parent_metrics_names(cls, metric_node, manifest): + yield metric_node.name + + 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: + yield from cls.parent_metrics_names(node, manifest) + + @classmethod + def reverse_dag_parsing(cls, metric_node, manifest, metric_depth_count): + if metric_node.type == "expression": + yield {metric_node.unique_id: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": + yield from cls.reverse_dag_parsing(node, manifest, metric_depth_count) + + def full_metric_dependency(self): + to_return = list(set(self.parent_metrics_names(self.node, self.manifest))) + return to_return + + def base_metric_dependency(self): in_scope_metrics = list(self.parent_metrics(self.node, self.manifest)) - to_return = { - "base": [], - "derived": [], - } + to_return =[] for metric in in_scope_metrics: - if metric.type == "expression": - to_return["derived"].append( - {"metric_source": None, "metric": metric, "is_derived": True} - ) - else: - for node_unique_id in metric.depends_on.nodes: - node = self.manifest.nodes.get(node_unique_id) - if node and node.resource_type in NodeType.refable(): - to_return["base"].append( - { - "metric_relation_node": node, - "metric_relation": self.Relation.create( - database=node.database, - schema=node.schema, - identifier=node.alias, - ), - "metric": metric, - "is_derived": False, - } - ) + if metric.type != "expression" and metric.name not in to_return: + to_return.append(metric.name) + + return to_return + def derived_metric_dependency(self): + in_scope_metrics = list(self.parent_metrics(self.node, self.manifest)) + + to_return =[] + for metric in in_scope_metrics: + if metric.type == "expression" and metric.name not in to_return: + to_return.append(metric.name) + return to_return + + def derived_metric_dependency_depth(self): + metric_depth_count = 1 + to_return = list(self.reverse_dag_parsing(self.node, self.manifest,metric_depth_count)) + + return to_return \ No newline at end of file From 087d3ec9462e559d886dbf53a7bbd65821cb1e8b Mon Sep 17 00:00:00 2001 From: Callum McCann Date: Wed, 3 Aug 2022 17:27:12 -0400 Subject: [PATCH 2/6] adding bad test --- .../metrics/test_metric_helper_functions.py | 94 +++++++++++++++++++ 1 file changed, 94 insertions(+) create mode 100644 tests/functional/metrics/test_metric_helper_functions.py diff --git a/tests/functional/metrics/test_metric_helper_functions.py b/tests/functional/metrics/test_metric_helper_functions.py new file mode 100644 index 00000000000..575cd84a5ab --- /dev/null +++ b/tests/functional/metrics/test_metric_helper_functions.py @@ -0,0 +1,94 @@ +import pytest + +from dbt.tests.util import run_dbt, get_manifest +from dbt.exceptions import ParsingException + +from tests.functional.metrics.fixture_metrics import mock_purchase_data_csv + + +metrics__yml = """ +version: 2 + +metrics: + + - name: number_of_people + label: "Number of people" + description: Total count of people + model: "ref('people')" + type: count + sql: "*" + timestamp: created_at + time_grains: [day, week, month] + dimensions: + - favorite_color + - loves_dbt + meta: + my_meta: 'testing' + + - name: collective_tenure + label: "Collective tenure" + description: Total number of years of team experience + model: "ref('people')" + type: sum + sql: tenure + timestamp: created_at + time_grains: [day, week, month] + filters: + - field: loves_dbt + operator: 'is' + value: 'true' + + - name: average_tenure + label: "Average tenure" + description: "The average tenure per person" + type: expression + sql: "{{metric('collective_tenure')}} / {{metric('number_of_people')}} " + timestamp: created_at + time_grains: [day, week, month] + + - name: average_tenure_plus_one + label: "Average tenure" + description: "The average tenure per person" + type: expression + sql: "{{metric('average_tenure')}} + 1 " + timestamp: created_at + time_grains: [day, week, month] +""" + +models__people_sql = """ +select 1 as id, 'Drew' as first_name, 'Banin' as last_name, 'yellow' as favorite_color, true as loves_dbt, 5 as tenure, current_timestamp as created_at +union all +select 1 as id, 'Jeremy' as first_name, 'Cohen' as last_name, 'indigo' as favorite_color, true as loves_dbt, 4 as tenure, current_timestamp as created_at +union all +select 1 as id, 'Callum' as first_name, 'McCann' as last_name, 'emerald' as favorite_color, true as loves_dbt, 0 as tenure, current_timestamp as created_at +""" + +class TestMetricHelperFunctions: + @pytest.fixture(scope="class") + def models(self): + return { + "metrics.yml": metrics__yml, + "people.sql": models__people_sql, + } + + def test_expression_metric( + self, + project, + ): + + # initial parse + results = run_dbt(["compile"]) + + # make sure all the metrics are in the manifest + manifest = get_manifest(project.project_root) + testing_metric = manifest.metrics["metric.test.average_tenure_plus_one"] + full_metric_dependency = testing_metric.full_metric_dependency() + + expected_full_metric_dependency = [ + "average_tenure_plus_one", + "average_tenure", + "collective_tenure", + "number_of_people" + ] + + assert full_metric_dependency == expected_full_metric_dependency From 24eff73d443a08029fcd3ff86534536a84c2ff37 Mon Sep 17 00:00:00 2001 From: Chenyu Li Date: Wed, 3 Aug 2022 15:45:09 -0700 Subject: [PATCH 3/6] use ResolvedMetricReference --- .../metrics/test_metric_helper_functions.py | 22 ++++++++----------- 1 file changed, 9 insertions(+), 13 deletions(-) diff --git a/tests/functional/metrics/test_metric_helper_functions.py b/tests/functional/metrics/test_metric_helper_functions.py index 575cd84a5ab..6203f184fe4 100644 --- a/tests/functional/metrics/test_metric_helper_functions.py +++ b/tests/functional/metrics/test_metric_helper_functions.py @@ -1,10 +1,7 @@ import pytest from dbt.tests.util import run_dbt, get_manifest -from dbt.exceptions import ParsingException - -from tests.functional.metrics.fixture_metrics import mock_purchase_data_csv - +from dbt.contracts.graph.metrics import ResolvedMetricReference metrics__yml = """ version: 2 @@ -63,6 +60,7 @@ select 1 as id, 'Callum' as first_name, 'McCann' as last_name, 'emerald' as favorite_color, true as loves_dbt, 0 as tenure, current_timestamp as created_at """ + class TestMetricHelperFunctions: @pytest.fixture(scope="class") def models(self): @@ -77,18 +75,16 @@ def test_expression_metric( ): # initial parse - results = run_dbt(["compile"]) + run_dbt(["compile"]) # make sure all the metrics are in the manifest manifest = get_manifest(project.project_root) - testing_metric = manifest.metrics["metric.test.average_tenure_plus_one"] - full_metric_dependency = testing_metric.full_metric_dependency() + parsed_metric = manifest.metrics["metric.test.average_tenure_plus_one"] + testing_metric = ResolvedMetricReference(parsed_metric, manifest, None) + full_metric_dependency = set(testing_metric.full_metric_dependency()) - expected_full_metric_dependency = [ - "average_tenure_plus_one", - "average_tenure", - "collective_tenure", - "number_of_people" - ] + expected_full_metric_dependency = set( + ["average_tenure_plus_one", "average_tenure", "collective_tenure", "number_of_people"] + ) assert full_metric_dependency == expected_full_metric_dependency From 2910f7bc0390c38dadca11bb9a44d52bd129cdfe Mon Sep 17 00:00:00 2001 From: Callum McCann Date: Thu, 4 Aug 2022 08:31:15 -0400 Subject: [PATCH 4/6] adding pytest --- core/dbt/contracts/graph/metrics.py | 2 +- .../metrics/test_metric_helper_functions.py | 22 +++++++++++++++++-- 2 files changed, 21 insertions(+), 3 deletions(-) diff --git a/core/dbt/contracts/graph/metrics.py b/core/dbt/contracts/graph/metrics.py index f161d13a3f3..ca93829f115 100644 --- a/core/dbt/contracts/graph/metrics.py +++ b/core/dbt/contracts/graph/metrics.py @@ -50,7 +50,7 @@ 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": - yield {metric_node.unique_id:metric_depth_count} + yield {metric_node.name:metric_depth_count} metric_depth_count = metric_depth_count+1 for parent_unique_id in metric_node.depends_on.nodes: diff --git a/tests/functional/metrics/test_metric_helper_functions.py b/tests/functional/metrics/test_metric_helper_functions.py index 6203f184fe4..55c0f250803 100644 --- a/tests/functional/metrics/test_metric_helper_functions.py +++ b/tests/functional/metrics/test_metric_helper_functions.py @@ -81,10 +81,28 @@ def test_expression_metric( manifest = get_manifest(project.project_root) parsed_metric = manifest.metrics["metric.test.average_tenure_plus_one"] testing_metric = ResolvedMetricReference(parsed_metric, manifest, None) + + full_metric_dependency = set(testing_metric.full_metric_dependency()) - expected_full_metric_dependency = set( ["average_tenure_plus_one", "average_tenure", "collective_tenure", "number_of_people"] ) - assert full_metric_dependency == expected_full_metric_dependency + + base_metric_dependency = set(testing_metric.base_metric_dependency()) + expected_base_metric_dependency = set( + ["collective_tenure", "number_of_people"] + ) + assert base_metric_dependency == expected_base_metric_dependency + + derived_metric_dependency = set(testing_metric.derived_metric_dependency()) + expected_derived_metric_dependency = set( + ["average_tenure_plus_one", "average_tenure"] + ) + assert derived_metric_dependency == expected_derived_metric_dependency + + derived_metric_dependency_depth = list(testing_metric.derived_metric_dependency_depth()) + expected_derived_metric_dependency_depth = list( + [{"average_tenure_plus_one":1}, {"average_tenure":2}] + ) + assert derived_metric_dependency_depth == expected_derived_metric_dependency_depth From 9ef38dbbb7d05e3fcccf56f609130e7d3263a4cc Mon Sep 17 00:00:00 2001 From: Callum McCann Date: Thu, 4 Aug 2022 12:09:45 -0400 Subject: [PATCH 5/6] adding changie updates --- .changes/unreleased/Features-20220804-120936.yaml | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 .changes/unreleased/Features-20220804-120936.yaml diff --git a/.changes/unreleased/Features-20220804-120936.yaml b/.changes/unreleased/Features-20220804-120936.yaml new file mode 100644 index 00000000000..5b1c46388be --- /dev/null +++ b/.changes/unreleased/Features-20220804-120936.yaml @@ -0,0 +1,7 @@ +kind: Features +body: Adding ResolvedMetricReference helper functions and tests +time: 2022-08-04T12:09:36.202919-04:00 +custom: + Author: callum-mcdata + Issue: "5567" + PR: "5607" From 13e8941b01261e232e67cee1768859f088fd6a16 Mon Sep 17 00:00:00 2001 From: Callum McCann Date: Thu, 4 Aug 2022 12:40:31 -0400 Subject: [PATCH 6/6] adding pre-commit changes --- .gitignore | 2 +- core/dbt/contracts/graph/metrics.py | 18 +++++++++--------- .../metrics/test_metric_helper_functions.py | 13 ++++--------- 3 files changed, 14 insertions(+), 19 deletions(-) diff --git a/.gitignore b/.gitignore index 6ed4301d235..7f8c5cf33d1 100644 --- a/.gitignore +++ b/.gitignore @@ -98,4 +98,4 @@ venv/ # poetry pyproject.toml -poetry.lock \ No newline at end of file +poetry.lock diff --git a/core/dbt/contracts/graph/metrics.py b/core/dbt/contracts/graph/metrics.py index ca93829f115..3ab307bfe54 100644 --- a/core/dbt/contracts/graph/metrics.py +++ b/core/dbt/contracts/graph/metrics.py @@ -50,14 +50,14 @@ 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": - yield {metric_node.name:metric_depth_count} - metric_depth_count = metric_depth_count+1 + 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": yield from cls.reverse_dag_parsing(node, manifest, metric_depth_count) - + def full_metric_dependency(self): to_return = list(set(self.parent_metrics_names(self.node, self.manifest))) return to_return @@ -65,25 +65,25 @@ def full_metric_dependency(self): def base_metric_dependency(self): in_scope_metrics = list(self.parent_metrics(self.node, self.manifest)) - to_return =[] + to_return = [] for metric in in_scope_metrics: if metric.type != "expression" and metric.name not in to_return: to_return.append(metric.name) - + return to_return def derived_metric_dependency(self): in_scope_metrics = list(self.parent_metrics(self.node, self.manifest)) - to_return =[] + to_return = [] for metric in in_scope_metrics: if metric.type == "expression" and metric.name not in to_return: to_return.append(metric.name) - + return to_return def derived_metric_dependency_depth(self): metric_depth_count = 1 - to_return = list(self.reverse_dag_parsing(self.node, self.manifest,metric_depth_count)) + to_return = list(self.reverse_dag_parsing(self.node, self.manifest, metric_depth_count)) - return to_return \ No newline at end of file + return to_return diff --git a/tests/functional/metrics/test_metric_helper_functions.py b/tests/functional/metrics/test_metric_helper_functions.py index 55c0f250803..6c60ea7a985 100644 --- a/tests/functional/metrics/test_metric_helper_functions.py +++ b/tests/functional/metrics/test_metric_helper_functions.py @@ -81,8 +81,7 @@ def test_expression_metric( manifest = get_manifest(project.project_root) parsed_metric = manifest.metrics["metric.test.average_tenure_plus_one"] testing_metric = ResolvedMetricReference(parsed_metric, manifest, None) - - + full_metric_dependency = set(testing_metric.full_metric_dependency()) expected_full_metric_dependency = set( ["average_tenure_plus_one", "average_tenure", "collective_tenure", "number_of_people"] @@ -90,19 +89,15 @@ def test_expression_metric( assert full_metric_dependency == expected_full_metric_dependency base_metric_dependency = set(testing_metric.base_metric_dependency()) - expected_base_metric_dependency = set( - ["collective_tenure", "number_of_people"] - ) + expected_base_metric_dependency = set(["collective_tenure", "number_of_people"]) assert base_metric_dependency == expected_base_metric_dependency derived_metric_dependency = set(testing_metric.derived_metric_dependency()) - expected_derived_metric_dependency = set( - ["average_tenure_plus_one", "average_tenure"] - ) + expected_derived_metric_dependency = set(["average_tenure_plus_one", "average_tenure"]) assert derived_metric_dependency == expected_derived_metric_dependency derived_metric_dependency_depth = list(testing_metric.derived_metric_dependency_depth()) expected_derived_metric_dependency_depth = list( - [{"average_tenure_plus_one":1}, {"average_tenure":2}] + [{"average_tenure_plus_one": 1}, {"average_tenure": 2}] ) assert derived_metric_dependency_depth == expected_derived_metric_dependency_depth