Skip to content
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.

Sign up for GitHub

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

feat(dbt sync): Support metric key remapping #311

Merged
merged 3 commits into from
Aug 16, 2024
Merged

feat(dbt sync): Support metric key remapping #311

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c97eb59
feat(dbt sync): Support metric key remapping
Vitor-Avila Aug 14, 2024
644bc29
Addressing PR feedback
Vitor-Avila Aug 14, 2024
ed52edc
Simplifying logic
Vitor-Avila Aug 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/preset_cli/api/clients/dbt.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down Expand Up @@ -860,6 +862,7 @@ def get_sl_metrics(self, environment_id: int) -> List[MFMetricSchema]:
name
description
type
label
}
}
"""
Expand Down
4 changes: 4 additions & 0 deletions src/preset_cli/cli/superset/sync/dbt/command.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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", {})),
},
)

Expand Down Expand Up @@ -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)
},
),
)
Expand Down
13 changes: 13 additions & 0 deletions src/preset_cli/cli/superset/sync/dbt/lib.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
}
45 changes: 22 additions & 23 deletions src/preset_cli/cli/superset/sync/dbt/metrics.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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__)

Expand Down Expand Up @@ -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", {})),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should move this logic to the parse_metric_meta, it would make it easier to understand. Here we'd simply do:

metric_meta = parse_metric_meta(metric)

And the parse_metric_meta function would handle the retrieval of meta.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that makes sense! let me get that done

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@betodealmeida I implemented a base MetricSchema (shared across OG/MF/etc) o that I can also use it as the type hint for this helper. This caused some additional updates to other files.

)
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
}


Expand Down Expand Up @@ -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)

Expand Down Expand Up @@ -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.
Expand All @@ -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
}


Expand Down
14 changes: 10 additions & 4 deletions tests/cli/superset/sync/dbt/command_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"},
]


Expand Down Expand Up @@ -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": "{}",
},
],
},
Expand Down
30 changes: 30 additions & 0 deletions tests/cli/superset/sync/dbt/lib_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
get_og_metric_from_config,
list_failed_models,
load_profiles,
parse_metric_meta,
)
from preset_cli.exceptions import CLIError

Expand Down Expand Up @@ -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,
}
Loading
Loading