Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 67624c0419
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for metric_ref in metrics: | ||
| metric_obj = None |
There was a problem hiding this comment.
This loop only inspects the top-level metrics argument, so Metric.having is applied only when that metric is queried directly. If a derived/ratio metric references another metric that defines having, _build_metric_sql inlines 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 like derived = 2 * base_metric will ignore base_metric.having unless base_metric is also selected explicitly.
Useful? React with 👍 / 👎.
| if metric_model_name: | ||
| resolved = resolved.replace("{model}", f"{metric_model_name}_cte") | ||
| having_filters.append(resolved) |
There was a problem hiding this comment.
Metric-level HAVING expressions are appended unconditionally here, but ungrouped queries intentionally skip GROUP BY and project raw *_raw columns. In that mode, adding aggregate HAVING predicates (the primary use case for this feature) produces invalid SQL on standard engines because the select list is not grouped/aggregated. This should be gated behind not ungrouped (or rejected during validation) to avoid generating non-executable queries.
Useful? React with 👍 / 👎.
Two fixes to metric-level HAVING clause handling: 1. The HAVING collection loop only inspected top-level metrics in the query. Derived/ratio metrics referencing base metrics with `having` would silently drop those clauses. Replace inline loop with _collect_metric_having_clauses that recursively traverses metric dependencies via get_dependencies(). 2. Metric-level HAVING expressions were appended unconditionally, but ungrouped queries skip GROUP BY. HAVING without GROUP BY is invalid SQL. Guard with `not ungrouped` check.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4f86535071
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| deps = metric.get_dependencies(self.graph) | ||
| for dep in deps: | ||
| collect_from_ref(dep) |
There was a problem hiding this comment.
_collect_metric_having_clauses traverses dependencies via metric.get_dependencies(self.graph) and then calls collect_from_ref(dep) without model context, so model-local ratio refs like numerator='revenue' / denominator='order_count' are treated as graph metrics and never resolved to orders.revenue; this drops base metric having clauses and silently changes results. Fresh evidence in this commit: compiling orders.aov (ratio with unqualified deps) where orders.revenue has having=['SUM(amount) > 100'] emits SQL with no HAVING clause.
Useful? React with 👍 / 👎.
|
Closing: HAVING as a separate field is the wrong abstraction for a semantic layer. The use case (cross-platform users etc.) is better served by subquery expressions in existing filters/segments. |
1 participant
Summary
havingfield to Metric for HAVING clause filters (post-aggregation conditions)count(distinct platform) > 1after GROUP BYfiltersremain as WHERE clauses via CASE WHEN in CTEshavingfieldExample