From c97eb5922fea9ee4df36a62d35f88f64e0f6e81d Mon Sep 17 00:00:00 2001 From: Vitor Avila Date: Wed, 14 Aug 2024 15:29:10 -0300 Subject: [PATCH 1/3] feat(dbt sync): Support metric key remapping --- src/preset_cli/api/clients/dbt.py | 3 + .../cli/superset/sync/dbt/command.py | 4 + src/preset_cli/cli/superset/sync/dbt/lib.py | 13 ++ .../cli/superset/sync/dbt/metrics.py | 45 ++++--- tests/cli/superset/sync/dbt/command_test.py | 14 +- tests/cli/superset/sync/dbt/lib_test.py | 30 +++++ tests/cli/superset/sync/dbt/metrics_test.py | 122 ++++++++++++++++-- 7 files changed, 193 insertions(+), 38 deletions(-) diff --git a/src/preset_cli/api/clients/dbt.py b/src/preset_cli/api/clients/dbt.py index fcd14ed1..4bd2f0f8 100644 --- a/src/preset_cli/api/clients/dbt.py +++ b/src/preset_cli/api/clients/dbt.py @@ -612,6 +612,8 @@ class MFMetricSchema(PostelSchema): name = fields.String() description = fields.String() type = PostelEnumField(MFMetricType) + meta = fields.Raw() + label = fields.String() class MFSQLEngine(str, Enum): @@ -860,6 +862,7 @@ def get_sl_metrics(self, environment_id: int) -> List[MFMetricSchema]: name description type + label } } """ diff --git a/src/preset_cli/cli/superset/sync/dbt/command.py b/src/preset_cli/cli/superset/sync/dbt/command.py index 1f56abe8..11cfc5b4 100644 --- a/src/preset_cli/cli/superset/sync/dbt/command.py +++ b/src/preset_cli/cli/superset/sync/dbt/command.py @@ -416,11 +416,13 @@ def get_sl_metric( return mf_metric_schema.load( { "name": metric["name"], + "label": metric["label"], "type": metric["type"], "description": metric["description"], "sql": sql, "dialect": dialect.value, "model": model["unique_id"], + "meta": metric.get("meta", metric.get("config", {}).get("meta", {})), }, ) @@ -452,9 +454,11 @@ def fetch_sl_metrics( "name": metric["name"], "type": metric["type"], "description": metric["description"], + "label": metric["label"], "sql": sql, "dialect": dialect.value, "model": model["unique_id"], + # TODO (Vitor-Avila): Pull meta from ``config.meta`` (supported in versionless) }, ), ) diff --git a/src/preset_cli/cli/superset/sync/dbt/lib.py b/src/preset_cli/cli/superset/sync/dbt/lib.py index 27c63c8b..223c86a4 100644 --- a/src/preset_cli/cli/superset/sync/dbt/lib.py +++ b/src/preset_cli/cli/superset/sync/dbt/lib.py @@ -526,3 +526,16 @@ def get_og_metric_from_config( metric_config["dialect"] = dialect return metric_schema.load(metric_config) + + +def parse_metric_meta(metric_meta: Dict[str, Any]) -> Dict[str, Any]: + """ + Parses the metric's meta information. + """ + kwargs = metric_meta.pop("superset", {}) + metric_name_override = kwargs.pop("metric_name", None) + return { + "meta": metric_meta, + "kwargs": kwargs, + "metric_name_override": metric_name_override, + } diff --git a/src/preset_cli/cli/superset/sync/dbt/metrics.py b/src/preset_cli/cli/superset/sync/dbt/metrics.py index 0c8a7a00..cc496809 100644 --- a/src/preset_cli/cli/superset/sync/dbt/metrics.py +++ b/src/preset_cli/cli/superset/sync/dbt/metrics.py @@ -36,6 +36,7 @@ ) from preset_cli.api.clients.superset import SupersetMetricDefinition from preset_cli.cli.superset.sync.dbt.exposures import ModelKey +from preset_cli.cli.superset.sync.dbt.lib import parse_metric_meta _logger = logging.getLogger(__name__) @@ -197,17 +198,19 @@ def get_metric_definition( """ metric_map = {metric["name"]: metric for metric in metrics} metric = metric_map[metric_name] - meta = metric.get("meta", {}) - kwargs = meta.pop("superset", {}) + metric_meta = parse_metric_meta( + metric.get("meta", metric.get("config", {}).get("meta", {})), + ) + final_metric_name = metric_meta["metric_name_override"] or metric_name return { "expression": get_metric_expression(metric_name, metric_map), - "metric_name": metric_name, + "metric_name": final_metric_name, "metric_type": (metric.get("type") or metric.get("calculation_method")), - "verbose_name": metric.get("label", metric_name), + "verbose_name": metric.get("label", final_metric_name), "description": metric.get("description", ""), - "extra": json.dumps(meta), - **kwargs, # type: ignore + "extra": json.dumps(metric_meta["meta"]), + **metric_meta["kwargs"], # type: ignore } @@ -254,13 +257,7 @@ def get_superset_metrics_per_model( superset_metrics[model].append(metric_definition) for sl_metric in sl_metrics or []: - metric_definition = convert_metric_flow_to_superset( - sl_metric["name"], - sl_metric["description"], - sl_metric["type"], - sl_metric["sql"], - sl_metric["dialect"], - ) + metric_definition = convert_metric_flow_to_superset(sl_metric) model = sl_metric["model"] superset_metrics[model].append(metric_definition) @@ -337,11 +334,7 @@ def convert_query_to_projection(sql: str, dialect: MFSQLEngine) -> str: def convert_metric_flow_to_superset( - name: str, - description: str, - metric_type: str, - sql: str, - dialect: MFSQLEngine, + sl_metric: MFMetricWithSQLSchema, ) -> SupersetMetricDefinition: """ Convert a MetricFlow metric to a Superset metric. @@ -368,12 +361,18 @@ def convert_metric_flow_to_superset( SUM(CASE WHEN order_total > 20 THEN 1 END) """ + metric_meta = parse_metric_meta(sl_metric.get("meta", {})) return { - "expression": convert_query_to_projection(sql, dialect), - "metric_name": name, - "metric_type": metric_type, - "verbose_name": name, - "description": description, + "expression": convert_query_to_projection( + sl_metric["sql"], + sl_metric["dialect"], + ), + "metric_name": metric_meta["metric_name_override"] or sl_metric["name"], + "metric_type": sl_metric["type"], + "verbose_name": sl_metric["label"], + "description": sl_metric["description"], + "extra": json.dumps(metric_meta["meta"]), + **metric_meta["kwargs"], # type: ignore } diff --git a/tests/cli/superset/sync/dbt/command_test.py b/tests/cli/superset/sync/dbt/command_test.py index b9e5d35f..c02d21e1 100644 --- a/tests/cli/superset/sync/dbt/command_test.py +++ b/tests/cli/superset/sync/dbt/command_test.py @@ -190,9 +190,14 @@ ] dbt_metricflow_metrics = [ - {"name": "a", "type": "Simple", "description": "The simplest metric"}, - {"name": "b", "type": "derived", "description": "Too complex for Superset"}, - {"name": "c", "type": "derived", "description": "Multiple models"}, + {"name": "a", "type": "Simple", "description": "The simplest metric", "label": "A"}, + { + "name": "b", + "type": "derived", + "description": "Too complex for Superset", + "label": "B", + }, + {"name": "c", "type": "derived", "description": "Multiple models", "label": "C"}, ] @@ -3065,7 +3070,8 @@ def test_dbt_cloud(mocker: MockerFixture) -> None: "expression": "COUNT(*)", "metric_name": "a", "metric_type": "Simple", - "verbose_name": "a", + "verbose_name": "A", + "extra": "{}", }, ], }, diff --git a/tests/cli/superset/sync/dbt/lib_test.py b/tests/cli/superset/sync/dbt/lib_test.py index fd9f265a..2f783175 100644 --- a/tests/cli/superset/sync/dbt/lib_test.py +++ b/tests/cli/superset/sync/dbt/lib_test.py @@ -25,6 +25,7 @@ get_og_metric_from_config, list_failed_models, load_profiles, + parse_metric_meta, ) from preset_cli.exceptions import CLIError @@ -815,3 +816,32 @@ def test_get_og_metric_from_config_ready_metric() -> None: "time_grains": [], "model_unique_id": None, } + + +def test_parse_metric_meta() -> None: + """ + Test the ``parse_metric_meta`` helper. + """ + assert parse_metric_meta( + { + "superset": { + "d3format": ",.2f", + "metric_name": "revenue_metric", + }, + "airflow": "other_id", + }, + ) == { + "meta": { + "airflow": "other_id", + }, + "kwargs": { + "d3format": ",.2f", + }, + "metric_name_override": "revenue_metric", + } + + assert parse_metric_meta({}) == { + "meta": {}, + "kwargs": {}, + "metric_name_override": None, + } diff --git a/tests/cli/superset/sync/dbt/metrics_test.py b/tests/cli/superset/sync/dbt/metrics_test.py index 34bfc771..22746703 100644 --- a/tests/cli/superset/sync/dbt/metrics_test.py +++ b/tests/cli/superset/sync/dbt/metrics_test.py @@ -771,21 +771,59 @@ def test_convert_metric_flow_to_superset(mocker: MockerFixture) -> None: """ mocker.patch( "preset_cli.cli.superset.sync.dbt.metrics.convert_query_to_projection", - return_value="SUM(order_total)", + side_effect=["SUM(order_total)", "SUM(price_each)"], + ) + mf_metric_schema = MFMetricWithSQLSchema() + semantic_metric = mf_metric_schema.load( + { + "name": "sales", + "description": "All sales", + "label": "Sales", + "type": "SIMPLE", + "sql": "SELECT SUM(order_total) AS order_total FROM orders", + "dialect": MFSQLEngine.BIGQUERY, + "meta": { + "superset": { + "d3format": "0.2f", + }, + }, + }, ) - assert convert_metric_flow_to_superset( - name="sales", - description="All sales", - metric_type="SIMPLE", - sql="SELECT SUM(order_total) AS order_total FROM orders", - dialect=MFSQLEngine.BIGQUERY, - ) == { + assert convert_metric_flow_to_superset(semantic_metric) == { "expression": "SUM(order_total)", "metric_name": "sales", "metric_type": "SIMPLE", - "verbose_name": "sales", + "verbose_name": "Sales", "description": "All sales", + "d3format": "0.2f", + "extra": "{}", + } + + # Metric key override + other_semantic_metric = mf_metric_schema.load( + { + "name": "revenue", + "description": "Total revenue in the period", + "label": "Total Revenue", + "type": "SIMPLE", + "sql": "SELECT SUM(price_each) AS price_each FROM orders", + "dialect": MFSQLEngine.BIGQUERY, + "meta": { + "superset": { + "metric_name": "preset_specific_key", + }, + }, + }, + ) + + assert convert_metric_flow_to_superset(other_semantic_metric) == { + "expression": "SUM(price_each)", + "metric_name": "preset_specific_key", + "metric_type": "SIMPLE", + "verbose_name": "Total Revenue", + "description": "Total revenue in the period", + "extra": "{}", } @@ -832,6 +870,7 @@ def test_get_superset_metrics_per_model() -> None: "depends_on": ["orders"], "calculation_method": "sum", "expression": "1", + "label": "Sales", }, { "name": "multi-model", @@ -845,6 +884,11 @@ def test_get_superset_metrics_per_model() -> None: "depends_on": ["orders"], "calculation_method": "sum", "expression": "1", + "meta": { + "superset": { + "warning_text": "caution", + }, + }, }, { "name": "b", @@ -852,6 +896,28 @@ def test_get_superset_metrics_per_model() -> None: "depends_on": ["customers"], "calculation_method": "sum", "expression": "1", + "config": { + "meta": { + "superset": { + "warning_text": "meta under config", + }, + }, + }, + }, + { + "name": "to_be_updated", + "label": "Preset Label", + "unique_id": "to_be_updated", + "depends_on": ["customers"], + "calculation_method": "max", + "expression": "1", + "config": { + "meta": { + "superset": { + "metric_name": "new_key", + }, + }, + }, }, ] ] @@ -862,11 +928,26 @@ def test_get_superset_metrics_per_model() -> None: { "name": "new", "description": "New metric", + "label": "New Label", "type": "SIMPLE", "sql": "SELECT COUNT(1) FROM a.b.c", "dialect": MFSQLEngine.BIGQUERY, "model": "new-model", }, + { + "name": "other_new", + "description": "This is a test replacing the metric key", + "label": "top Label", + "type": "SIMPLE", + "sql": "SELECT COUNT(1) FROM a.b.c", + "dialect": MFSQLEngine.BIGQUERY, + "model": "new-model", + "meta": { + "superset": { + "metric_name": "preset_sl_key", + }, + }, + }, ] ] @@ -876,7 +957,7 @@ def test_get_superset_metrics_per_model() -> None: "expression": "SUM(1)", "metric_name": "sales", "metric_type": "sum", - "verbose_name": "sales", + "verbose_name": "Sales", "description": "", "extra": "{}", }, @@ -886,6 +967,7 @@ def test_get_superset_metrics_per_model() -> None: "metric_type": "sum", "verbose_name": "a", "description": "", + "warning_text": "caution", "extra": "{}", }, ], @@ -896,6 +978,15 @@ def test_get_superset_metrics_per_model() -> None: "metric_type": "sum", "verbose_name": "b", "description": "", + "warning_text": "meta under config", + "extra": "{}", + }, + { + "expression": "MAX(1)", + "metric_name": "new_key", + "metric_type": "max", + "verbose_name": "Preset Label", + "description": "", "extra": "{}", }, ], @@ -904,8 +995,17 @@ def test_get_superset_metrics_per_model() -> None: "expression": "COUNT(1)", "metric_name": "new", "metric_type": "SIMPLE", - "verbose_name": "new", + "verbose_name": "New Label", "description": "New metric", + "extra": "{}", + }, + { + "expression": "COUNT(1)", + "metric_name": "preset_sl_key", + "metric_type": "SIMPLE", + "verbose_name": "top Label", + "description": "This is a test replacing the metric key", + "extra": "{}", }, ], } From 644bc29003f54b74fa99d76d7351c34dccbbc3d5 Mon Sep 17 00:00:00 2001 From: Vitor Avila Date: Wed, 14 Aug 2024 19:38:54 -0300 Subject: [PATCH 2/3] Addressing PR feedback --- src/preset_cli/api/clients/dbt.py | 27 +++++---- src/preset_cli/cli/superset/sync/dbt/lib.py | 9 +-- .../cli/superset/sync/dbt/metrics.py | 24 ++++---- tests/cli/superset/sync/dbt/datasets_test.py | 4 +- tests/cli/superset/sync/dbt/lib_test.py | 56 +++++++++++++++++-- tests/cli/superset/sync/dbt/metrics_test.py | 26 +++++---- 6 files changed, 98 insertions(+), 48 deletions(-) diff --git a/src/preset_cli/api/clients/dbt.py b/src/preset_cli/api/clients/dbt.py index 4bd2f0f8..b65f5f9f 100644 --- a/src/preset_cli/api/clients/dbt.py +++ b/src/preset_cli/api/clients/dbt.py @@ -574,15 +574,22 @@ class FilterSchema(PostelSchema): class MetricSchema(PostelSchema): """ - Schema for a metric. + Base schema for a dbt metric. """ - depends_on = fields.List(fields.String(), data_key="dependsOn") - description = fields.String() - filters = fields.List(fields.Nested(FilterSchema)) - meta = fields.Raw() name = fields.String() label = fields.String() + description = fields.String() + meta = fields.Raw() + + +class OGMetricSchema(MetricSchema): + """ + Schema for an OG metric. + """ + + depends_on = fields.List(fields.String(), data_key="dependsOn") + filters = fields.List(fields.Nested(FilterSchema)) sql = fields.String() type = fields.String() unique_id = fields.String(data_key="uniqueId") @@ -604,16 +611,12 @@ class MFMetricType(str, Enum): DERIVED = "DERIVED" -class MFMetricSchema(PostelSchema): +class MFMetricSchema(MetricSchema): """ Schema for a MetricFlow metric. """ - name = fields.String() - description = fields.String() type = PostelEnumField(MFMetricType) - meta = fields.Raw() - label = fields.String() class MFSQLEngine(str, Enum): @@ -814,7 +817,7 @@ def get_models(self, job_id: int) -> List[ModelSchema]: return models - def get_og_metrics(self, job_id: int) -> List[MetricSchema]: + def get_og_metrics(self, job_id: int) -> List[OGMetricSchema]: """ Fetch all available metrics. """ @@ -845,7 +848,7 @@ def get_og_metrics(self, job_id: int) -> List[MetricSchema]: headers=self.session.headers, ) - metric_schema = MetricSchema() + metric_schema = OGMetricSchema() metrics = [ metric_schema.load(metric) for metric in payload["data"]["job"]["metrics"] ] diff --git a/src/preset_cli/cli/superset/sync/dbt/lib.py b/src/preset_cli/cli/superset/sync/dbt/lib.py index 223c86a4..225c492a 100644 --- a/src/preset_cli/cli/superset/sync/dbt/lib.py +++ b/src/preset_cli/cli/superset/sync/dbt/lib.py @@ -16,7 +16,7 @@ from sqlalchemy.engine.url import URL from sqlalchemy.exc import NoSuchModuleError -from preset_cli.api.clients.dbt import MetricSchema, ModelSchema +from preset_cli.api.clients.dbt import MetricSchema, ModelSchema, OGMetricSchema from preset_cli.exceptions import CLIError _logger = logging.getLogger(__name__) @@ -505,11 +505,11 @@ def get_og_metric_from_config( dialect: str, depends_on: Optional[List[str]] = None, sql: Optional[str] = None, -) -> MetricSchema: +) -> OGMetricSchema: """ Return an og metric from the config, adhering to the dbt Cloud schema. """ - metric_schema = MetricSchema() + metric_schema = OGMetricSchema() if depends_on is not None: metric_config["dependsOn"] = depends_on metric_config.pop("depends_on", None) @@ -528,10 +528,11 @@ def get_og_metric_from_config( return metric_schema.load(metric_config) -def parse_metric_meta(metric_meta: Dict[str, Any]) -> Dict[str, Any]: +def parse_metric_meta(metric: MetricSchema) -> Dict[str, Any]: """ Parses the metric's meta information. """ + metric_meta = metric.get("meta", metric.get("config", {}).get("meta", {})) kwargs = metric_meta.pop("superset", {}) metric_name_override = kwargs.pop("metric_name", None) return { diff --git a/src/preset_cli/cli/superset/sync/dbt/metrics.py b/src/preset_cli/cli/superset/sync/dbt/metrics.py index cc496809..f61e3f9b 100644 --- a/src/preset_cli/cli/superset/sync/dbt/metrics.py +++ b/src/preset_cli/cli/superset/sync/dbt/metrics.py @@ -29,10 +29,10 @@ from preset_cli.api.clients.dbt import ( FilterSchema, - MetricSchema, MFMetricWithSQLSchema, MFSQLEngine, ModelSchema, + OGMetricSchema, ) from preset_cli.api.clients.superset import SupersetMetricDefinition from preset_cli.cli.superset.sync.dbt.exposures import ModelKey @@ -52,7 +52,7 @@ # pylint: disable=too-many-locals -def get_metric_expression(metric_name: str, metrics: Dict[str, MetricSchema]) -> str: +def get_metric_expression(metric_name: str, metrics: Dict[str, OGMetricSchema]) -> str: """ Return a SQL expression for a given dbt metric using sqlglot. """ @@ -125,7 +125,7 @@ def apply_filters(sql: str, filters: List[FilterSchema]) -> str: return f"CASE WHEN {condition} THEN {sql} END" -def is_derived(metric: MetricSchema) -> bool: +def is_derived(metric: OGMetricSchema) -> bool: """ Return if the metric is derived. """ @@ -138,8 +138,8 @@ def is_derived(metric: MetricSchema) -> bool: def get_metrics_for_model( model: ModelSchema, - metrics: List[MetricSchema], -) -> List[MetricSchema]: + metrics: List[OGMetricSchema], +) -> List[OGMetricSchema]: """ Given a list of metrics, return those that are based on a given model. """ @@ -171,7 +171,7 @@ def get_metrics_for_model( return related_metrics -def get_metric_models(unique_id: str, metrics: List[MetricSchema]) -> Set[str]: +def get_metric_models(unique_id: str, metrics: List[OGMetricSchema]) -> Set[str]: """ Given a metric, return the models it depends on. """ @@ -191,16 +191,14 @@ def get_metric_models(unique_id: str, metrics: List[MetricSchema]) -> Set[str]: def get_metric_definition( metric_name: str, - metrics: List[MetricSchema], + metrics: List[OGMetricSchema], ) -> SupersetMetricDefinition: """ Build a Superset metric definition from an OG (< 1.6) dbt metric. """ metric_map = {metric["name"]: metric for metric in metrics} metric = metric_map[metric_name] - metric_meta = parse_metric_meta( - metric.get("meta", metric.get("config", {}).get("meta", {})), - ) + metric_meta = parse_metric_meta(metric) final_metric_name = metric_meta["metric_name_override"] or metric_name return { @@ -215,7 +213,7 @@ def get_metric_definition( def get_superset_metrics_per_model( - og_metrics: List[MetricSchema], + og_metrics: List[OGMetricSchema], sl_metrics: Optional[List[MFMetricWithSQLSchema]] = None, ) -> Dict[str, List[SupersetMetricDefinition]]: """ @@ -361,7 +359,7 @@ def convert_metric_flow_to_superset( SUM(CASE WHEN order_total > 20 THEN 1 END) """ - metric_meta = parse_metric_meta(sl_metric.get("meta", {})) + metric_meta = parse_metric_meta(sl_metric) return { "expression": convert_query_to_projection( sl_metric["sql"], @@ -397,7 +395,7 @@ def get_models_from_sql( def replace_metric_syntax( sql: str, dependencies: List[str], - metrics: Dict[str, MetricSchema], + metrics: Dict[str, OGMetricSchema], ) -> str: """ Replace metric keys with their SQL syntax. diff --git a/tests/cli/superset/sync/dbt/datasets_test.py b/tests/cli/superset/sync/dbt/datasets_test.py index f29dd0dc..0ad966ba 100644 --- a/tests/cli/superset/sync/dbt/datasets_test.py +++ b/tests/cli/superset/sync/dbt/datasets_test.py @@ -12,7 +12,7 @@ from sqlalchemy.engine.url import make_url from yarl import URL -from preset_cli.api.clients.dbt import MetricSchema, ModelSchema +from preset_cli.api.clients.dbt import ModelSchema, OGMetricSchema from preset_cli.api.clients.superset import SupersetMetricDefinition from preset_cli.cli.superset.sync.dbt.datasets import ( DEFAULT_CERTIFICATION, @@ -36,7 +36,7 @@ SupersetError, ) -metric_schema = MetricSchema() +metric_schema = OGMetricSchema() metrics: Dict[str, List[SupersetMetricDefinition]] = { "model.superset_examples.messages_channels": [ diff --git a/tests/cli/superset/sync/dbt/lib_test.py b/tests/cli/superset/sync/dbt/lib_test.py index 2f783175..6ee78cee 100644 --- a/tests/cli/superset/sync/dbt/lib_test.py +++ b/tests/cli/superset/sync/dbt/lib_test.py @@ -14,7 +14,7 @@ from pytest_mock import MockerFixture from sqlalchemy.engine.url import URL -from preset_cli.api.clients.dbt import ModelSchema +from preset_cli.api.clients.dbt import MetricSchema, ModelSchema from preset_cli.cli.superset.sync.dbt.lib import ( apply_select, as_number, @@ -822,14 +822,49 @@ def test_parse_metric_meta() -> None: """ Test the ``parse_metric_meta`` helper. """ + metric_schema = MetricSchema() assert parse_metric_meta( - { - "superset": { - "d3format": ",.2f", - "metric_name": "revenue_metric", + metric_schema.load( + { + "name": "test metric", + "label": "Test Metric", + "description": "This is a test metric", + "meta": { + "superset": { + "d3format": ",.2f", + "metric_name": "revenue_metric", + }, + "airflow": "other_id", + }, }, + ), + ) == { + "meta": { "airflow": "other_id", }, + "kwargs": { + "d3format": ",.2f", + }, + "metric_name_override": "revenue_metric", + } + + assert parse_metric_meta( + metric_schema.load( + { + "name": "Sabe but using config", + "label": "Meta inside config", + "description": "", + "config": { + "meta": { + "superset": { + "d3format": ",.2f", + "metric_name": "revenue_metric", + }, + "airflow": "other_id", + }, + }, + }, + ), ) == { "meta": { "airflow": "other_id", @@ -840,7 +875,16 @@ def test_parse_metric_meta() -> None: "metric_name_override": "revenue_metric", } - assert parse_metric_meta({}) == { + assert parse_metric_meta( + metric_schema.load( + { + "name": "Sabe but using config", + "label": "Meta inside config", + "description": "", + "meta": {}, + }, + ), + ) == { "meta": {}, "kwargs": {}, "metric_name_override": None, diff --git a/tests/cli/superset/sync/dbt/metrics_test.py b/tests/cli/superset/sync/dbt/metrics_test.py index 22746703..1b06ea3b 100644 --- a/tests/cli/superset/sync/dbt/metrics_test.py +++ b/tests/cli/superset/sync/dbt/metrics_test.py @@ -9,7 +9,11 @@ import pytest from pytest_mock import MockerFixture -from preset_cli.api.clients.dbt import MetricSchema, MFMetricWithSQLSchema, MFSQLEngine +from preset_cli.api.clients.dbt import ( + MFMetricWithSQLSchema, + MFSQLEngine, + OGMetricSchema, +) from preset_cli.cli.superset.sync.dbt.exposures import ModelKey from preset_cli.cli.superset.sync.dbt.metrics import ( convert_metric_flow_to_superset, @@ -27,8 +31,8 @@ def test_get_metric_expression() -> None: """ Tests for ``get_metric_expression``. """ - metric_schema = MetricSchema() - metrics: Dict[str, MetricSchema] = { + metric_schema = OGMetricSchema() + metrics: Dict[str, OGMetricSchema] = { "one": metric_schema.load( { "type": "count", @@ -121,8 +125,8 @@ def test_get_metric_expression_new_schema() -> None: See https://docs.getdbt.com/guides/migration/versions/upgrading-to-v1.3#for-users-of-dbt-metrics """ - metric_schema = MetricSchema() - metrics: Dict[str, MetricSchema] = { + metric_schema = OGMetricSchema() + metrics: Dict[str, OGMetricSchema] = { "one": metric_schema.load( { "calculation_method": "count", @@ -146,8 +150,8 @@ def test_get_metric_expression_derived_legacy() -> None: """ Test ``get_metric_expression`` with derived metrics created using a legacy dbt version. """ - metric_schema = MetricSchema() - metrics: Dict[str, MetricSchema] = { + metric_schema = OGMetricSchema() + metrics: Dict[str, OGMetricSchema] = { "revenue_verbose_name_from_dbt": metric_schema.load( { "name": "revenue_verbose_name_from_dbt", @@ -476,7 +480,7 @@ def test_get_metric_models() -> None: """ Tests for ``get_metric_models``. """ - metric_schema = MetricSchema() + metric_schema = OGMetricSchema() metrics = [ metric_schema.load( { @@ -859,7 +863,7 @@ def test_get_superset_metrics_per_model() -> None: Tests for the ``get_superset_metrics_per_model`` function. """ mf_metric_schema = MFMetricWithSQLSchema() - og_metric_schema = MetricSchema() + og_metric_schema = OGMetricSchema() og_metrics = [ og_metric_schema.load(obj) @@ -1018,7 +1022,7 @@ def test_get_superset_metrics_per_model_og_derived( Tests for the ``get_superset_metrics_per_model`` function with derived OG metrics. """ - og_metric_schema = MetricSchema() + og_metric_schema = OGMetricSchema() og_metrics = [ og_metric_schema.load( @@ -1219,7 +1223,7 @@ def test_replace_metric_syntax() -> None: """ Test the ``replace_metric_syntax`` method. """ - og_metric_schema = MetricSchema() + og_metric_schema = OGMetricSchema() sql = "revenue - cost" metrics = { "revenue": og_metric_schema.load( From ed52edc85d8960fe94c12ee5666650b331277e0a Mon Sep 17 00:00:00 2001 From: Vitor Avila Date: Fri, 16 Aug 2024 15:19:23 -0300 Subject: [PATCH 3/3] Simplifying logic --- .../cli/superset/sync/dbt/command.py | 11 ++++++--- src/preset_cli/cli/superset/sync/dbt/lib.py | 5 ++-- tests/cli/superset/sync/dbt/lib_test.py | 12 ++++------ tests/cli/superset/sync/dbt/metrics_test.py | 24 +++++++++++-------- 4 files changed, 29 insertions(+), 23 deletions(-) diff --git a/src/preset_cli/cli/superset/sync/dbt/command.py b/src/preset_cli/cli/superset/sync/dbt/command.py index 11cfc5b4..a58eedbc 100644 --- a/src/preset_cli/cli/superset/sync/dbt/command.py +++ b/src/preset_cli/cli/superset/sync/dbt/command.py @@ -215,9 +215,14 @@ def dbt_core( # pylint: disable=too-many-arguments, too-many-branches, too-many og_metrics = [] sl_metrics = [] for config in configs["metrics"].values(): + # dbt is shifting from `metric.meta` to `metric.config.meta` + config["meta"] = config.get("meta") or config.get("config", {}).get( + "meta", + {}, + ) # First validate if metadata is already available - if config.get("meta", {}).get("superset", {}).get("model") and ( - sql := config.get("meta", {}).get("superset", {}).pop("expression") + if config["meta"].get("superset", {}).get("model") and ( + sql := config["meta"].get("superset", {}).pop("expression") ): metric = get_og_metric_from_config( config, @@ -422,7 +427,7 @@ def get_sl_metric( "sql": sql, "dialect": dialect.value, "model": model["unique_id"], - "meta": metric.get("meta", metric.get("config", {}).get("meta", {})), + "meta": metric["meta"], }, ) diff --git a/src/preset_cli/cli/superset/sync/dbt/lib.py b/src/preset_cli/cli/superset/sync/dbt/lib.py index 225c492a..1eea9786 100644 --- a/src/preset_cli/cli/superset/sync/dbt/lib.py +++ b/src/preset_cli/cli/superset/sync/dbt/lib.py @@ -532,11 +532,10 @@ def parse_metric_meta(metric: MetricSchema) -> Dict[str, Any]: """ Parses the metric's meta information. """ - metric_meta = metric.get("meta", metric.get("config", {}).get("meta", {})) - kwargs = metric_meta.pop("superset", {}) + kwargs = metric.get("meta", {}).pop("superset", {}) metric_name_override = kwargs.pop("metric_name", None) return { - "meta": metric_meta, + "meta": metric.get("meta", {}), "kwargs": kwargs, "metric_name_override": metric_name_override, } diff --git a/tests/cli/superset/sync/dbt/lib_test.py b/tests/cli/superset/sync/dbt/lib_test.py index 6ee78cee..91ba68d9 100644 --- a/tests/cli/superset/sync/dbt/lib_test.py +++ b/tests/cli/superset/sync/dbt/lib_test.py @@ -854,14 +854,12 @@ def test_parse_metric_meta() -> None: "name": "Sabe but using config", "label": "Meta inside config", "description": "", - "config": { - "meta": { - "superset": { - "d3format": ",.2f", - "metric_name": "revenue_metric", - }, - "airflow": "other_id", + "meta": { + "superset": { + "d3format": ",.2f", + "metric_name": "revenue_metric", }, + "airflow": "other_id", }, }, ), diff --git a/tests/cli/superset/sync/dbt/metrics_test.py b/tests/cli/superset/sync/dbt/metrics_test.py index 1b06ea3b..6b6ec719 100644 --- a/tests/cli/superset/sync/dbt/metrics_test.py +++ b/tests/cli/superset/sync/dbt/metrics_test.py @@ -875,12 +875,14 @@ def test_get_superset_metrics_per_model() -> None: "calculation_method": "sum", "expression": "1", "label": "Sales", + "meta": {}, }, { "name": "multi-model", "unique_id": "multi-model", "depends_on": ["a", "b"], "calculation_method": "derived", + "meta": {}, }, { "name": "a", @@ -900,11 +902,9 @@ def test_get_superset_metrics_per_model() -> None: "depends_on": ["customers"], "calculation_method": "sum", "expression": "1", - "config": { - "meta": { - "superset": { - "warning_text": "meta under config", - }, + "meta": { + "superset": { + "warning_text": "meta under config", }, }, }, @@ -915,11 +915,9 @@ def test_get_superset_metrics_per_model() -> None: "depends_on": ["customers"], "calculation_method": "max", "expression": "1", - "config": { - "meta": { - "superset": { - "metric_name": "new_key", - }, + "meta": { + "superset": { + "metric_name": "new_key", }, }, }, @@ -937,6 +935,7 @@ def test_get_superset_metrics_per_model() -> None: "sql": "SELECT COUNT(1) FROM a.b.c", "dialect": MFSQLEngine.BIGQUERY, "model": "new-model", + "meta": {}, }, { "name": "other_new", @@ -1032,6 +1031,7 @@ def test_get_superset_metrics_per_model_og_derived( "depends_on": ["orders"], "calculation_method": "sum", "expression": "1", + "meta": {}, }, ), og_metric_schema.load( @@ -1041,6 +1041,7 @@ def test_get_superset_metrics_per_model_og_derived( "depends_on": ["orders"], "calculation_method": "sum", "expression": "price_each", + "meta": {}, }, ), og_metric_schema.load( @@ -1050,6 +1051,7 @@ def test_get_superset_metrics_per_model_og_derived( "depends_on": [], "calculation_method": "derived", "expression": "price_each * 1.2", + "meta": {}, }, ), og_metric_schema.load( @@ -1092,6 +1094,7 @@ def test_get_superset_metrics_per_model_og_derived( {% endfor %} ) """, + "meta": {}, }, ), og_metric_schema.load( @@ -1102,6 +1105,7 @@ def test_get_superset_metrics_per_model_og_derived( "dialect": "postgres", "calculation_method": "derived", "expression": "derived_metric_with_jinja_and_other_metric / revenue", + "meta": {}, }, ), og_metric_schema.load(