From d43c070007b823588e7a170c6c37495ce319b011 Mon Sep 17 00:00:00 2001 From: Callum McCann <101437052+callum-mcdata@users.noreply.github.com> Date: Thu, 5 Jan 2023 14:49:55 -0600 Subject: [PATCH] Making Metric Timestamps Optional (#6402) * changing to optional * adding tests * tests and changie * pre-commit cleaning * formatting fixes * pre-commit update --- .../unreleased/Features-20221207-091722.yaml | 7 ++ core/dbt/contracts/graph/nodes.py | 2 +- core/dbt/contracts/graph/unparsed.py | 12 +++- tests/functional/metrics/fixtures.py | 55 +++++++++++++++ tests/functional/metrics/test_metrics.py | 69 +++++++++++++++++++ 5 files changed, 143 insertions(+), 2 deletions(-) create mode 100644 .changes/unreleased/Features-20221207-091722.yaml diff --git a/.changes/unreleased/Features-20221207-091722.yaml b/.changes/unreleased/Features-20221207-091722.yaml new file mode 100644 index 00000000000..16845f3663e --- /dev/null +++ b/.changes/unreleased/Features-20221207-091722.yaml @@ -0,0 +1,7 @@ +kind: Features +body: Making timestamp optional for metrics +time: 2022-12-07T09:17:22.571877-06:00 +custom: + Author: callum-mcdata + Issue: "6398" + PR: "9400" diff --git a/core/dbt/contracts/graph/nodes.py b/core/dbt/contracts/graph/nodes.py index 033318a34c1..730e2286ccd 100644 --- a/core/dbt/contracts/graph/nodes.py +++ b/core/dbt/contracts/graph/nodes.py @@ -976,12 +976,12 @@ class Metric(GraphNode): description: str label: str calculation_method: str - timestamp: str expression: str filters: List[MetricFilter] time_grains: List[str] dimensions: List[str] 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 diff --git a/core/dbt/contracts/graph/unparsed.py b/core/dbt/contracts/graph/unparsed.py index 453dc883d7b..ba2e48c7c9c 100644 --- a/core/dbt/contracts/graph/unparsed.py +++ b/core/dbt/contracts/graph/unparsed.py @@ -484,9 +484,9 @@ class UnparsedMetric(dbtClassMixin, Replaceable): name: str label: str calculation_method: str - timestamp: str expression: str description: str = "" + timestamp: Optional[str] = None time_grains: List[str] = field(default_factory=list) dimensions: List[str] = field(default_factory=list) window: Optional[MetricTime] = None @@ -518,6 +518,16 @@ def validate(cls, data): f"The metric name '{data['name']}' is invalid. It {', '.join(e for e in errors)}" ) + if data.get("timestamp") is None and data.get("time_grains") is not None: + raise ValidationError( + f"The metric '{data['name']} has time_grains defined but is missing a timestamp dimension." + ) + + if data.get("timestamp") is None and data.get("window") is not None: + raise ValidationError( + f"The metric '{data['name']} has a window defined but is missing a timestamp dimension." + ) + if data.get("model") is None and data.get("calculation_method") != "derived": raise ValidationError("Non-derived metrics require a 'model' property") diff --git a/tests/functional/metrics/fixtures.py b/tests/functional/metrics/fixtures.py index e191f609977..8a03cb0d7fa 100644 --- a/tests/functional/metrics/fixtures.py +++ b/tests/functional/metrics/fixtures.py @@ -642,3 +642,58 @@ meta: my_meta: 'testing' """ + +metric_without_timestamp_or_timegrains_yml = """ +version: 2 + +metrics: + - name: number_of_people + label: "Number of people" + description: Total count of people + model: "ref('people')" + calculation_method: count + expression: "*" + dimensions: + - favorite_color + - loves_dbt + meta: + my_meta: 'testing' +""" + +invalid_metric_without_timestamp_with_time_grains_yml = """ +version: 2 + +metrics: + - name: number_of_people + label: "Number of people" + description: Total count of people + model: "ref('people')" + time_grains: [day, week, month] + calculation_method: count + expression: "*" + dimensions: + - favorite_color + - loves_dbt + meta: + my_meta: 'testing' +""" + +invalid_metric_without_timestamp_with_window_yml = """ +version: 2 + +metrics: + - name: number_of_people + label: "Number of people" + description: Total count of people + model: "ref('people')" + window: + count: 14 + period: day + calculation_method: count + expression: "*" + dimensions: + - favorite_color + - loves_dbt + meta: + my_meta: 'testing' +""" diff --git a/tests/functional/metrics/test_metrics.py b/tests/functional/metrics/test_metrics.py index 37446589cd2..de8c022f3d3 100644 --- a/tests/functional/metrics/test_metrics.py +++ b/tests/functional/metrics/test_metrics.py @@ -3,6 +3,7 @@ from dbt.tests.util import run_dbt, get_manifest from dbt.exceptions import ParsingException + from tests.functional.metrics.fixtures import ( mock_purchase_data_csv, models_people_sql, @@ -18,6 +19,9 @@ invalid_derived_metric_contains_model_yml, derived_metric_yml, derived_metric_old_attr_names_yml, + metric_without_timestamp_or_timegrains_yml, + invalid_metric_without_timestamp_with_time_grains_yml, + invalid_metric_without_timestamp_with_window_yml ) @@ -46,6 +50,33 @@ def test_simple_metric( assert metric_ids == expected_metric_ids +class TestSimpleMetricsNoTimestamp: + @pytest.fixture(scope="class") + def models(self): + return { + "people_metrics.yml": metric_without_timestamp_or_timegrains_yml, + "people.sql": models_people_sql, + } + + def test_simple_metric_no_timestamp( + self, + project, + ): + # initial run + results = run_dbt(["run"]) + assert len(results) == 1 + manifest = get_manifest(project.project_root) + metric_ids = list(manifest.metrics.keys()) + expected_metric_ids = [ + "metric.test.number_of_people", + ] + assert metric_ids == expected_metric_ids + + # make sure the 'expression' metric depends on the two upstream metrics + metric_test = manifest.metrics["metric.test.number_of_people"] + assert metric_test.timestamp is None + + class TestInvalidRefMetrics: @pytest.fixture(scope="class") def models(self): @@ -253,3 +284,41 @@ def models(self): "derived_metric.yml": derived_metric_old_attr_names_yml, "downstream_model.sql": downstream_model_sql, } + + +class TestInvalidTimestampTimeGrainsMetrics: + @pytest.fixture(scope="class") + def models(self): + return { + "people_metrics.yml": invalid_metric_without_timestamp_with_time_grains_yml, + "people.sql": models_people_sql, + } + + # Tests that we get a ParsingException with an invalid metric definition. + # This metric definition is missing timestamp but HAS a time_grains property + def test_simple_metric( + self, + project, + ): + # initial run + with pytest.raises(ParsingException): + run_dbt(["run"]) + + +class TestInvalidTimestampWindowMetrics: + @pytest.fixture(scope="class") + def models(self): + return { + "people_metrics.yml": invalid_metric_without_timestamp_with_window_yml, + "people.sql": models_people_sql, + } + + # Tests that we get a ParsingException with an invalid metric definition. + # This metric definition is missing timestamp but HAS a window property + def test_simple_metric( + self, + project, + ): + # initial run + with pytest.raises(ParsingException): + run_dbt(["run"])