Fix: expand {model} placeholders, strict periods default, parse retention_granularity

Author: nicosuave

sidemantic/adapters/sidemantic.py

@@ -301,7 +301,7 @@ def _parse_model(self, model_def: dict) -> Model | None:
                 cohort_event=measure_def.get("cohort_event"),
                 activity_event=measure_def.get("activity_event"),
                 periods=measure_def.get("periods"),
-                retention_granularity=measure_def.get("granularity")
+                retention_granularity=(measure_def.get("retention_granularity") or measure_def.get("granularity"))
                 if measure_def.get("type") == "retention"
                 else None,
                 # Cumulative/window parameters
@@ -423,7 +423,9 @@ def _parse_metric(self, metric_def: dict) -> Metric | None:
             cohort_event=metric_def.get("cohort_event"),
             activity_event=metric_def.get("activity_event"),
             periods=metric_def.get("periods"),
-            retention_granularity=metric_def.get("granularity") if metric_type == "retention" else None,
+            retention_granularity=(metric_def.get("retention_granularity") or metric_def.get("granularity"))
+            if metric_type == "retention"
+            else None,
             window=metric_def.get("window"),
             grain_to_date=metric_def.get("grain_to_date"),
             window_expression=metric_def.get("window_expression"),
@@ -593,7 +595,7 @@ def _export_model(self, model: Model) -> dict:
                 if measure.periods is not None:
                     measure_def["periods"] = measure.periods
                 if measure.retention_granularity:
-                    measure_def["granularity"] = measure.retention_granularity
+                    measure_def["retention_granularity"] = measure.retention_granularity
                 # Cumulative/window parameters
                 if measure.window:
                     measure_def["window"] = measure.window
@@ -682,7 +684,7 @@ def _export_metric(self, measure: Metric, graph) -> dict:
         if measure.periods is not None:
             result["periods"] = measure.periods
         if measure.retention_granularity:
-            result["granularity"] = measure.retention_granularity
+            result["retention_granularity"] = measure.retention_granularity
         if measure.sql:
             result["sql"] = measure.sql
             # Auto-detect and export dependencies for derived measures

sidemantic/sql/generator.py

