Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
48 changes: 48 additions & 0 deletions sidemantic-schema.json
Original file line number Diff line number Diff line change
Expand Up @@ -468,6 +468,22 @@
"description": "Grain for period-to-date (e.g., 'month' for MTD)",
"title": "Grain To Date"
},
"having": {
"anyOf": [
{
"items": {
"type": "string"
},
"type": "array"
},
{
"type": "null"
}
],
"default": null,
"description": "Optional HAVING clause filters applied after GROUP BY (e.g., 'count(distinct platform) > 1')",
"title": "Having"
},
"label": {
"anyOf": [
{
Expand Down Expand Up @@ -1401,6 +1417,22 @@
"description": "Grain for period-to-date (e.g., 'month' for MTD)",
"title": "Grain To Date"
},
"having": {
"anyOf": [
{
"items": {
"type": "string"
},
"type": "array"
},
{
"type": "null"
}
],
"default": null,
"description": "Optional HAVING clause filters applied after GROUP BY (e.g., 'count(distinct platform) > 1')",
"title": "Having"
},
"label": {
"anyOf": [
{
Expand Down Expand Up @@ -2084,6 +2116,22 @@
"description": "Grain for period-to-date (e.g., 'month' for MTD)",
"title": "Grain To Date"
},
"having": {
"anyOf": [
{
"items": {
"type": "string"
},
"type": "array"
},
{
"type": "null"
}
],
"default": null,
"description": "Optional HAVING clause filters applied after GROUP BY (e.g., 'count(distinct platform) > 1')",
"title": "Having"
},
"label": {
"anyOf": [
{
Expand Down
6 changes: 6 additions & 0 deletions sidemantic/adapters/sidemantic.py
Original file line number Diff line number Diff line change
Expand Up @@ -278,6 +278,7 @@ def _parse_model(self, model_def: dict) -> Model | None:
sql=measure_def.get("sql") or measure_def.get("expr"),
type=measure_def.get("type"),
filters=measure_def.get("filters"),
having=measure_def.get("having"),
fill_nulls_with=measure_def.get("fill_nulls_with"),
description=measure_def.get("description"),
label=measure_def.get("label"),
Expand Down Expand Up @@ -419,6 +420,7 @@ def _parse_metric(self, metric_def: dict) -> Metric | None:
window_frame=metric_def.get("window_frame"),
window_order=metric_def.get("window_order"),
filters=metric_def.get("filters"),
having=metric_def.get("having"),
fill_nulls_with=metric_def.get("fill_nulls_with"),
format=metric_def.get("format"),
value_format_name=metric_def.get("value_format_name"),
Expand Down Expand Up @@ -540,6 +542,8 @@ def _export_model(self, model: Model) -> dict:
measure_def["sql"] = measure.sql
if measure.filters:
measure_def["filters"] = measure.filters
if measure.having:
measure_def["having"] = measure.having
if measure.description:
measure_def["description"] = measure.description
if measure.label:
Expand Down Expand Up @@ -666,5 +670,7 @@ def _export_metric(self, measure: Metric, graph) -> dict:
result["window"] = measure.window
if measure.filters:
result["filters"] = measure.filters
if measure.having:
result["having"] = measure.having

return result
4 changes: 4 additions & 0 deletions sidemantic/core/metric.py
Original file line number Diff line number Diff line change
Expand Up @@ -254,6 +254,10 @@ def validate_type_specific_fields(self):

# Common parameters
filters: list[str] | None = Field(None, description="Optional WHERE clause filters")
having: list[str] | None = Field(
None,
description="Optional HAVING clause filters applied after GROUP BY (e.g., 'count(distinct platform) > 1')",
)
fill_nulls_with: int | float | str | None = Field(None, description="Default value when result is NULL")
description: str | None = Field(None, description="Human-readable description")
label: str | None = Field(None, description="Display label")
Expand Down
82 changes: 81 additions & 1 deletion sidemantic/sql/generator.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -1793,6 +1805,12 @@ 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.
# Traverses metric dependencies so derived/ratio metrics inherit base metric HAVING.
self._collect_metric_having_clauses(metrics, having_filters)

# 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
Expand Down Expand Up @@ -1850,7 +1868,8 @@ def replace_field(match):
query = query.group_by(*group_by_positions)

# Add HAVING clause (metric filters applied after aggregation)
if having_filters:
# Skip HAVING entirely for ungrouped queries: HAVING without GROUP BY is invalid SQL
if having_filters and not ungrouped:
for filter_expr in having_filters:
# Replace model.metric_name with metric_name (the aggregated column alias)
parsed_having = filter_expr
Expand Down Expand Up @@ -2016,6 +2035,67 @@ def _build_measure_aggregation_sql(self, model_name: str, measure) -> str:
return f"COUNT({raw_col})"
return f"{agg_func}({raw_col})"

def _collect_metric_having_clauses(self, metrics: list[str], having_filters: list[str]) -> None:
"""Collect HAVING clauses from metric definitions and their dependencies.

Inspects each metric referenced in the query. If a metric (or any base
metric it depends on) declares a ``having`` list, those expressions are
appended to *having_filters* (in-place) so they end up in the generated
HAVING clause.

Args:
metrics: Metric references from the query (e.g. ["orders.revenue"])
having_filters: Mutable list to which new HAVING expressions are appended
"""
seen: set[str] = set()

def collect_from_ref(metric_ref: str) -> None:
if metric_ref in seen:
return
seen.add(metric_ref)

metric = None
metric_model_name = None
if "." in metric_ref:
metric_model_name, measure_name = metric_ref.split(".", 1)
try:
model = self.graph.get_model(metric_model_name)
if model:
metric = model.get_metric(measure_name)
except KeyError:
pass
# Fall back to graph-level metric with dotted name
if not metric:
try:
metric = self.graph.get_metric(metric_ref)
except KeyError:
pass
else:
try:
metric = self.graph.get_metric(metric_ref)
except KeyError:
pass

if not metric:
return

# Append this metric's own having clauses
if metric.having:
for h in metric.having:
resolved = h
if metric_model_name:
resolved = resolved.replace("{model}", f"{metric_model_name}_cte")
if resolved not in having_filters:
having_filters.append(resolved)

# Traverse dependencies (ratio numerator/denominator, derived refs, etc.)
deps = metric.get_dependencies(self.graph)
for dep in deps:
collect_from_ref(dep)
Comment on lines +2092 to +2094
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Resolve model-scoped deps when collecting metric HAVING

_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 👍 / 👎.


for m in metrics:
collect_from_ref(m)

def _build_metric_sql(self, metric, model_context: str | None = None) -> str:
"""Build SQL expression for a metric.

Expand Down
Loading
Loading