-
Notifications
You must be signed in to change notification settings - Fork 26
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.
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
fix (dbt): derived metrics #154
Conversation
395c233
to
9f706a1
Compare
@@ -150,7 +150,7 @@ def dbt_core( # pylint: disable=too-many-arguments, too-many-locals | |||
for config in configs["metrics"].values(): | |||
# conform to the same schema that dbt Cloud uses for metrics | |||
config["dependsOn"] = config["depends_on"]["nodes"] | |||
config["uniqueID"] = config["unique_id"] | |||
config["uniqueId"] = config["unique_id"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was a bug, but since the metric unique ID was not needed anywhere it was not caught. With the changes in this PR we use the metric ID to traverse the DAG upstream to find parent models.
metric["name"]: metric | ||
for metric in metrics | ||
if model["unique_id"] in metric["depends_on"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was too naive, and didn't account for how derived metrics depend on other metrics, instead of depending on models. I replaced it with a smarter function get_metrics_for_model
that traverses the DAG upstream.
if is_derived(node): | ||
queue.extend(metric_map[parent] for parent in depends_on) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the crux of the PR: for derived metrics we need to look at the upstream metrics to find the parent models.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is the metric list something from superset or something from the dbt manifest?
if "calculation_method" in metric: | ||
# dbt >= 1.3 | ||
type_ = metric["calculation_method"] | ||
sql = metric["expression"] | ||
expression = "derived" | ||
else: | ||
# dbt < 1.3 | ||
type_ = metric["type"] | ||
sql = metric["sql"] | ||
expression = "expression" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice
@@ -22,8 +25,16 @@ def get_metric_expression(metric_name: str, metrics: Dict[str, MetricSchema]) -> | |||
raise Exception(f"Invalid metric {metric_name}") | |||
|
|||
metric = metrics[metric_name] | |||
type_ = metric["type"] | |||
sql = metric["sql"] | |||
if "calculation_method" in metric: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: have it inline
legacy = "calculation_method" in metric
type_ = metric["calculation_method"] if legacy else metric["type"]
sql = metric["expression"] if legacy else metric["sql"]
expression = "derived" if legacy else "expression"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Exciting work!! Can't wait to make some time and test this out. |
@betodealmeida The |
Ah, thanks! Let me fix it. |
Some fixes to derived metrics in dbt:
Closes #153 and #126.