@@ -2310,11 +2310,20 @@ def _generate_retention_query(
         if not model:
             raise ValueError(f"No model found for retention metric {metric_name}")
 
-        # Defaults
-        periods = metric.periods or 28
-        granularity = metric.retention_granularity or "day"
+        # Defaults (use `is not None` to avoid converting 0 to the default)
+        periods = metric.periods if metric.periods is not None else 28
+        granularity = metric.retention_granularity if metric.retention_granularity is not None else "day"
         activity_event = metric.activity_event or "TRUE"
 
+        # Replace {model} placeholders in event predicates with actual table alias
+        table_alias = "t" if model.sql else ""
+        if table_alias:
+            cohort_event = metric.cohort_event.replace("{model}", table_alias)
+            activity_event = activity_event.replace("{model}", table_alias)
+        else:
+            cohort_event = metric.cohort_event.replace("{model}.", "")
+            activity_event = activity_event.replace("{model}.", "")
+
         # Validate entity identifier
         if not _re.match(r"^[a-zA-Z_][a-zA-Z0-9_.]*$", metric.entity):
             raise ValueError(f"Invalid entity identifier: {metric.entity}")
@@ -2385,7 +2394,7 @@ def _generate_retention_query(
         sql = f"""WITH cohorts AS (
   SELECT {metric.entity}, MIN({trunc_expr}) AS cohort_date
   FROM {from_clause}
-  WHERE {metric.cohort_event}{filter_clause}
+  WHERE {cohort_event}{filter_clause}
   GROUP BY {metric.entity}
 ),
 activity AS (

tests/metrics/test_retention.py

@@ -316,3 +316,234 @@ def test_retention_week_granularity():
     assert week_data[0][4] == 100.0
     assert week_data[1][4] == 100.0
     assert week_data[2][4] == 50.0
+
+
+def test_retention_model_placeholder_expansion_sql_model():
+    """Test that {model} placeholders in cohort_event/activity_event are expanded to table alias."""
+    events = Model(
+        name="events",
+        sql="""
+            SELECT 1 AS uid, 'signup' AS event, '2024-01-01'::DATE AS ts
+            UNION ALL SELECT 1, 'login', '2024-01-02'::DATE
+        """,
+        primary_key="uid",
+        dimensions=[
+            Dimension(name="uid", sql="uid", type="categorical"),
+            Dimension(name="event", sql="event", type="categorical"),
+            Dimension(name="ts", sql="ts", type="time"),
+        ],
+        metrics=[],
+    )
+
+    retention = Metric(
+        name="retention",
+        type="retention",
+        entity="uid",
+        cohort_event="{model}.event = 'signup'",
+        activity_event="{model}.event IS NOT NULL",
+        periods=1,
+        retention_granularity="day",
+    )
+
+    graph = SemanticGraph()
+    graph.add_model(events)
+    graph.add_metric(retention)
+
+    generator = SQLGenerator(graph)
+    sql = generator.generate(metrics=["retention"], dimensions=[])
+
+    # {model} should be replaced with 't' (SQL subquery alias)
+    assert "{model}" not in sql
+    assert "t.event = 'signup'" in sql
+    assert "t.event IS NOT NULL" in sql
+
+    # Should still execute correctly
+    conn = duckdb.connect(":memory:")
+    result = conn.execute(sql)
+    rows = df_rows(result)
+    assert len(rows) > 0
+
+
+def test_retention_model_placeholder_expansion_table_model():
+    """Test that {model} placeholders are stripped for table-backed models."""
+    conn = duckdb.connect(":memory:")
+    conn.execute("""
+        CREATE TABLE test_events AS
+        SELECT 1 AS uid, 'signup' AS event, '2024-01-01'::DATE AS ts
+        UNION ALL SELECT 1, 'login', '2024-01-02'::DATE
+    """)
+
+    events = Model(
+        name="events",
+        table="test_events",
+        primary_key="uid",
+        dimensions=[
+            Dimension(name="uid", sql="uid", type="categorical"),
+            Dimension(name="event", sql="event", type="categorical"),
+            Dimension(name="ts", sql="ts", type="time"),
+        ],
+        metrics=[],
+    )
+
+    retention = Metric(
+        name="retention",
+        type="retention",
+        entity="uid",
+        cohort_event="{model}.event = 'signup'",
+        activity_event="{model}.event IS NOT NULL",
+        periods=1,
+        retention_granularity="day",
+    )
+
+    graph = SemanticGraph()
+    graph.add_model(events)
+    graph.add_metric(retention)
+
+    generator = SQLGenerator(graph)
+    sql = generator.generate(metrics=["retention"], dimensions=[])
+
+    # {model}. should be stripped for table-backed models
+    assert "{model}" not in sql
+    assert "event = 'signup'" in sql
+
+    result = conn.execute(sql)
+    rows = df_rows(result)
+    assert len(rows) > 0
+
+
+def test_retention_periods_zero_raises_validation_error():
+    """Test that periods=0 raises a validation error instead of silently becoming 28."""
+    events = Model(
+        name="events",
+        sql="""
+            SELECT 1 AS uid, 'signup' AS event, '2024-01-01'::DATE AS ts
+        """,
+        primary_key="uid",
+        dimensions=[
+            Dimension(name="uid", sql="uid", type="categorical"),
+            Dimension(name="event", sql="event", type="categorical"),
+            Dimension(name="ts", sql="ts", type="time"),
+        ],
+        metrics=[],
+    )
+
+    retention = Metric(
+        name="retention",
+        type="retention",
+        entity="uid",
+        cohort_event="event = 'signup'",
+        periods=0,
+    )
+
+    graph = SemanticGraph()
+    graph.add_model(events)
+    graph.add_metric(retention)
+
+    generator = SQLGenerator(graph)
+    with pytest.raises(ValueError, match="Invalid periods value"):
+        generator.generate(metrics=["retention"], dimensions=[])
+
+
+def test_retention_yaml_retention_granularity_key():
+    """Test that YAML with retention_granularity: week parses correctly."""
+    import os
+    import tempfile
+
+    from sidemantic.adapters.sidemantic import SidemanticAdapter
+
+    yaml_content = """
+models:
+  - name: events
+    table: events
+    dimensions:
+      - name: user_id
+        type: categorical
+      - name: ts
+        type: time
+    metrics:
+      - name: weekly_retention
+        type: retention
+        entity: user_id
+        cohort_event: "event = 'signup'"
+        retention_granularity: week
+        periods: 4
+"""
+    with tempfile.NamedTemporaryFile(mode="w", suffix=".yml", delete=False) as f:
+        f.write(yaml_content)
+        tmp_path = f.name
+
+    try:
+        adapter = SidemanticAdapter()
+        graph = adapter.parse(tmp_path)
+        model = graph.get_model("events")
+        metric = model.get_metric("weekly_retention")
+        assert metric.retention_granularity == "week"
+        assert metric.periods == 4
+    finally:
+        os.unlink(tmp_path)
+
+
+def test_retention_yaml_granularity_fallback():
+    """Test that YAML with granularity: month also parses for retention metrics."""
+    import os
+    import tempfile
+
+    from sidemantic.adapters.sidemantic import SidemanticAdapter
+
+    yaml_content = """
+metrics:
+  - name: monthly_retention
+    type: retention
+    entity: user_id
+    cohort_event: "event = 'signup'"
+    granularity: month
+    periods: 12
+"""
+    with tempfile.NamedTemporaryFile(mode="w", suffix=".yml", delete=False) as f:
+        f.write(yaml_content)
+        tmp_path = f.name
+
+    try:
+        adapter = SidemanticAdapter()
+        graph = adapter.parse(tmp_path)
+        metric = graph.get_metric("monthly_retention")
+        assert metric.retention_granularity == "month"
+        assert metric.periods == 12
+    finally:
+        os.unlink(tmp_path)
+
+
+def test_retention_export_roundtrip_retention_granularity():
+    """Test that export uses retention_granularity key and roundtrips correctly."""
+    import os
+    import tempfile
+
+    from sidemantic.adapters.sidemantic import SidemanticAdapter
+
+    # Create a graph with a retention metric
+    graph = SemanticGraph()
+    retention = Metric(
+        name="weekly_retention",
+        type="retention",
+        entity="user_id",
+        cohort_event="event = 'signup'",
+        retention_granularity="week",
+        periods=4,
+    )
+    graph.add_metric(retention)
+
+    # Export
+    with tempfile.NamedTemporaryFile(mode="w", suffix=".yml", delete=False) as f:
+        tmp_path = f.name
+
+    try:
+        adapter = SidemanticAdapter()
+        adapter.export(graph, tmp_path)
+
+        # Re-parse and verify
+        graph2 = adapter.parse(tmp_path)
+        metric = graph2.get_metric("weekly_retention")
+        assert metric.retention_granularity == "week"
+        assert metric.periods == 4
+    finally:
+        os.unlink(tmp_path)