Fix: push window dim filters into model subqueries, preserve preagg grain

Remove window_dim_extra_dims logic that added extra dimensions to preagg
subqueries, changing the CTE's aggregation grain. Instead, push window
dim filters into each owning model's model_filters dict. The recursive
generate() call handles window dim filter placement correctly in its
own outer WHERE, preserving the requested dimension grain.

Also fix merge conflict markers in test_advanced.py from prior stash.

Author: nicosuave

sidemantic/sql/generator.py

@@ -1468,28 +1468,6 @@ def _generate_with_preaggregation(
             all_filters, all_model_names
         )
 
-        # Collect window dimension columns that need to be projected through
-        # preagg CTEs so the outer WHERE can reference them.
-        window_dim_extra_dims: dict[str, list[str]] = {}
-        for model_name, wf_list in window_dim_filters.items():
-            for f_expr in wf_list:
-                try:
-                    parsed_f = sqlglot.parse_one(f_expr, dialect=self.dialect)
-                    for col in parsed_f.find_all(exp.Column):
-                        clean = (col.table or "").replace("_cte", "")
-                        if clean in all_model_names:
-                            model_obj = self.graph.get_model(clean)
-                            if model_obj:
-                                dim_obj = model_obj.get_dimension(col.name)
-                                if dim_obj and dim_obj.window is not None:
-                                    dim_ref = f"{clean}.{col.name}"
-                                    if clean not in window_dim_extra_dims:
-                                        window_dim_extra_dims[clean] = []
-                                    if dim_ref not in window_dim_extra_dims[clean]:
-                                        window_dim_extra_dims[clean].append(dim_ref)
-                except Exception:
-                    pass
-
         # Generate a pre-aggregated CTE for each metric model
         preagg_ctes = []
         cte_names = []
