-
Notifications
You must be signed in to change notification settings - Fork 11
Add having filter support to metrics #120
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -773,6 +773,9 @@ def extract_from_measure_ref(metric_ref: str): | |
| # Check the measure's own filters | ||
| if measure.filters: | ||
| add_filter_columns(model_name, measure.filters) | ||
| # Check the measure's having clauses (columns in aggregate expressions) | ||
| if measure.having: | ||
| add_filter_columns(model_name, measure.having) | ||
| # If measure is a ratio/derived, recursively check dependencies | ||
| if measure.type == "ratio": | ||
| if measure.numerator: | ||
|
|
@@ -812,6 +815,15 @@ def extract_from_metric(metric): | |
| add_filter_columns(dep_model_name, metric.filters) | ||
| break | ||
|
|
||
| # Extract from metric's having clauses | ||
| if metric.having: | ||
| deps = metric.get_dependencies(self.graph) | ||
| for dep in deps: | ||
| if "." in dep: | ||
| dep_model_name = dep.split(".")[0] | ||
| add_filter_columns(dep_model_name, metric.having) | ||
| break | ||
|
|
||
| # For ratio metrics, check numerator and denominator | ||
| if metric.type == "ratio": | ||
| if metric.numerator: | ||
|
|
@@ -1793,6 +1805,30 @@ def _build_main_select( | |
| else: | ||
| where_filters.append(filter_expr) | ||
|
|
||
| # Collect metric-level having clauses (Metric.having) and add to HAVING filters. | ||
| # These are post-aggregation conditions defined on the metric itself, e.g., | ||
| # having: ["count(distinct platform) > 1"] for cross-platform user metrics. | ||
| for metric_ref in metrics: | ||
| metric_obj = None | ||
| metric_model_name = None | ||
| if "." in metric_ref: | ||
| metric_model_name, measure_name = metric_ref.split(".", 1) | ||
| model_obj = self.graph.get_model(metric_model_name) | ||
| if model_obj: | ||
| metric_obj = model_obj.get_metric(measure_name) | ||
| else: | ||
| try: | ||
| metric_obj = self.graph.get_metric(metric_ref) | ||
| except KeyError: | ||
| pass | ||
| if metric_obj and metric_obj.having: | ||
| for having_expr in metric_obj.having: | ||
| # Replace {model} placeholder with CTE reference | ||
| resolved = having_expr | ||
| if metric_model_name: | ||
| resolved = resolved.replace("{model}", f"{metric_model_name}_cte") | ||
| having_filters.append(resolved) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Metric-level HAVING expressions are appended unconditionally here, but ungrouped queries intentionally skip Useful? React with 👍 / 👎. |
||
|
|
||
| # Add WHERE clause (dimension filters only - metric-level filters are in CASE WHEN) | ||
| if where_filters: | ||
| # Parse filters to add table aliases and handle measure vs dimension columns | ||
|
|
||
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 loop only inspects the top-level
metricsargument, soMetric.havingis applied only when that metric is queried directly. If a derived/ratio metric references another metric that defineshaving,_build_metric_sqlinlines the dependency’s aggregation but this code never adds the dependency HAVING clause, which changes results by keeping groups that should be filtered out. A query likederived = 2 * base_metricwill ignorebase_metric.havingunlessbase_metricis also selected explicitly.Useful? React with 👍 / 👎.