@@ -1498,25 +1476,18 @@ def _generate_with_preaggregation(
             cte_name = f"{model_name}_preagg"
             cte_names.append(cte_name)
 
-            # Only pass filters relevant to this model's sub-query.
-            # Window-dim filters are NOT pushed here: they are applied as outer
-            # WHERE on the final preagg join so that ALL models' metrics are
-            # constrained, not just the model that owns the window dimension.
-            model_filters = pushdown_by_model.get(model_name, [])
-
-            # Add extra window dim columns to the sub-query dimensions so they
-            # get projected through the preagg CTE for outer WHERE access.
-            sub_dimensions = list(dimensions)
-            for extra_dim in window_dim_extra_dims.get(model_name, []):
-                if extra_dim not in sub_dimensions:
-                    sub_dimensions.append(extra_dim)
+            # Pass pushdown filters plus any window-dim filters for this model.
+            # Window-dim filters are pushed into the model's sub-query (not the
+            # outer preagg join) so the recursive generate() handles them in its
+            # own outer WHERE, preserving the requested dimension grain.
+            model_filters = pushdown_by_model.get(model_name, []) + window_dim_filters.get(model_name, [])
 
             # Generate sub-query for this model's metrics at the dimension grain
             # We call generate() recursively but it won't trigger pre-aggregation
             # again because each sub-query has metrics from only one model
             sub_query = self.generate(
                 metrics=model_metrics,
-                dimensions=sub_dimensions,
+                dimensions=dimensions,
                 filters=model_filters,
                 segments=None,  # Already resolved
                 order_by=None,
@@ -1597,25 +1568,18 @@ def _generate_with_preaggregation(
 
         final_query = f"SELECT\n  {select_str}\nFROM {from_str}"
 
-        # Apply shared filters and window-dim filters on the outer query.
-        # Window-dim filters are applied here (not inside model sub-queries) so
-        # that ALL models' metrics are constrained by the filter, not just the
-        # model that owns the window dimension.
+        # Apply shared filters (cross-model or metric-level) on the outer query.
         # Rewrite table references from model/model_cte to model_preagg using
         # sqlglot AST rewriting so we don't accidentally mangle column names
         # that happen to contain a model name as a substring.
-        all_outer_filters = list(shared_filters)
-        for wf_list in window_dim_filters.values():
-            all_outer_filters.extend(wf_list)
-
-        if all_outer_filters:
+        if shared_filters:
             preagg_table_map = {}
             for model_name in metrics_by_model:
                 preagg_table_map[model_name] = f"{model_name}_preagg"
                 preagg_table_map[f"{model_name}_cte"] = f"{model_name}_preagg"
 
             rewritten = []
-            for f in all_outer_filters:
+            for f in shared_filters:
                 try:
                     parsed = sqlglot.parse_one(f, dialect=self.dialect)
                     for col in parsed.find_all(exp.Column):

tests/metrics/test_advanced.py

@@ -567,8 +567,6 @@ def test_wow_percent_change_partitions_by_dimension():
     assert rows[2][3] is None
     assert rows[3][0] == "B"
     assert abs(rows[3][3] - (-50.0)) < 0.1
-<<<<<<< Updated upstream
-=======
 
 
 def test_multistep_conversion_funnel():
@@ -1562,4 +1560,3 @@ def test_multistep_funnel_order_by_metric_name():
     # Both execute without error, confirming the metric-named column exists
     for row in rows:
         assert row[0] in ("US", "EU")  # region
->>>>>>> Stashed changes

tests/metrics/test_symmetric_aggs.py

@@ -324,3 +324,92 @@ def test_symmetric_aggregates_with_data():
     assert revenues == [100, 200]  # Correct totals, not inflated
 
     conn.close()
+
+
+def test_preagg_grain_preserved_with_filters():
+    """Test that preagg subqueries preserve the requested dimension grain when filters are applied.
+
+    Filters should not introduce extra dimensions into preagg subqueries. Each
+    preagg CTE must produce exactly one row per dimension key so the FULL OUTER
+    JOIN does not inflate results.
+    """
+    conn = duckdb.connect(":memory:")
+
+    conn.execute("""
+        CREATE TABLE raw_orders AS
+        SELECT * FROM (VALUES
+            (1, '2024-01-01'::DATE, 100, 'shipped'),
+            (2, '2024-01-01'::DATE, 200, 'pending'),
+            (3, '2024-01-02'::DATE, 150, 'shipped')
+        ) AS t(id, order_date, amount, status)
+    """)
+
+    conn.execute("""
+        CREATE TABLE raw_items AS
+        SELECT * FROM (VALUES
+            (1, 1, 5),
+            (2, 1, 3),
+            (3, 2, 10),
+            (4, 3, 7)
+        ) AS t(id, order_id, quantity)
+    """)
+
+    graph = SemanticGraph()
+
+    orders = Model(
+        name="orders",
+        table="raw_orders",
+        primary_key="id",
+        dimensions=[
+            Dimension(name="order_date", type="time", sql="order_date"),
+            Dimension(name="status", type="categorical", sql="status"),
+        ],
+        metrics=[Metric(name="revenue", agg="sum", sql="amount")],
+        relationships=[
+            Relationship(name="items", type="one_to_many", sql="id", foreign_key="order_id"),
+        ],
+    )
+
+    items = Model(
+        name="items",
+        table="raw_items",
+        primary_key="id",
+        dimensions=[],
+        metrics=[Metric(name="total_qty", agg="sum", sql="quantity")],
+        relationships=[
+            Relationship(name="orders", type="many_to_one", foreign_key="order_id", primary_key="id"),
+        ],
+    )
+
+    graph.add_model(orders)
+    graph.add_model(items)
+
+    generator = SQLGenerator(graph)
+
+    # Query with a filter on orders.status: preagg should still produce one
+    # row per order_date, not one row per (order_date, status).
+    sql = generator.generate(
+        metrics=["orders.revenue", "items.total_qty"],
+        dimensions=["orders.order_date"],
+        filters=["orders.status = 'shipped'"],
+    )
+
+    result = conn.execute(sql)
+    rows = fetch_rows(result)
+
+    # Verify no duplicate dimension keys
+    dates = [row[0] for row in rows]
+    assert len(dates) == len(set(dates)), f"Duplicate dimension keys in preagg result: {dates}"
+
+    # Verify correct values:
+    # The orders filter (status='shipped') only constrains orders_preagg.
+    # items_preagg aggregates ALL items by order_date (joined via orders).
+    # 2024-01-01: revenue=100 (shipped), total_qty=18 (items for orders 1+2)
+    # 2024-01-02: revenue=150 (shipped), total_qty=7 (items for order 3)
+    rows_sorted = sorted(rows, key=lambda r: r[0])
+    assert rows_sorted[0][1] == 100  # 2024-01-01 revenue (shipped only)
+    assert rows_sorted[0][2] == 18  # 2024-01-01 total_qty (all items for date)
+    assert rows_sorted[1][1] == 150  # 2024-01-02 revenue
+    assert rows_sorted[1][2] == 7  # 2024-01-02 total_qty
+
+    conn.close()

tests/optimizations/test_predicate_pushdown.py

@@ -519,13 +519,13 @@ def test_window_dimension_filter_with_regular_filter(layer):
     pytest.main([__file__, "-v"])
 
 
-def test_window_dim_filter_applied_as_outer_where_in_preagg(layer):
-    """Test that window-dim filters are applied as outer WHERE in the preagg path.
+def test_window_dim_filter_pushed_into_model_subquery_in_preagg(layer):
+    """Test that window-dim filters are pushed into model subqueries in the preagg path.
 
     When metrics come from multiple models (triggering pre-aggregation), a filter
-    on a window dimension must be applied as an outer WHERE on the final preagg
-    join so that ALL models' metrics are constrained. The window dim column is
-    projected through the owning model's preagg CTE to make it available.
+    on a window dimension is pushed into the owning model's recursive generate()
+    call. This preserves the preagg grain (no extra dimensions added) while the
+    recursive generate() handles the window dim filter in its own outer WHERE.
     """
     from sidemantic.core.model import Relationship
 
@@ -588,26 +588,24 @@ def test_window_dim_filter_applied_as_outer_where_in_preagg(layer):
     assert "orders_preagg" in sql, "Should use pre-aggregation path"
     assert "order_items_preagg" in sql, "Should use pre-aggregation path"
 
-    # The window dim column should be projected in the orders preagg CTE
+    # The window dim filter should be inside the orders preagg CTE (pushed into
+    # the model subquery), not in the outer WHERE
     preagg_start = sql.index("orders_preagg AS (")
     preagg_end = sql.index("order_items_preagg AS (")
     orders_subquery = sql[preagg_start:preagg_end]
-    assert "next_status" in orders_subquery, "Window dim column should be projected in orders preagg CTE"
+    assert "next_status" in orders_subquery, "Window dim filter should be in orders preagg subquery"
 
-    # The filter should appear in the outer WHERE referencing the preagg CTE,
-    # so that BOTH models' metrics are constrained by the filter
+    # The outer query should NOT have the window dim filter (it's handled inside subquery)
     outer_query = sql[sql.rindex("SELECT") :]
-    assert "orders_preagg.next_status" in outer_query, (
-        "Window-dim filter should be in outer WHERE referencing preagg CTE"
-    )
+    assert "next_status" not in outer_query, "Window dim filter should NOT be in outer WHERE"
 
 
-def test_multi_model_window_dim_filter_applied_as_outer_where_in_preagg(layer):
-    """Test that multi-model window-dim filters are applied as outer WHERE in preagg.
+def test_multi_model_window_dim_filter_pushed_into_model_subquery_in_preagg(layer):
+    """Test that multi-model window-dim filters are pushed into model subqueries in preagg.
 
     When a filter references columns from multiple models and at least one is a
-    window dimension, the window dim column is projected through the preagg CTE
-    and the filter is applied on the outer WHERE so both models are constrained.
+    window dimension, the filter is pushed into the owning model's subquery so
+    the recursive generate() handles it correctly without changing the preagg grain.
     """
     from sidemantic.core.model import Relationship
 
@@ -672,24 +670,24 @@ def test_multi_model_window_dim_filter_applied_as_outer_where_in_preagg(layer):
     assert "orders_preagg" in sql, "Should use pre-aggregation path"
     assert "order_items_preagg" in sql, "Should use pre-aggregation path"
 
-    # The window dim column should be projected in the orders preagg CTE
+    # The window dim filter should be inside the orders preagg CTE
     preagg_start = sql.index("orders_preagg AS (")
     preagg_end = sql.index("order_items_preagg AS (")
     orders_subquery = sql[preagg_start:preagg_end]
-    assert "next_status" in orders_subquery, "Window dim column should be projected in orders preagg CTE"
+    assert "next_status" in orders_subquery, "Window dim filter should be in orders preagg subquery"
 
-    # The filter should appear in the outer WHERE
+    # The outer query should NOT have the window dim filter
     outer_query = sql[sql.rindex("SELECT") :]
-    assert "orders_preagg.next_status" in outer_query, "Multi-model window-dim filter should be in outer WHERE"
+    assert "next_status" not in outer_query, "Window dim filter should NOT be in outer WHERE"
 
 
-def test_mixed_metric_and_window_dim_filter_applied_as_outer_where_in_preagg(layer):
-    """Test that a filter referencing both a metric and a window dim is applied as outer WHERE.
+def test_mixed_metric_and_window_dim_filter_pushed_into_model_subquery_in_preagg(layer):
+    """Test that a filter referencing both a metric and a window dim is pushed into model subquery.
 
     When a filter like "orders.next_status = 'complete' OR orders.revenue > 100"
     references both a window dimension (next_status) and a metric (revenue), the
-    window dim check takes priority and the filter is applied on the outer preagg
-    WHERE. The window dim column is projected through the preagg CTE.
+    window dim check takes priority and the filter is pushed into the owning model's
+    subquery to preserve the preagg grain.
     """
     from sidemantic.core.model import Relationship
 
@@ -752,12 +750,12 @@ def test_mixed_metric_and_window_dim_filter_applied_as_outer_where_in_preagg(lay
     assert "orders_preagg" in sql, "Should use pre-aggregation path"
     assert "order_items_preagg" in sql, "Should use pre-aggregation path"
 
-    # The window dim column should be projected in the orders preagg CTE
+    # The window dim filter should be inside the orders preagg CTE
     preagg_start = sql.index("orders_preagg AS (")
     preagg_end = sql.index("order_items_preagg AS (")
     orders_subquery = sql[preagg_start:preagg_end]
-    assert "next_status" in orders_subquery, "Window dim column should be projected in orders preagg CTE"
+    assert "next_status" in orders_subquery, "Window dim filter should be in orders preagg subquery"
 
-    # The filter should be in the outer WHERE, constraining both models
+    # The outer query should NOT have the window dim filter
     outer_query = sql[sql.rindex("SELECT") :]
-    assert "orders_preagg.next_status" in outer_query, "Mixed metric/window-dim filter should be in outer WHERE"
+    assert "next_status" not in outer_query, "Window dim filter should NOT be in outer WHERE"