diff --git a/docs/md/doc/compose.md b/docs/md/doc/compose.md index a00c5ed1..7791de6e 100644 --- a/docs/md/doc/compose.md +++ b/docs/md/doc/compose.md @@ -152,7 +152,7 @@ result = ( ## Key Takeaways -- **Composition via Joins**: Use `join_many()`, `join_one()`, or `join()` to compose models +- **Composition via Joins**: Use `join_many()`, `join_one()`, or `join_cross()` to compose models - **Additive**: Each join adds dimensions and measures from the joined table - **Table Prefixes**: Dimensions/measures are prefixed with table names (`flights.`, `carriers.`, `aircraft.`) - **No Limit**: Compose as many models as needed for your analysis diff --git a/docs/md/doc/nested-subtotals.md b/docs/md/doc/nested-subtotals.md index 6620b725..27b967e6 100644 --- a/docs/md/doc/nested-subtotals.md +++ b/docs/md/doc/nested-subtotals.md @@ -76,7 +76,16 @@ result = ( .aggregate( year_order_count=lambda t: t.order_count.sum(), year_total_sales=lambda t: t.total_sales.sum(), - nest={"by_month": lambda t: t.group_by(["created_month", "order_count", "total_sales"]).order_by("created_month")} + nest={ + "by_month": lambda t: ( + t.group_by("created_month") + .aggregate( + order_count=lambda t: t.order_count.sum(), + total_sales=lambda t: t.total_sales.sum(), + ) + .order_by("created_month") + ) + } ) .order_by("created_year") ) @@ -109,7 +118,17 @@ result = ( .aggregate( year_order_count=lambda t: t.order_count.sum(), year_total_sales=lambda t: t.total_sales.sum(), - nest={"by_status": lambda t: t.group_by(["status", "order_count", "total_sales", "avg_price"]).order_by(xo.desc("total_sales"))} + nest={ + "by_status": lambda t: ( + t.group_by("status") + .aggregate( + order_count=lambda t: t.order_count.sum(), + total_sales=lambda t: t.total_sales.sum(), + avg_price=lambda t: t.avg_price.mean(), + ) + .order_by(lambda t: t.total_sales.desc()) + ) + } ) .order_by("created_year") ) @@ -138,7 +157,14 @@ monthly_with_status = ( .aggregate( month_order_count=lambda t: t.order_count.sum(), month_total_sales=lambda t: t.total_sales.sum(), - nest={"by_status": lambda t: t.group_by(["status", "order_count", "total_sales"])} + nest={ + "by_status": lambda t: ( + t.group_by("status").aggregate( + order_count=lambda t: t.order_count.sum(), + total_sales=lambda t: t.total_sales.sum(), + ) + ) + } ) ) @@ -149,7 +175,11 @@ result = ( .aggregate( year_order_count=lambda t: t.month_order_count.sum(), year_total_sales=lambda t: t.month_total_sales.sum(), - nest={"by_month": lambda t: t.group_by(["created_month", "month_order_count", "month_total_sales", "by_status"]).order_by("created_month")} + nest={ + "by_month": lambda t: t.group_by( + "created_month", "month_order_count", "month_total_sales", "by_status" + ) + } ) .order_by("created_year") .limit(3) diff --git a/docs/md/doc/query-methods.md b/docs/md/doc/query-methods.md index b0fd3c26..051ee015 100644 --- a/docs/md/doc/query-methods.md +++ b/docs/md/doc/query-methods.md @@ -297,19 +297,21 @@ result = ( **How it works:** - The `nest` parameter accepts a dictionary: `{"column_name": lambda t: ...}` -- The lambda specifies which columns to collect using `.group_by()` or `.select()` +- A bare `.group_by()` in the lambda specifies which row fields to collect - Results in an array of structs column named `flights` -You can also use `.select()` to specify which columns to nest: +To nest selected row-level fields, use the bare `group_by(...)` form. In a +`nest` lambda this selects the struct fields to collect; it does not collapse +duplicate source rows: ```query_nest_select -# Nest specific columns +# Nest specific row-level fields result = ( flights_st .group_by("carrier") .aggregate( "flight_count", - nest={"routes": lambda t: t.select("origin", "distance", "duration")} + nest={"routes": lambda t: t.group_by("origin", "distance", "duration")} ) ) ``` diff --git a/docs/md/doc/reference.md b/docs/md/doc/reference.md index d0c14285..9f6b7cd8 100644 --- a/docs/md/doc/reference.md +++ b/docs/md/doc/reference.md @@ -169,6 +169,7 @@ Methods for composing semantic tables through joins. join_many( other: SemanticTable, on: Callable, + how: str = "left", name: str = None ) -> SemanticTable ``` @@ -179,6 +180,7 @@ One-to-many relationship join (LEFT JOIN). Use when the left table can match mul |-----------|------|-------------| | `other` | `SemanticTable` | The semantic table to join with | | `on` | `Callable` | Lambda function defining the join condition | +| `how` | `str` | Join type; only `'left'` is supported | | `name` | `str` | Optional name for the joined table reference | **Example:** @@ -196,11 +198,13 @@ flights_st = flights_st.join_many( join_one( other: SemanticTable, on: Callable, + how: str = "left", name: str = None ) -> SemanticTable ``` -One-to-one relationship join (INNER JOIN). Use when each row in the left table matches exactly one row in the right table. +One-to-one relationship join (LEFT JOIN). Use when each left row can match at +most one row in the right table. Only `how="left"` is supported. **Example:** ```python @@ -221,26 +225,6 @@ join_cross( Cross join (CARTESIAN PRODUCT). Creates all possible combinations of rows from both tables. -### join() - -```python -join( - other: SemanticTable, - on: Callable, - how: str = "inner", - name: str = None -) -> SemanticTable -``` - -Custom join with flexible join type. Supports 'inner', 'left', 'right', 'outer', and 'cross'. - -| Parameter | Type | Description | -|-----------|------|-------------| -| `other` | `SemanticTable` | The semantic table to join with | -| `on` | `Callable` | Lambda function defining the join condition | -| `how` | `str` | Join type: 'inner', 'left', 'right', 'outer', or 'cross' | -| `name` | `str` | Optional name for the joined table reference | - ## Query Methods Methods for querying and transforming semantic tables. @@ -369,7 +353,7 @@ Create nested data structures within aggregations. aggregate( *measures, nest={ - "nested_column": lambda t: t.group_by([...]) | t.select(...) + "nested_column": lambda t: t.group_by(...) } ) ``` @@ -531,7 +515,7 @@ model_name: join_name: model: model_reference on: join_condition - how: join_type # left, inner, right, outer, cross + how: left # Optional; non-cross semantic joins are always left joins ``` #### Expression Syntax diff --git a/docs/md/doc/semantic-table.md b/docs/md/doc/semantic-table.md index 897a07bf..977eadb1 100644 --- a/docs/md/doc/semantic-table.md +++ b/docs/md/doc/semantic-table.md @@ -289,7 +289,9 @@ After joining, all dimensions and measures from both tables are available. Each ### join_one() - One-to-One Relationships -Use `join_one()` when rows have a unique matching relationship (INNER JOIN). +Use `join_one()` when rows have a unique matching relationship. Like all +non-cross semantic joins, it uses a LEFT JOIN so unmatched left rows remain +visible to measures. ```python # Many flights → one carrier (each flight has exactly one carrier) @@ -330,34 +332,20 @@ Use `join_cross()` to create every possible combination of rows from both tables all_combinations = flights_st.join_cross(carriers) ``` -### join() - Custom Join Conditions +### Requiring a Match -Use `join()` for complex join conditions or specific SQL join types. +`join_one()` and `join_many()` only support `how="left"`. When a query should +require a match, make the row removal explicit with a filter on a non-nullable +field from the right table: ```python -# LEFT JOIN with custom condition -flights_with_carriers = flights_st.join( +flights_matched = flights_st.join_one( carriers, lambda f, c: f.carrier == c.code, - how="left" -) - -# INNER JOIN -flights_matched = flights_st.join( - carriers, - lambda f, c: f.carrier == c.code, - how="inner" -) - -# Complex conditions -date_range_join = flights_st.join( - promotions, - lambda f, p: (f.date >= p.start_date) & (f.date <= p.end_date), - how="left" -) +).filter(lambda t: t["carriers.name"].notnull()) ``` -**Supported join types:** `"inner"`, `"left"`, `"right"`, `"outer"`, `"cross"` +Use `join_cross()` for Cartesian products. ## Next Steps diff --git a/docs/md/prompts/build/system.md b/docs/md/prompts/build/system.md index 743e60a7..1014ce93 100644 --- a/docs/md/prompts/build/system.md +++ b/docs/md/prompts/build/system.md @@ -104,7 +104,7 @@ flights_with_carriers = flights_st.join_many( ) ``` -### join_one() - One-to-One (INNER JOIN) +### join_one() - One-to-One (LEFT JOIN) ```python # Each flight has exactly one carrier diff --git a/docs/md/skills/claude-code/bsl-model-builder/SKILL.md b/docs/md/skills/claude-code/bsl-model-builder/SKILL.md index 9eae3391..a94c7d9b 100644 --- a/docs/md/skills/claude-code/bsl-model-builder/SKILL.md +++ b/docs/md/skills/claude-code/bsl-model-builder/SKILL.md @@ -109,7 +109,7 @@ flights_with_carriers = flights_st.join_many( ) ``` -### join_one() - One-to-One (INNER JOIN) +### join_one() - One-to-One (LEFT JOIN) ```python # Each flight has exactly one carrier diff --git a/docs/md/skills/codex/bsl-model-builder.codex b/docs/md/skills/codex/bsl-model-builder.codex index a9eaf96f..25a196b8 100644 --- a/docs/md/skills/codex/bsl-model-builder.codex +++ b/docs/md/skills/codex/bsl-model-builder.codex @@ -108,7 +108,7 @@ flights_with_carriers = flights_st.join_many( ) ``` -### join_one() - One-to-One (INNER JOIN) +### join_one() - One-to-One (LEFT JOIN) ```python # Each flight has exactly one carrier diff --git a/docs/md/skills/cursor/bsl-model-builder.mdc b/docs/md/skills/cursor/bsl-model-builder.mdc index 05efa062..9cfd4ca6 100644 --- a/docs/md/skills/cursor/bsl-model-builder.mdc +++ b/docs/md/skills/cursor/bsl-model-builder.mdc @@ -110,7 +110,7 @@ flights_with_carriers = flights_st.join_many( ) ``` -### join_one() - One-to-One (INNER JOIN) +### join_one() - One-to-One (LEFT JOIN) ```python # Each flight has exactly one carrier diff --git a/src/boring_semantic_layer/agents/tools.py b/src/boring_semantic_layer/agents/tools.py index e47ba7ae..7f739f23 100644 --- a/src/boring_semantic_layer/agents/tools.py +++ b/src/boring_semantic_layer/agents/tools.py @@ -18,6 +18,23 @@ from boring_semantic_layer.utils import safe_eval +def _models_ibis_module(models: dict) -> Any: + """Return the ibis module matching the models' table flavor. + + Model tables are converted to xorq's vendored ibis at construction when + xorq is installed; query strings evaluated against them must build + literals/expressions with the same flavor (``ibis.literal``, + ``ibis.cases``, ...) or comparisons silently mis-compose. + """ + from boring_semantic_layer.nested_compile import get_ibis_module + + for model in models.values(): + table = getattr(model, "table", None) + if table is not None: + return get_ibis_module(table) + return ibis + + @cache def _get_md_dir() -> Path: """Get the directory containing markdown documentation files. @@ -282,14 +299,18 @@ def _query_model( chart_format: str | None = None, chart_spec: dict | None = None, ) -> str: - from ibis import _ from returns.result import Failure, Success # Extract model name for error context model_name = self._extract_model_name(query) try: - result = safe_eval(query, context={**self.models, "ibis": ibis, "_": _}) + # Match the models' ibis flavor so agent-built literals + # (ibis.literal, ibis.cases, ...) compose with the tables. + ibis_module = _models_ibis_module(self.models) + result = safe_eval( + query, context={**self.models, "ibis": ibis_module, "_": ibis_module._} + ) if isinstance(result, Failure): raise result.failure() query_result = result.unwrap() if isinstance(result, Success) else result diff --git a/src/boring_semantic_layer/api.py b/src/boring_semantic_layer/api.py index b3564c20..a7df097f 100644 --- a/src/boring_semantic_layer/api.py +++ b/src/boring_semantic_layer/api.py @@ -58,7 +58,7 @@ def join_one( on: Join predicate. Accepts a lambda ``(left, right) -> bool``, a column name string, a Deferred ``_.col``, or a list of strings/Deferred for compound equi-joins. - how: Join type - "left", "inner", "right", or "outer" (default: "left") + how: Join type. Only ``"left"`` is supported. Returns: Joined SemanticModel @@ -85,7 +85,7 @@ def join_many( on: Join predicate. Accepts a lambda ``(left, right) -> bool``, a column name string, a Deferred ``_.col``, or a list of strings/Deferred for compound equi-joins. - how: Join type - "inner", "left", "right", or "outer" (default: "left") + how: Join type. Only ``"left"`` is supported. Returns: Joined SemanticModel diff --git a/src/boring_semantic_layer/calc_compiler.py b/src/boring_semantic_layer/calc_compiler.py index a6ca50b7..0c99882f 100644 --- a/src/boring_semantic_layer/calc_compiler.py +++ b/src/boring_semantic_layer/calc_compiler.py @@ -42,6 +42,7 @@ from ._xorq import operations as ibis_ops from .calc_analyzer import ( CalcExprAnalysis, + _is_empty_window, _walk, analyze_calc_expr, virtual_agg_table, @@ -82,6 +83,43 @@ class TotalsNotAvailableError(RuntimeError): """ +class WindowedBaseReductionError(RuntimeError): + """Raised when a windowed reduction over raw base columns cannot be + compiled soundly at the query's grain. + + A calc measure like ``t.amount.sum().over(window(group_by=t.region))`` + reads base columns directly, so the window must be re-expressed over + the aggregated result. That is only possible when the reduction is + decomposable (sum/count/min/max/any/all) and every column the window + partitions or orders by is a group key of the query. Anything else + would silently produce wrong numbers, so it raises this error with a + pointer to the measure-reference form (define a base measure, then + window over the measure name), which compiles at the output grain. + """ + + +_WINDOW_REAGG_METHODS: dict[type, str] = { + cls: method + for cls_name, method in ( + # Decomposable reductions: the windowed value over base rows can + # be recovered by re-aggregating the lifted per-group value over + # the output rows. Count re-aggregates with sum (sum of per-group + # counts). Exact-type keys: subclasses (e.g. CountDistinct) are + # NOT decomposable and must not inherit a mapping. + ("Sum", "sum"), + ("Count", "sum"), + ("CountStar", "sum"), + ("Min", "min"), + ("Max", "max"), + ("Any", "any"), + ("All", "all"), + ) + if (cls := getattr(ibis_ops, cls_name, None)) is not None +} +"""Reduction op type → re-aggregation method used to rewrite a non-empty +window over a lifted base reduction onto the aggregated result.""" + + def _to_op(x): """Return ``x.op()`` if ``x`` is an ibis expression-like, else ``x``. @@ -953,7 +991,9 @@ def _topological_order( return topological_order_from_deps(calc_lambdas, deps) -def lift_inline_reductions(expr, virtual_agg_tbl, base_tbl, totals_virtual_agg_tbl=None): +def lift_inline_reductions( + expr, virtual_agg_tbl, base_tbl, totals_virtual_agg_tbl=None, group_keys=() +): """Lift inline reductions over the base table out of a calc expression. The user's calc lambda may contain reductions that read base-table @@ -968,12 +1008,23 @@ def lift_inline_reductions(expr, virtual_agg_tbl, base_tbl, totals_virtual_agg_t - A reduction at the top level becomes ``Field(vt, anon_name)`` — a column reference on the per-group result. - - A reduction that is the ``func`` of a ``WindowFunction`` (the - ``t.all(...)`` totals shape) becomes ``Field(totals_vt, anon_name)`` - — a reference to the same reduction computed over the full - filtered base. The compiler later substitutes ``totals_vt`` with a - real totals table cross-joined into the result, so non-sum + - A reduction that is the ``func`` of an *empty* ``WindowFunction`` + (the ``t.all(...)`` totals shape) becomes ``Field(totals_vt, + anon_name)`` — a reference to the same reduction computed over the + full filtered base. The compiler later substitutes ``totals_vt`` + with a real totals table cross-joined into the result, so non-sum reductions (mean/quantile/…) get correct overall values. + - A reduction under a *non-empty* window (``group_by=``/``order_by=``) + is re-expressed over the aggregated result: the per-group lifted + value is re-aggregated inside the same window with its partition + and order keys remapped from base columns to the corresponding + output columns. This is only sound when the reduction is + decomposable and every window key is one of ``group_keys``; + anything else raises :class:`WindowedBaseReductionError` rather + than silently flattening the window to the grand total. + + ``group_keys`` is the set of group-by column names of the enclosing + aggregation; it gates the non-empty-window rewrite above. Returns ``(rewritten_expr, new_vt, new_totals_vt, lifted)`` where ``lifted`` maps anonymous names to the original scalar reduction @@ -997,9 +1048,20 @@ def lift_inline_reductions(expr, virtual_agg_tbl, base_tbl, totals_virtual_agg_t if Reduction is None: return expr, virtual_agg_tbl, totals_virtual_agg_tbl, {} + Relation = getattr(ibis_ops.relations, "Relation", None) + def is_base_reduction(node): if not isinstance(node, Reduction): return False + # CountStar-style reductions hold the relation as a direct argument + # (no Field child): ``t.count()`` is ``CountStar(base)``. Walking for + # Fields alone misses them, leaving the reduction unlifted and the + # ``t.all(t.count())`` totals shape without a totals column. + if Relation is not None and any( + isinstance(a, Relation) and id(a) == id(base_op) + for a in getattr(node, "__args__", ()) + ): + return True for c in _walk(node): if isinstance(c, Field) and id(c.rel) == id(base_op): return True @@ -1044,6 +1106,7 @@ def is_base_reduction(node): # value). ``op.replace`` dedupes by equality, so we can't tell those # apart in one pass: handle WindowFunctions first, then the bare # Reductions. + group_key_set = frozenset(group_keys) if WindowFunction is not None: window_subs: dict = {} for n in _walk(op): @@ -1053,7 +1116,86 @@ def is_base_reduction(node): if inner is None or id(inner) not in reduction_to_name: continue anon = reduction_to_name[id(inner)] - window_subs[n] = Field(new_totals_vt_op, anon) + if _is_empty_window(n): + # The ``t.all(...)`` grand-totals shape. One exception: + # when the window IS the whole calc expression and the + # reduction reads only group-key columns, "totals over + # base rows" (``t.all`` semantics) and "window over the + # aggregated output rows" (post-aggregation ``mutate`` + # semantics) are both plausible readings that produce + # different numbers — refuse to guess. + reduction_cols = { + c.name + for c in _walk(inner) + if isinstance(c, Field) and id(c.rel) == id(base_op) + } + if ( + id(n) == id(op) + and reduction_cols + and reduction_cols <= group_key_set + ): + cols = ", ".join(sorted(reduction_cols)) + raise WindowedBaseReductionError( + f"An empty window over a reduction of group-key " + f"column(s) ({cols}) is ambiguous: it could mean " + "the total over the base rows (t.all semantics) or " + "a window over the aggregated output rows. Use " + "t.all(...) explicitly for base-row totals, or " + "window over a measure reference (e.g. " + "t..count().over(window())) for " + "output-grain values." + ) + window_subs[n] = Field(new_totals_vt_op, anon) + continue + + # Non-empty window over a base-column reduction: re-aggregate + # the lifted per-group value over the output rows inside the + # same window, with partition/order keys remapped to output + # columns. Sound only for decomposable reductions whose + # window keys are all group keys of this aggregation. + reagg_method = _WINDOW_REAGG_METHODS.get(type(inner)) + if reagg_method is None: + raise WindowedBaseReductionError( + f"Cannot compile {type(inner).__name__}(...) over a " + "raw base column inside a partitioned/ordered window: " + "the reduction is not decomposable, so it cannot be " + "recomputed at the query's grain. Define it as a base " + "measure and window over the measure name instead " + "(e.g. with_measures(m=lambda t: t.col.mean()) then " + "t.m.mean().over(window(...)))." + ) + key_subs: dict = {} + key_pieces = list(n.group_by) + list(n.order_by) + key_pieces += [b for b in (n.start, n.end) if b is not None] + for piece in key_pieces: + for f in _walk(piece): + if not isinstance(f, Field): + continue + if id(f.rel) == id(base_op) and f.name in group_key_set: + key_subs[f] = Field(new_vt_op, f.name) + else: + raise WindowedBaseReductionError( + f"Window key {f.name!r} is not a group key of " + "this aggregation, so a window over a raw " + "base-column reduction cannot be computed at " + "the query's grain " + f"(group keys: {sorted(group_key_set)!r}). " + "Add it to group_by, or define a base measure " + "and window over the measure name instead." + ) + + def _remap(piece, _subs=key_subs): + return piece.replace(_subs) if _subs else piece + + reagg_func = getattr(new_vt[anon], reagg_method)().op() + window_subs[n] = WindowFunction( + reagg_func, + how=n.how, + start=None if n.start is None else _remap(n.start), + end=None if n.end is None else _remap(n.end), + group_by=tuple(_remap(g) for g in n.group_by), + order_by=tuple(_remap(s) for s in n.order_by), + ) intermediate = op.replace(window_subs) if window_subs else op else: intermediate = op diff --git a/src/boring_semantic_layer/expr.py b/src/boring_semantic_layer/expr.py index 2c9af93a..38a04a58 100644 --- a/src/boring_semantic_layer/expr.py +++ b/src/boring_semantic_layer/expr.py @@ -1,7 +1,6 @@ from __future__ import annotations from collections.abc import Callable, Mapping, Sequence -from operator import attrgetter from typing import Any import ibis @@ -22,6 +21,7 @@ from .ops import ( Dimension, Measure, + NestAggSpec, SemanticAggregateOp, SemanticFilterOp, SemanticGroupByOp, @@ -36,6 +36,7 @@ _find_all_root_models, _get_merged_fields, _is_deferred, + _unwrap, _normalize_join_predicate, _normalize_to_name, make_bare_ref_lambda, @@ -53,6 +54,14 @@ " table.join_cross(other)" ) +_NON_LEFT_JOIN_MESSAGE = ( + "Semantic joins only support how='left'; got how={how!r}. " + "Non-left joins can silently change which left-side rows contribute to measures. " + "For inner-join semantics, use a left semantic join followed by an explicit " + "filter on a non-nullable field from the right table. Use join_cross() for a " + "Cartesian product." +) + _BLOCKED_IBIS_METHODS = [ "alias", "anti_join", @@ -120,6 +129,17 @@ def to_untagged(expr): raise TypeError(f"Cannot convert {type(expr)} to Ibis expression") +def _flatten_group_keys(keys: tuple) -> tuple: + """Flatten list/tuple arguments so ``group_by(["a", "b"])`` works like ibis.""" + flat: list = [] + for k in keys: + if isinstance(k, (list, tuple)): + flat.extend(k) + else: + flat.append(k) + return tuple(flat) + + def to_tagged(expr, aggregate_cache_storage=None): from .serialization import to_tagged as _to_tagged @@ -207,7 +227,7 @@ def filter(self, predicate: Callable) -> SemanticFilter: return SemanticFilter(source=self.op(), predicate=predicate) def group_by(self, *keys: str | Deferred): - normalized = tuple(_normalize_to_name(k) for k in keys) + normalized = tuple(_normalize_to_name(k) for k in _flatten_group_keys(keys)) return SemanticGroupBy(source=self.op(), keys=normalized) def aggregate(self, *measure_names, nest: dict[str, Callable] | None = None, **aliased): @@ -699,7 +719,7 @@ def with_measures(self, **meas) -> SemanticModel: for name, fn_or_expr in meas.items(): kind, value = _classify_measure(fn_or_expr, scope, name) - (new_calc_meas if kind == "calc" else new_base_meas)[name] = value + _store_classified_measure(name, kind, value, new_base_meas, new_calc_meas) return SemanticModel( table=self.op().table, @@ -729,7 +749,7 @@ def join_one( on: Join predicate. Accepts a lambda ``(left, right) -> bool``, a column name string, a Deferred ``_.col``, or a list of strings/Deferred for compound equi-joins. - how: Join type - "left", "inner", "right", or "outer" (default: "left") + how: Join type. Only ``"left"`` is supported. Returns: SemanticJoin: The joined semantic model @@ -756,7 +776,7 @@ def join_many( on: Join predicate. Accepts a lambda ``(left, right) -> bool``, a column name string, a Deferred ``_.col``, or a list of strings/Deferred for compound equi-joins. - how: Join type - "inner", "left", "right", or "outer" (default: "left") + how: Join type. Only ``"left"`` is supported. Returns: SemanticJoin: The joined semantic model @@ -911,9 +931,12 @@ def __init__( | Deferred | Sequence[str | Deferred] | None = None, - how: str = "inner", + how: str = "left", cardinality: str = "one", ) -> None: + is_cross_join = how == "cross" and cardinality == "cross" + if how != "left" and not is_cross_join: + raise ValueError(_NON_LEFT_JOIN_MESSAGE.format(how=how)) on = _normalize_join_predicate(on) op = SemanticJoinOp(left=left, right=right, on=on, how=how, cardinality=cardinality) super().__init__(op) @@ -1093,7 +1116,7 @@ def with_measures(self, **meas) -> SemanticModel: ) for name, fn_or_expr in meas.items(): kind, value = _classify_measure(fn_or_expr, scope, name) - (new_calc if kind == "calc" else new_base)[name] = value + _store_classified_measure(name, kind, value, new_base, new_calc) return SemanticModel( table=joined_tbl, @@ -1148,7 +1171,7 @@ def join(self, *args, **kwargs): raise TypeError(_JOIN_REMOVED_MESSAGE) def group_by(self, *keys: str | Deferred): - normalized = tuple(_normalize_to_name(k) for k in keys) + normalized = tuple(_normalize_to_name(k) for k in _flatten_group_keys(keys)) return self.op().group_by(*normalized) def filter(self, predicate: Callable): @@ -1230,7 +1253,7 @@ def with_measures(self, **meas) -> SemanticModel: for name, fn_or_expr in meas.items(): kind, value = _classify_measure(fn_or_expr, scope, name) - (new_calc_meas if kind == "calc" else new_base_meas)[name] = value + _store_classified_measure(name, kind, value, new_base_meas, new_calc_meas) return SemanticModel( table=self.op().to_untagged(), @@ -1284,6 +1307,189 @@ def join(self, *args, **kwargs): raise TypeError(_JOIN_REMOVED_MESSAGE) +def _collect_struct(struct_dict: dict[str, Any], **collect_kwargs): + """Build ``struct(...).collect()`` from columns of a single ibis flavor. + + ``ibis.struct`` and ``xorq.vendor.ibis.struct`` are not interchangeable: + each can only infer types from columns of its own module. + """ + first_col = next(iter(struct_dict.values())) + if isinstance(first_col, XorqColumn): + from ._xorq import ibis as xibis + + return xibis.struct(struct_dict).collect(**collect_kwargs) + return ibis.struct(struct_dict).collect(**collect_kwargs) + + +def _make_row_struct_collector(columns: tuple[str, ...]) -> Callable: + """Per-row struct collection for the bare ``group_by`` nest form. + + Collects one struct per source row within each outer group, duplicates + included — the historical ``nest={"x": lambda t: t.group_by([...])}`` + semantics that re-grouping via nested access relies on. + """ + + def collect_rows(tbl): + return _collect_struct({col: tbl[col] for col in columns}) + + return collect_rows + + +def _split_nest_pipeline(name: str, probe_op): + """Separate an inner aggregate from pipeline steps chained after it. + + ``order_by``/``limit``/``filter`` applied after the inner ``aggregate`` + are per-group modifiers: ordering and truncation of the collected + array, and HAVING predicates evaluated at the inner grain. + """ + order_keys: tuple[Any, ...] = () + limit_spec: tuple[int, int] | None = None + having: list[Any] = [] + current = probe_op + while not isinstance(current, SemanticAggregateOp): + if isinstance(current, SemanticLimitOp): + if limit_spec is not None: + raise NotImplementedError( + f"nest entry {name!r}: multiple limit() steps are not supported." + ) + limit_spec = (current.n, current.offset) + elif isinstance(current, SemanticOrderByOp): + if order_keys: + raise NotImplementedError( + f"nest entry {name!r}: multiple order_by() steps are not supported." + ) + order_keys = tuple(current.keys) + elif isinstance(current, SemanticFilterOp): + having.append(current.predicate) + else: + raise NotImplementedError( + f"nest entry {name!r}: unsupported nested query shape " + f"{type(current).__name__}. Supported forms are t.group_by(...) " + "and t.group_by(...).aggregate(...) optionally followed by " + "filter/order_by/limit.", + ) + current = current.source + return current, order_keys, limit_spec, tuple(having) + + +def _regrain_nested_specs(aggs: dict, new_outer_keys: tuple[str, ...]) -> dict: + """Widen child ``nest=`` plans to a re-grained parent's keys. + + A nest entry's inner aggregate is re-grouped at (outer + inner) keys; + any ``nest=`` entries *it* carries were compiled against the lambda's + original grain and must widen to the new keys too, or the group-by + that joins them back onto their parent has no outer key columns. + """ + out = {} + for agg_name, agg_fn in aggs.items(): + spec = _unwrap(agg_fn) + if not isinstance(spec, NestAggSpec): + out[agg_name] = agg_fn + continue + inner = spec.inner_op + widened = tuple(new_outer_keys) + tuple( + k for k in inner.keys if k not in new_outer_keys + ) + if widened == tuple(inner.keys): + out[agg_name] = agg_fn + continue + inner_source = inner.source + if isinstance(inner_source, SemanticGroupByOp): + inner_source = inner_source.source + out[agg_name] = NestAggSpec( + inner_op=SemanticAggregateOp( + source=SemanticGroupByOp(source=inner_source, keys=widened), + keys=widened, + aggs=_regrain_nested_specs(dict(inner.aggs), widened), + nested_columns=inner.nested_columns, + ), + struct_fields=spec.struct_fields, + order_keys=spec.order_keys, + limit_spec=spec.limit_spec, + having=spec.having, + ) + return out + + +def _build_nest_agg(name: str, fn: Callable, source_op, outer_keys: tuple[str, ...]): + """Classify a ``nest=`` lambda against the semantic source. + + The lambda receives the aggregation's semantic source table, so measure + and dimension names resolve exactly like a top-level query: + + - ``t.group_by(...).aggregate(...)`` compiles as its own semantic + aggregation at (outer keys + inner keys) grain and is attached to the + outer aggregate as an array-of-structs column (:class:`NestAggSpec`). + ``filter``/``order_by``/``limit`` chained after the aggregate become + HAVING predicates, array ordering, and array truncation per outer + group. + - Bare ``t.group_by(...)`` keeps the historical per-row struct + collection over the raw source rows. + + Anything else raises ``NotImplementedError`` — silently collecting raw + rows in place of the requested query is never acceptable. + """ + probe = fn(SemanticTable(source_op)) + + if isinstance(probe, SemanticAggregate) or ( + isinstance(probe, SemanticFilter | SemanticOrderBy | SemanticLimit) + and not isinstance(probe, SemanticGroupBy) + ): + inner_op, order_keys, limit_spec, having = _split_nest_pipeline(name, probe.op()) + inner_keys = tuple(inner_op.keys) + combined = tuple(outer_keys) + tuple(k for k in inner_keys if k not in outer_keys) + inner_source = inner_op.source + if isinstance(inner_source, SemanticGroupByOp): + # Re-group the inner query at (outer + inner) grain over its own + # source chain, keeping any filters the lambda applied. + inner_source = inner_source.source + combined_op = SemanticAggregateOp( + source=SemanticGroupByOp(source=inner_source, keys=combined), + keys=combined, + aggs=_regrain_nested_specs(dict(inner_op.aggs), combined), + nested_columns=inner_op.nested_columns, + ) + struct_fields = inner_keys + tuple(n for n in inner_op.aggs if n not in inner_keys) + return NestAggSpec( + inner_op=combined_op, + struct_fields=struct_fields, + order_keys=order_keys, + limit_spec=limit_spec, + having=having, + ) + + if isinstance(probe, SemanticGroupBy): + if probe.op().source != source_op: + raise NotImplementedError( + f"nest entry {name!r}: transformations before a bare group_by " + "(e.g. filter without a following .aggregate(...)) are not " + "supported inside nest=. Add .aggregate(...) to the nested query.", + ) + return _make_row_struct_collector(tuple(probe.op().keys)) + + if isinstance(probe, SemanticTable): + raise NotImplementedError( + f"nest entry {name!r}: unsupported nested query shape " + f"{type(probe).__name__}. Supported forms are t.group_by(...) and " + "t.group_by(...).aggregate(...), optionally followed by " + "filter/order_by/limit.", + ) + + if isinstance(probe, GroupedTable | IbisGroupedTable | Table | IbisTable): + raise NotImplementedError( + f"nest entry {name!r}: the lambda returned a raw ibis expression " + f"({type(probe).__module__}.{type(probe).__name__}). Build the nested " + "query from the semantic table argument instead, e.g. " + 'nest={"x": lambda t: t.group_by("dim").aggregate("measure")}.', + ) + + raise NotImplementedError( + f"nest entry {name!r}: nest lambdas must return t.group_by(...) or " + f"t.group_by(...).aggregate(...), got " + f"{type(probe).__module__}.{type(probe).__name__}.", + ) + + class SemanticGroupBy(SemanticTable): def __init__(self, source: SemanticTableOp, keys: tuple[str, ...]) -> None: op = SemanticGroupByOp(source=source, keys=keys) @@ -1345,60 +1551,11 @@ def aggregate( aggs.update(aliased) if nest: - - def make_nest_agg(fn): - def build_struct_dict(columns, source_tbl): - return {col: source_tbl[col] for col in columns} - - def collect_struct(struct_dict): - # ibis.struct and xorq.vendor.ibis.struct are not interchangeable: - # each can only infer types from columns of its own module - first_col = next(iter(struct_dict.values())) - if isinstance(first_col, XorqColumn): - from ._xorq import ibis as xibis - - return xibis.struct(struct_dict).collect() - return ibis.struct(struct_dict).collect() - - def handle_grouped_table(result, ibis_tbl): - group_cols = tuple(map(attrgetter("name"), result.groupings)) - return collect_struct(build_struct_dict(group_cols, ibis_tbl)) - - def handle_table(result, ibis_tbl): - return collect_struct(build_struct_dict(result.columns, ibis_tbl)) - - def nest_agg(ibis_tbl): - result = fn(ibis_tbl) - - if isinstance(result, SemanticTable): - return to_untagged(result) - - if isinstance(result, GroupedTable | IbisGroupedTable): - return handle_grouped_table(result, ibis_tbl) - - if isinstance(result, Table | IbisTable): - return handle_table(result, ibis_tbl) - - raise TypeError( - f"Nest lambda must return GroupedTable, Table, or SemanticExpression, " - f"got {type(result).__module__}.{type(result).__name__}", - ) - - # Keep the semantic lambda available to the aggregate compiler. - # Treating this callable like an ordinary measure causes - # _make_base_measure() to invoke it with ColumnScope, which is - # correct for scalar measures but loses the BSL query API used by - # lambdas such as - # - # lambda t: t.group_by("carrier").aggregate("flight_count") - # - # SemanticAggregateOp lowers that query at the combined outer + - # inner grain and collects its rows. Raw ibis/xorq nest lambdas - # continue through the callable above unchanged. - nest_agg.__bsl_semantic_nest__ = fn - return nest_agg - - nest_aggs = {name: make_nest_agg(fn) for name, fn in nest.items()} + source_op = self.op().source + nest_aggs = { + name: _build_nest_agg(name, fn, source_op, self.keys) + for name, fn in nest.items() + } aggs = {**aggs, **nest_aggs} nested_columns = tuple(nest.keys()) else: @@ -1711,7 +1868,7 @@ def with_measures(self, **meas) -> SemanticModel: for name, fn_or_expr in meas.items(): kind, value = _classify_measure(fn_or_expr, scope, name) - (new_calc_meas if kind == "calc" else new_base_meas)[name] = value + _store_classified_measure(name, kind, value, new_base_meas, new_calc_meas) return SemanticModel( table=self, @@ -1721,6 +1878,24 @@ def with_measures(self, **meas) -> SemanticModel: ) +def _store_classified_measure(name, kind, value, base_meas, calc_meas): + """Store a classified measure, evicting a same-named entry of the other + kind. Measure lookup is base-first, so a base measure left behind when a + redefinition lands in the calc map would silently keep serving the old + definition.""" + if kind == "calc": + if name in getattr(value, "depends_on", ()): + raise ValueError( + f"Measure {name!r} cannot be defined in terms of itself. " + "Reference other measures or columns, or use a new name." + ) + base_meas.pop(name, None) + calc_meas[name] = value + else: + calc_meas.pop(name, None) + base_meas[name] = value + + class SemanticProject(SemanticTable): def __init__(self, source: SemanticTableOp, fields: tuple[str, ...]) -> None: op = SemanticProjectOp(source=source, fields=fields) diff --git a/src/boring_semantic_layer/join_utils.py b/src/boring_semantic_layer/join_utils.py new file mode 100644 index 00000000..94040479 --- /dev/null +++ b/src/boring_semantic_layer/join_utils.py @@ -0,0 +1,14 @@ +"""Small helpers shared by semantic join compilation paths.""" + +from __future__ import annotations + + +def null_safe_equal(left, right): + """Return equality that also matches two NULL values. + + xorq/DataFusion currently misplans multiple ``identical_to`` join + predicates by folding an integer key into a boolean ``AND``. Expressing + the same semantics with ordinary equality and explicit NULL checks keeps + multi-key joins portable across the plain-ibis and xorq backends. + """ + return (left == right) | (left.isnull() & right.isnull()) diff --git a/src/boring_semantic_layer/nested_compile.py b/src/boring_semantic_layer/nested_compile.py index 9c77cbd7..7ea4c859 100644 --- a/src/boring_semantic_layer/nested_compile.py +++ b/src/boring_semantic_layer/nested_compile.py @@ -19,13 +19,19 @@ import ibis from toolz import curry, pipe +from .join_utils import null_safe_equal + def get_ibis_module(table): """Return the ibis module that built ``table`` (regular vs xorq-vendored). BSL coexists with both flavors of ibis. Picking the right module avoids - cross-flavor literal/struct construction errors. + cross-flavor literal/struct construction errors. Filter and dimension + callables receive resolver proxies rather than the table itself, so + unwrap those first — otherwise flavor detection would report plain ibis + for a xorq-backed table. """ + table = _unwrap_table_proxy(table) table_module = type(table).__module__ if table_module.startswith("xorq.vendor.ibis"): from ._xorq import ibis as xorq_ibis @@ -34,6 +40,20 @@ def get_ibis_module(table): return ibis +def _unwrap_table_proxy(obj): + for _ in range(8): + if not type(obj).__module__.startswith("boring_semantic_layer"): + return obj + inner = getattr(obj, "_t", None) + if inner is None: + resolver = getattr(obj, "_resolver", None) + inner = getattr(resolver, "_t", None) if resolver is not None else None + if inner is None: + return obj + obj = inner + return obj + + @curry def _extract_nested_array(prev_col: str, array_col: str, table): if prev_col not in table.columns: @@ -143,7 +163,7 @@ def join_step(left, right): # Null-safe equality: group keys can legitimately be NULL (real NULL # dim values, or keys minted by an outer join). Plain == drops those # groups from every table but the first. - predicates = [left[c].identical_to(right[c]) for c in by_cols] + predicates = [null_safe_equal(left[c], right[c]) for c in by_cols] right_cols = [c for c in right.columns if c not in by_cols_set] right_select = [right[c] for c in right_cols] return left.left_join(right, predicates).select([left] + right_select) diff --git a/src/boring_semantic_layer/ops.py b/src/boring_semantic_layer/ops.py index c83e626c..448d07d2 100644 --- a/src/boring_semantic_layer/ops.py +++ b/src/boring_semantic_layer/ops.py @@ -54,6 +54,7 @@ def _reductions_for_expr(expr): TOTALS_PREFIX, TotalsNotAvailableError, UnknownMeasureRefError, + WindowedBaseReductionError, _drop_totals_columns, _to_op, apply_calc_measures, @@ -65,6 +66,7 @@ def _reductions_for_expr(expr): topological_order_from_deps, ) from .graph_utils import walk_nodes +from .join_utils import null_safe_equal from .measure_scope import ( ColumnScope, MeasureScope, @@ -492,9 +494,38 @@ def _make_schema(fields_dict: dict[str, str]): return _SchemaClass(cleaned) +def _reject_bool_resolution(result: Any, source: Any) -> None: + """Reject expressions that resolved to a Python bool. + + A bool here almost always means a comparison mixed plain-ibis and + xorq-vendored objects (e.g. ``t.col == ibis.literal(...)`` where ``t`` + is xorq-backed): both ``__eq__`` implementations return + ``NotImplemented`` for the foreign type, so Python falls back to + identity comparison and yields a plain ``False``. Left unchecked, that + compiles into a constant predicate and silently returns wrong results. + """ + if isinstance(result, bool): + raise TypeError( + f"Expression {source!r} resolved to the Python bool {result!r} " + "instead of an ibis expression. This usually means a comparison " + "mixed plain-ibis and xorq-vendored objects (e.g. " + "`t.col == ibis.literal(...)` against a xorq-backed table). " + "Compare against plain Python values instead (`t.col == 'AA'`) " + "or build the literal with the table's own ibis flavor: " + "`from boring_semantic_layer.nested_compile import get_ibis_module; " + "get_ibis_module(t).literal(...)`. For a deliberately constant " + "predicate, return `get_ibis_module(t).literal(True)` rather " + "than a Python bool." + ) + + def _resolve_expr(expr: Deferred | Callable | Any, scope: ir.Table) -> ir.Value: + was_resolved = _is_deferred(expr) or callable(expr) result = expr.resolve(scope) if _is_deferred(expr) else expr(scope) if callable(expr) else expr + if was_resolved: + _reject_bool_resolution(result, expr) + if hasattr(result, "__class__") and hasattr(scope, "__class__"): result_module = result.__class__.__module__ scope_module = scope.__class__.__module__ @@ -532,6 +563,91 @@ def _get_merged_fields(all_roots: list, field_type: str) -> dict: ) +def _augment_dimensions_with_raw_columns( + merged_dimensions: Mapping[str, Any], + keys: Iterable[str], + all_roots: Sequence[Any], + source: Any = None, +) -> dict: + """Expose requested ``.`` group keys as auto-dimensions. + + On a single un-joined model, raw table columns are queryable without a + ``with_dimensions`` declaration. A joined table flattens to physical + columns with collision suffixes (``_right``, ``_right2``, …), so a + prefixed raw-column reference has nothing to resolve against unless a + dimension was declared. For each requested key that names a root table + and one of its raw columns, synthesize an identity dimension and run it + through the same rename-aware merge that declared dimensions use, so + collided right-side columns resolve to their suffixed physical name. + + Declared dimensions always win over synthesized ones. + """ + requested: dict[str, dict[str, Dimension]] = {} + for key in keys: + if key in merged_dimensions or "." not in key: + continue + prefix, col = key.split(".", 1) + for root in all_roots: + if root.name != prefix: + continue + cols = getattr(getattr(root, "table", None), "columns", ()) + if col in cols: + requested.setdefault(prefix, {})[col] = Dimension( + expr=lambda t, _c=col: t[_c] + ) + break + if not requested: + return dict(merged_dimensions) + synthesized = _merge_fields_with_prefixing( + all_roots, + lambda r: requested.get(r.name, {}), + source=source, + ) + return {**dict(synthesized), **dict(merged_dimensions)} + + +def _reject_unresolvable_group_keys( + keys: Iterable[str], + merged_dimensions: Mapping[str, Any], + tbl, + all_roots: Sequence[Any], +) -> None: + """Raise a semantic-layer error for group keys that resolve to nothing. + + Without this, an unknown key reaches ibis as a physical column lookup on + the joined table and fails with an error that leaks the flattened join + schema (``name_right``, ``tournament_id_right2``, …) instead of naming + the model's queryable surface. + """ + tbl_columns = frozenset(getattr(tbl, "columns", ())) + unresolved = [ + k for k in keys if k not in merged_dimensions and k not in tbl_columns + ] + if not unresolved: + return + + candidates: set[str] = set(merged_dimensions) + for root in all_roots: + cols = getattr(getattr(root, "table", None), "columns", ()) + candidates.update(cols) + if root.name: + candidates.update(f"{root.name}.{c}" for c in cols) + + suggestions = [] + for key in unresolved: + close = get_close_matches(key, sorted(candidates), n=3, cutoff=0.6) + if close: + suggestions.append(f"{key!r} (did you mean: {', '.join(map(repr, close))}?)") + else: + suggestions.append(repr(key)) + declared = ", ".join(sorted(merged_dimensions)) or "none" + raise KeyError( + f"Unknown group_by key(s): {'; '.join(suggestions)}. " + f"Declared dimensions: {declared}. Raw table columns can also be " + "referenced directly ('column' or '
.' on joins)." + ) + + def _extract_missing_column_name(exc: Exception) -> str | None: """Extract a missing column/attribute name from common resolution errors.""" message = str(exc) @@ -552,8 +668,18 @@ def _mutate_dimensions_with_dependencies( tbl: ir.Table, dimension_names: Iterable[str], merged_dimensions: Mapping[str, Any], + *, + overwrite_existing: bool = True, ) -> ir.Table: - """Mutate requested dimensions, recursively materializing derived deps first.""" + """Mutate requested dimensions, recursively materializing derived deps first. + + ``overwrite_existing=False`` leaves dimensions that share a name with an + existing column unmaterialized. Filter resolution needs this: it resolves + such dimensions through the dimension lambda against raw columns, and + materializing them first would both re-apply the expression (``amount*2`` + filtering as ``amount*4``) and hand downstream measures the mutated + column in place of the raw one. + """ resolving: list[str] = [] # Dim lambdas reference sibling dims by their BARE name (t.region_band), @@ -572,6 +698,8 @@ def _mutate_dimensions_with_dependencies( def resolve_one(dim_name: str, current_tbl: ir.Table) -> ir.Table: if dim_name not in merged_dimensions: return current_tbl + if not overwrite_existing and dim_name in current_tbl.columns: + return current_tbl if dim_name in resolving: cycle = " -> ".join([*resolving, dim_name]) raise ValueError(f"Circular dimension dependency detected: {cycle}") @@ -606,6 +734,62 @@ def resolve_one(dim_name: str, current_tbl: ir.Table) -> ir.Table: return tbl +def _reject_shadowed_group_keys( + tbl, keys, merged_dimensions, aggs, merged_base_measures, raw_columns=None +): + """Reject group keys whose dimension redefines a column a measure reads. + + Materializing such a dimension overwrites the raw column before measures + are computed, so the measure would silently aggregate the dimension's + values (e.g. ``amount * 2``) instead of the column it was defined over. + Identity dimensions (``lambda t: t.amount``) and measures that don't + touch the shadowed column are unaffected and stay allowed. + + ``raw_columns`` is the union of the root tables' own column names: a key + absent from it can only exist in ``tbl`` as an upstream materialization + of the dimension itself (e.g. by a pre-aggregation filter), so there is + no raw column to shadow and expressions reading it are well-defined + (e.g. ``mutate`` entries desugared onto the measure path). + """ + for key in keys: + if key not in merged_dimensions or key not in tbl.columns: + continue + if raw_columns is not None and key not in raw_columns: + continue + dim_fn = merged_dimensions[key] + try: + dim_expr = dim_fn(tbl) + except Exception: + continue + target = tbl[key].op() + try: + if dim_expr.op() == target: + continue + except Exception: + continue + for name, agg in aggs.items(): + measure = merged_base_measures.get(name) + try: + measure_expr = measure(tbl) if measure is not None else _unwrap(agg)(tbl) + except Exception: + continue + try: + reads_shadowed = any( + node == target for node in measure_expr.op().find(type(target)) + ) + except Exception: + continue + if reads_shadowed: + raise ValueError( + f"Group key {key!r} is a dimension that redefines column " + f"{key!r} with a different expression, and measure {name!r} " + "reads that column. Grouping would replace the column with " + "the dimension's values and silently change the measure. " + f"Rename the dimension (e.g. '{key}_bucket') or define the " + "measure against a column the dimension does not shadow." + ) + + def _classify_dependencies( fields: list, dimensions: dict, @@ -654,6 +838,62 @@ def _ensure_wrapped(fn: Any) -> _CallableWrapper: return fn if isinstance(fn, _CallableWrapper) else _CallableWrapper(fn) +class NestAggSpec: + """Compiled plan for a semantic ``nest=`` aggregation entry. + + Built by ``SemanticGroupBy.aggregate`` when a nest lambda returns a + semantic aggregation. ``inner_op`` is that aggregation re-grouped at + (outer keys + inner keys) grain — including any filters the lambda + applied — and ``struct_fields`` are the columns collected into the + array-of-structs (inner keys + inner aggregates). Pipeline steps + chained after the inner aggregate are carried as per-group modifiers: + ``having`` predicates run at the inner grain before collection, + ``order_keys`` order each group's array, and ``limit_spec`` (n, + offset) truncates it. ``SemanticAggregateOp.to_untagged`` compiles it + as its own query and joins it back to the outer aggregate on the + outer keys. + """ + + __slots__ = ("having", "inner_op", "limit_spec", "order_keys", "struct_fields") + + def __init__( + self, + inner_op: SemanticAggregateOp, + struct_fields: Iterable[str], + order_keys: Iterable[Any] = (), + limit_spec: tuple[int, int] | None = None, + having: Iterable[Any] = (), + ) -> None: + self.inner_op = inner_op + self.struct_fields = tuple(struct_fields) + self.order_keys = tuple(order_keys) + self.limit_spec = limit_spec + self.having = tuple(having) + + def __call__(self, *args, **kwargs): + # Callable so it passes the ``aggs: dict[str, Callable]`` signature + # validation, but it is a compile plan, not an aggregation lambda: + # SemanticAggregateOp.to_untagged routes it to _to_untagged_with_nest + # before any agg spec is invoked. + raise TypeError( + "NestAggSpec is not an aggregation lambda; nest= entries are " + "compiled by SemanticAggregateOp._to_untagged_with_nest", + ) + + def __repr__(self) -> str: + return ( + f"NestAggSpec(keys={self.inner_op.keys!r}, " + f"struct_fields={self.struct_fields!r})" + ) + + +def _resolve_nest_order_key(key, table): + """Resolve a nest order_by key against the compiled inner table.""" + if isinstance(key, str): + return table[key] + return _resolve_expr(_unwrap(key), ColumnScope(_tbl=table)) + + def _infer_unnest(fn: Callable, table: Any) -> tuple[str, ...]: """Infer required unnest operations from the table. @@ -955,14 +1195,14 @@ class Dimension: def __call__(self, table: ir.Table, _dims: dict | None = None) -> ir.Value: try: - return self.expr.resolve(table) if _is_deferred(self.expr) else self.expr(table) + result = self.expr.resolve(table) if _is_deferred(self.expr) else self.expr(table) except AttributeError as e: # Retry with a prefix-aware proxy for joined tables where # model prefixes are used (e.g., lambda t: t.flights.carrier) if _dims and not _is_deferred(self.expr) and callable(self.expr): try: proxy = _DimensionTableProxy(table, _dims) - return self.expr(proxy) + proxy_result = self.expr(proxy) except AttributeError as proxy_err: # Preserve explicit prefix-proxy errors (e.g. missing # "model.field") to avoid silent fallback to unprefixed @@ -972,12 +1212,18 @@ def __call__(self, table: ir.Table, _dims: dict | None = None) -> ir.Value: raise except Exception: pass + else: + _reject_bool_resolution(proxy_result, self.expr) + return proxy_result # Provide helpful error for missing columns if "'Table' object has no attribute" in str( e ) or "'Join' object has no attribute" in str(e): raise AttributeError(_format_column_error(e, table)) from e raise + else: + _reject_bool_resolution(result, self.expr) + return result def to_json(self) -> Mapping[str, Any]: base = {"description": self.description} @@ -1015,7 +1261,9 @@ class Measure: metadata: Mapping[str, Any] = field(factory=dict, eq=False, hash=False) def __call__(self, table: ir.Table) -> ir.Value: - return self.expr.resolve(table) if _is_deferred(self.expr) else self.expr(table) + result = self.expr.resolve(table) if _is_deferred(self.expr) else self.expr(table) + _reject_bool_resolution(result, self.expr) + return result @property def locality(self) -> str | None: @@ -1268,7 +1516,7 @@ def to_untagged(self): for dim_name in dim_map: try: enriched = _mutate_dimensions_with_dependencies( - enriched, [dim_name], dim_map + enriched, [dim_name], dim_map, overwrite_existing=False ) except (TypeError, KeyError, AttributeError): pass @@ -1651,6 +1899,25 @@ def _resolve_dep(ref: str) -> str | None: ) +def _make_rebindable_reduction_spec(reduction_expr, origin_op) -> Callable: + """Wrap a lifted inline reduction as an agg-spec callable. + + The reduction was built against the pre-totals base table. Field-based + reductions (``Sum(Field(base, x))``) survive on a mutated descendant via + ibis's field dereferencing, but relation-argument reductions + (``CountStar(base)``) hold the relation itself and fail the aggregate + integrity check unless rebound to the table actually being aggregated. + """ + + def spec(t, _r=reduction_expr, _origin=origin_op): + target = _to_op(t) + if target is _origin: + return _r + return _to_op(_r).replace({_origin: target}).to_expr() + + return spec + + def _compile_aggregation( base_tbl, by_cols: list[str], @@ -1730,7 +1997,11 @@ def _compile_aggregation( priority_measures=cm.prefer_known, ) new_expr, new_vt, new_totals_vt, lifted = lift_inline_reductions( - expr, vt, base_tbl, totals_virtual_agg_tbl=totals_vt + expr, + vt, + base_tbl, + totals_virtual_agg_tbl=totals_vt, + group_keys=by_cols, ) analysis = analyze_calc_expr( new_expr, @@ -1744,7 +2015,15 @@ def _compile_aggregation( needs_totals = True for anon_name, reduction_expr in lifted.items(): if anon_name not in agg_specs: - agg_specs[anon_name] = lambda t, r=reduction_expr: r + agg_specs[anon_name] = _make_rebindable_reduction_spec( + reduction_expr, base_op + ) + except WindowedBaseReductionError: + # The apply-time fallback re-evaluates the lambda against + # the aggregated result, which would silently give the + # windowed reduction different (output-grain) semantics — + # surface the soundness error instead. + raise except Exception as exc: logger.debug( "calc-measure lift/classify failed for %r; will re-evaluate " @@ -1943,7 +2222,11 @@ def _compile_aggregation( ) rewritten_expr, rewritten_vt, rewritten_totals_vt, lifted = ( lift_inline_reductions( - expr0, vt0, base_tbl, totals_virtual_agg_tbl=totals_vt0 + expr0, + vt0, + base_tbl, + totals_virtual_agg_tbl=totals_vt0, + group_keys=by_cols, ) ) if lifted: @@ -2476,7 +2759,7 @@ def _key_covers_entity(entity_name): def _left_join_bridge(left, bridge, common_keys): """Left-join *bridge* onto *left*, selecting only new columns from bridge.""" # Null-safe equality so NULL-valued keys still pair up - preds = [left[c].identical_to(bridge[c]) for c in common_keys] + preds = [null_safe_equal(left[c], bridge[c]) for c in common_keys] bridge_only = tuple(c for c in bridge.columns if c not in frozenset(common_keys)) return left.left_join(bridge, preds).select([left] + [bridge[c] for c in bridge_only]) @@ -2647,7 +2930,7 @@ def _exact_grain_preagg(raw_tbl, tbl, group_by_cols, join_keys, exact_measures): bridge = tbl.select( [tbl[c].name(tmp[c]) for c in group_by_cols] + [tbl[k] for k in shared_jk] ).distinct() - preds = [bridge[k].identical_to(raw_tbl[k]) for k in shared_jk] + preds = [null_safe_equal(bridge[k], raw_tbl[k]) for k in shared_jk] joined = bridge.inner_join(raw_tbl, preds) aggs = {m: fn(joined) for m, fn in exact_measures.items()} pt = joined.group_by([joined[t] for t in tmp.values()]).aggregate(**aggs) @@ -2783,9 +3066,13 @@ def required_columns(self) -> dict[str, set[str]]: return combined.to_dict() def to_untagged(self): - semantic_nest_result = self._lower_semantic_nests() - if semantic_nest_result is not None: - return semantic_nest_result + nest_specs = { + name: _unwrap(fn) + for name, fn in self.aggs.items() + if isinstance(_unwrap(fn), NestAggSpec) + } + if nest_specs: + return self._to_untagged_with_nest(nest_specs) all_roots = _find_all_root_models(self.source) @@ -2946,11 +3233,31 @@ def collect_filters_to_join(node): root_dimensions, ) + if not is_post_agg: + raw_columns = set() + for root in all_roots: + cols = getattr(getattr(root, "table", None), "columns", ()) + raw_columns.update(cols) + if root.name: + raw_columns.update(f"{root.name}.{c}" for c in cols) + _reject_shadowed_group_keys( + tbl, + self.keys, + merged_dimensions, + self.aggs, + merged_base_measures, + raw_columns=raw_columns, + ) + merged_dimensions = _augment_dimensions_with_raw_columns( + merged_dimensions, self.keys, all_roots, join_op + ) tbl = _mutate_dimensions_with_dependencies( tbl, [k for k in self.keys if k in merged_dimensions], merged_dimensions, ) + if not is_post_agg: + _reject_unresolvable_group_keys(self.keys, merged_dimensions, tbl, all_roots) scope = ( ColumnScope(_tbl=tbl) @@ -2984,151 +3291,77 @@ def collect_filters_to_join(node): is_post_agg=is_post_agg, ) - def _lower_semantic_nests(self): - """Lower nest lambdas which return a BSL aggregate pipeline. - - A nest is correlated with this aggregate's grouping keys. The inner - query therefore has to run at ``outer keys + inner keys`` grain before - its rows are collected into an array of structs. Invoking the lambda - as a scalar measure cannot express that correlation and, historically, - also passed a :class:`ColumnScope` where the BSL query API was expected. - - Return ``None`` when every nest lambda is a raw ibis/xorq lambda; that - preserves the original lightweight struct-collect implementation. + def _to_untagged_with_nest(self, nest_specs: dict[str, NestAggSpec]): + """Compile ``nest=`` aggregate entries and join them to the outer result. + + Each nest spec compiles as its own semantic aggregation at + (outer keys + inner keys) grain — measure and dimension names + resolve exactly like a top-level query. Its rows are collected + into one array-of-structs per outer group and attached to the + outer aggregate with a null-safe left join on the outer keys, so + outer groups the inner query filtered away keep a NULL array + instead of disappearing. HAVING predicates run at the inner grain + before collection; ``order_by``/``limit`` order and truncate each + group's array. """ - from .expr import SemanticTable - - marked: dict[str, Any] = {} - regular: dict[str, Any] = {} - for name, wrapped in self.aggs.items(): - fn = _unwrap(wrapped) - semantic_fn = getattr(fn, "__bsl_semantic_nest__", None) - if semantic_fn is None: - regular[name] = fn - continue - - # group_by().aggregate() stores the SemanticGroupByOp as the - # aggregate source. Nested pipelines should start from the same - # ungrouped semantic source, not from that bookkeeping wrapper. - base_source = ( - self.source.source - if isinstance(self.source, SemanticGroupByOp) - else self.source - ) - try: - nested = semantic_fn(SemanticTable(base_source)) - except (AttributeError, TypeError, NotImplementedError): - # The established raw-table form (notably - # ``t.group_by(["a", "b"])``) is intentionally not valid BSL - # syntax. Leave it on the old path. - regular[name] = fn - continue - if not isinstance(nested, SemanticTable): - regular[name] = fn - continue - marked[name] = nested.op() + from .expr import _collect_struct - if not marked: - return None - - # Compile the outer query without the nest measures. Reusing a normal - # SemanticAggregateOp keeps joins, filters, calculated measures, and - # fan-out-safe aggregation on their existing paths. - outer_op = SemanticAggregateOp( - source=self.source, - keys=self.keys, - aggs=regular, - nested_columns=(), - ) - result = outer_op.to_untagged() - - for output_name, pipeline_op in marked.items(): - inner_agg, order_keys, limit_spec, predicates = self._split_nest_pipeline( - pipeline_op, output_name - ) - fine_keys = tuple(dict.fromkeys((*self.keys, *inner_agg.keys))) - fine_op = SemanticAggregateOp( - source=inner_agg.source, - keys=fine_keys, - aggs={name: _unwrap(fn) for name, fn in inner_agg.aggs.items()}, - nested_columns=(), + plain_aggs = {name: fn for name, fn in self.aggs.items() if name not in nest_specs} + outer_keys = list(self.keys) + result = None + if outer_keys or plain_aggs: + outer_op = SemanticAggregateOp( + source=self.source, + keys=self.keys, + aggs=plain_aggs, + nested_columns=tuple(n for n in self.nested_columns if n not in nest_specs), ) - fine = fine_op.to_untagged() - - # Filters above the inner aggregate are HAVING predicates and must - # run at the fine grain, before collection. - for predicate in reversed(predicates): - fine = fine.filter(_resolve_expr(_unwrap(predicate), ColumnScope(_tbl=fine))) + result = outer_op.to_untagged() - struct_cols = tuple(dict.fromkeys((*inner_agg.keys, *inner_agg.aggs.keys()))) - if not struct_cols: - raise TypeError( - f"Nest lambda for {output_name!r} must produce at least one column" + for name, spec in nest_specs.items(): + inner_tbl = spec.inner_op.to_untagged() + for predicate in reversed(spec.having): + inner_tbl = inner_tbl.filter( + _resolve_expr(_unwrap(predicate), ColumnScope(_tbl=inner_tbl)) ) - struct_values = {col: fine[col] for col in struct_cols} - first_col = next(iter(struct_values.values())) - if "xorq.vendor.ibis" in type(first_col).__module__: - from ._xorq import ibis as ibis_mod - else: - ibis_mod = ibis - collect_kwargs = {} - if order_keys: + if spec.order_keys: collect_kwargs["order_by"] = [ - self._resolve_nest_order_key(key, fine) for key in order_keys + _resolve_nest_order_key(key, inner_tbl) for key in spec.order_keys ] - collected_expr = ibis_mod.struct(struct_values).collect(**collect_kwargs) - if limit_spec is not None: - n, offset = limit_spec - collected_expr = collected_expr[offset : offset + n] - - if self.keys: - part = fine.group_by([fine[key] for key in self.keys]).aggregate( - **{output_name: collected_expr} + collected = _collect_struct( + {c: inner_tbl[c] for c in spec.struct_fields}, **collect_kwargs + ) + if spec.limit_spec is not None: + n, offset = spec.limit_spec + collected = collected[offset : offset + n] + if outer_keys: + nest_tbl = inner_tbl.group_by([inner_tbl[k] for k in outer_keys]).aggregate( + **{name: collected} ) - from .nested_compile import join_tables - - result = join_tables(self.keys, [result, part]) - else: - part = fine.aggregate(**{output_name: collected_expr}) - result = result.cross_join(part) - - wanted = [*self.keys, *self.aggs.keys()] - return result.select(*wanted) - - @staticmethod - def _split_nest_pipeline(pipeline_op, output_name): - """Return inner aggregate plus post-aggregate pipeline modifiers.""" - order_keys: tuple[Any, ...] = () - limit_spec: tuple[int, int] | None = None - predicates: list[Any] = [] - current = pipeline_op - while not isinstance(current, SemanticAggregateOp): - if isinstance(current, SemanticLimitOp): - if limit_spec is not None: - raise TypeError(f"Nest lambda for {output_name!r} has multiple limits") - limit_spec = (current.n, current.offset) - elif isinstance(current, SemanticOrderByOp): - if order_keys: - raise TypeError( - f"Nest lambda for {output_name!r} has multiple order_by steps" - ) - order_keys = current.keys - elif isinstance(current, SemanticFilterOp): - predicates.append(current.predicate) + # Temp-rename the join keys so the left join has no name + # collisions; null-safe equality keeps NULL dimension groups + # matched to their own nested rows. + tmp_keys = {f"__bsl_nest_k{i}__": k for i, k in enumerate(outer_keys)} + nest_tbl = nest_tbl.rename(tmp_keys) + tmp_for = {old: tmp for tmp, old in tmp_keys.items()} + preds = [ + null_safe_equal(result[k], nest_tbl[tmp_for[k]]) for k in outer_keys + ] + joined = result.left_join(nest_tbl, preds) + result = joined.select([*result.columns, name]) else: - raise TypeError( - f"Nest lambda for {output_name!r} must return an aggregate pipeline, " - f"got {type(current).__name__}" - ) - current = current.source - return current, order_keys, limit_spec, predicates - - @staticmethod - def _resolve_nest_order_key(key, table): - if isinstance(key, str): - return table[key] - return _resolve_expr(_unwrap(key), ColumnScope(_tbl=table)) + nest_tbl = inner_tbl.aggregate(**{name: collected}) + result = nest_tbl if result is None else result.cross_join(nest_tbl) + + # Restore the requested column order: keys, then aggregates (nest + # entries included) in declaration order. + desired = list(dict.fromkeys([*self.keys, *self.aggs.keys()])) + cols = list(result.columns) + ordered = [c for c in desired if c in cols] + [c for c in cols if c not in desired] + if ordered != cols: + result = result.select(ordered) + return result def _to_untagged_with_preagg( self, @@ -3142,6 +3375,9 @@ def _to_untagged_with_preagg( This prevents fan-out inflation when ``join_many`` is used. """ merged_dimensions = _get_merged_fields(all_roots, "dimensions") + merged_dimensions = _augment_dimensions_with_raw_columns( + merged_dimensions, self.keys, all_roots, join_op + ) merged_base_measures = _get_merged_fields(all_roots, "measures") merged_calc_measures = _get_merged_fields(all_roots, "calc_measures") group_by_cols = list(self.keys) @@ -3251,7 +3487,6 @@ def _to_untagged_with_preagg( # each leg's source tables via field provenance so legs can be pushed # row-precisely to the table they constrain. filter_legs: dict[int, list] = {} - many_side_tables: set[str] = set() if tbl_filter_exprs: leaf_types = _leaf_rel_types() base_rel_to_table: dict = {} @@ -3272,17 +3507,22 @@ def _to_untagged_with_preagg( for leg in _flatten_and_legs(expr) ] - def _collect_many_sides(node): - if isinstance(node, SemanticJoinOp): - if node.cardinality == "many": - for r in _find_all_root_models(node.right): - if getattr(r, "name", None): - many_side_tables.add(r.name) - _collect_many_sides(node.left) - _collect_many_sides(node.right) + # Tables reached through a ``join_many`` edge. Their raw rows only + # count when they participate in the join, so this set drives both + # the residual-filter-leg check and the unconditional measure-leg + # participation restriction below. + many_side_tables: set[str] = set() - if filter_legs: - _collect_many_sides(join_op) + def _collect_many_sides(node): + if isinstance(node, SemanticJoinOp): + if node.cardinality == "many": + for r in _find_all_root_models(node.right): + if getattr(r, "name", None): + many_side_tables.add(r.name) + _collect_many_sides(node.left) + _collect_many_sides(node.right) + + _collect_many_sides(join_op) # --- 2. Build aggregation plan --- if tbl is not None: @@ -3357,8 +3597,8 @@ def _collect_many_sides(node): # Push filters owned by this table onto its raw table. Filters # handled elsewhere (applied to the full joined table, or owned # by another table) reach this table via a join-key bridge. + needs_bridge = False if filter_fns: - needs_bridge = False residual_cross_legs = False for i, pred_fn in enumerate(filter_fns): if filter_owners[i] == frozenset({table_name}): @@ -3394,52 +3634,107 @@ def _collect_many_sides(node): "calls, or restate it against a single table." ) - # Filters not pushed here (cross-table, ambiguous, or owned - # by another table) restrict via join keys from the filtered - # full joined table, or from the owning table's raw table. - if needs_bridge: - jk = join_tree_info.table_join_keys.get(table_name, set()) - if tbl is not None: - shared = sorted(jk & set(raw_tbl.columns) & set(tbl.columns)) - if shared: - key_bridge = tbl.select([tbl[c] for c in shared]).distinct() - preds = [raw_tbl[c] == key_bridge[c] for c in shared] - raw_tbl = raw_tbl.inner_join(key_bridge, preds).select(raw_tbl) - else: - # Chasm fallback: restrict via each owning table's keys - for i, pred_fn in enumerate(filter_fns): - owners = filter_owners[i] - if table_name in owners or len(owners) != 1: + # Rows of a join_many table whose join keys are NULL or match no + # left-side row never appear in the LEFT JOIN output, so measure + # legs must ALWAYS be restricted to join participants — not only + # when cross-table filter routing forces a bridge. Otherwise + # grand totals and many-side-only group-bys silently count + # orphan rows the joined table can never produce, and the sum + # over groups stops matching the ungrouped total. + needs_participation = table_name in many_side_tables and bool(measures) + + # Filters not pushed here (cross-table, ambiguous, or owned + # by another table) restrict via join keys from the filtered + # full joined table, or from the owning table's raw table. + if needs_bridge or needs_participation: + jk = join_tree_info.table_join_keys.get(table_name, set()) + if tbl is not None: + shared = sorted(jk & set(raw_tbl.columns) & set(tbl.columns)) + if shared: + key_bridge = tbl.select([tbl[c] for c in shared]).distinct() + preds = [raw_tbl[c] == key_bridge[c] for c in shared] + raw_tbl = raw_tbl.inner_join(key_bridge, preds).select(raw_tbl) + elif needs_participation: + raise ValueError( + f"Measures on {table_name!r} cannot be restricted " + "to rows that participate in its join_many: no " + "join-key column is available on both the raw " + "table and the joined table. Computing them on " + "the raw table would silently count rows the " + "join can never produce." + ) + else: + # Chasm fallback: restrict participation via the raw + # keys of root-side tables that share join-key columns. + participation_bridged = not needs_participation + if needs_participation: + for root_name, card in ( + join_tree_info.table_cardinalities.items() + ): + if card != "root": continue - (owner_name,) = owners - owner_op = join_tree_info.table_ops.get(owner_name) - owner_raw = raw_tables.get(owner_name) - if owner_op is None or owner_raw is None: + root_op = join_tree_info.table_ops.get(root_name) + if root_op is None: continue - owner_raw = owner_raw.filter( - _resolve_expr( - pred_fn, - _table_filter_resolver( - owner_raw, owner_op, owner_name - ), - ) - ) - owner_jk = join_tree_info.table_join_keys.get(owner_name, set()) + try: + root_raw = _to_untagged(root_op) + except Exception: + continue + root_jk = join_tree_info.table_join_keys.get(root_name, set()) shared = sorted( - jk & owner_jk & set(raw_tbl.columns) & set(owner_raw.columns) + jk & root_jk & set(raw_tbl.columns) & set(root_raw.columns) ) if shared: - key_bridge = owner_raw.select( - [owner_raw[c] for c in shared] + key_bridge = root_raw.select( + [root_raw[c] for c in shared] ).distinct() preds = [raw_tbl[c] == key_bridge[c] for c in shared] raw_tbl = raw_tbl.inner_join(key_bridge, preds).select(raw_tbl) + participation_bridged = True + if not participation_bridged: + raise ValueError( + f"Measures on {table_name!r} cannot be restricted " + "to rows that participate in its join_many: the " + "full joined table is unavailable (chasm fallback) " + "and no join-key column is shared with the root " + "table. Computing them on the raw table would " + "silently count rows the join can never produce." + ) + # Chasm fallback: restrict via each owning table's keys + for i, pred_fn in enumerate(filter_fns): + owners = filter_owners[i] + if table_name in owners or len(owners) != 1: + continue + (owner_name,) = owners + owner_op = join_tree_info.table_ops.get(owner_name) + owner_raw = raw_tables.get(owner_name) + if owner_op is None or owner_raw is None: + continue + owner_raw = owner_raw.filter( + _resolve_expr( + pred_fn, + _table_filter_resolver( + owner_raw, owner_op, owner_name + ), + ) + ) + owner_jk = join_tree_info.table_join_keys.get(owner_name, set()) + shared = sorted( + jk & owner_jk & set(raw_tbl.columns) & set(owner_raw.columns) + ) + if shared: + key_bridge = owner_raw.select( + [owner_raw[c] for c in shared] + ).distinct() + preds = [raw_tbl[c] == key_bridge[c] for c in shared] + raw_tbl = raw_tbl.inner_join(key_bridge, preds).select(raw_tbl) table_measures = _get_field_dict(table_op, "measures") table_dims = _get_field_dict(table_op, "dimensions") raw_columns = set(raw_tbl.columns) # Build agg expressions on the raw table + measure_binding_op = _to_op(raw_tbl) agg_exprs: dict = {} _tot_exprs: dict = {} _exact_measures_t: dict = {} @@ -3584,6 +3879,23 @@ def _collect_many_sides(node): case _: grain = tuple(_local_dims) + # Materializing a derived group dimension above replaces raw_tbl + # with a Project relation. Reductions were built before that + # projection; relation-argument reductions such as CountStar keep + # pointing at the old table and fail ibis's aggregate integrity + # check. Rebind every reduction to the final relation used by the + # group-by. Field-based reductions need the same treatment for + # consistency, even though ibis can sometimes dereference them + # through a projection automatically. + final_raw_op = _to_op(raw_tbl) + if final_raw_op is not measure_binding_op: + agg_exprs = { + name: _to_op(expr) + .replace({measure_binding_op: final_raw_op}) + .to_expr() + for name, expr in agg_exprs.items() + } + if _exact_measures_t: if not has_cross_table_gb: # Local grain IS the target grain — aggregate the @@ -3731,15 +4043,27 @@ def _fanout_safe_totals(): # --- 7. Select requested columns --- available = frozenset(result.columns) - select_cols = tuple( + requested = tuple( dict.fromkeys( - c - for c in (*plan.group_by_cols, *plan.requested_measures, *plan.calc_specs.keys()) - if c in available + (*plan.group_by_cols, *plan.requested_measures, *plan.calc_specs.keys()) ) ) - if select_cols: - result = result.select([result[c] for c in select_cols]) + missing = [c for c in requested if c not in available] + if missing: + # Dropping the missing columns would return a result that silently + # ignores part of the query (e.g. cross-joined models have no + # dimension bridge, so group keys from the other side never get + # attached to a pre-aggregated measure leg). + raise ValueError( + f"Pre-aggregation could not attach requested column(s) {missing} " + f"to the result; available columns: {sorted(available)}. " + "Grouping a cross-joined model by one side's dimension while " + "aggregating the other side's measures is not supported — " + "restructure the query (e.g. join on an explicit key, or " + "aggregate each side separately and combine)." + ) + if requested: + result = result.select([result[c] for c in requested]) return result @@ -3764,6 +4088,9 @@ def _to_untagged_with_deferred_joins( deferred_names = {d.table_name for d in deferrable} merged_dimensions = _get_merged_fields(all_roots, "dimensions") + merged_dimensions = _augment_dimensions_with_raw_columns( + merged_dimensions, self.keys, all_roots, join_op + ) merged_base_measures = _get_merged_fields(all_roots, "measures") merged_calc_measures = _get_merged_fields(all_roots, "calc_measures") @@ -4002,7 +4329,7 @@ def _rejoin_one(pt): # Null-safe equality: a NULL group key (real NULL dim value, or # minted by the outer join for parents with no children) must # still match its pre-agg row - preds = [dim_bridge[c].identical_to(pt[c]) for c in common] + preds = [null_safe_equal(dim_bridge[c], pt[c]) for c in common] joined_pt = dim_bridge.left_join(pt, preds).select( [dim_bridge] + [pt[c] for c in pt_meas] ) @@ -5143,8 +5470,8 @@ class SemanticLimitOp(Relation): offset: int def __init__(self, source: Relation, n: int, offset: int = 0) -> None: - if n <= 0: - raise ValueError(f"limit must be positive, got {n}") + if n < 0: + raise ValueError(f"limit must be non-negative, got {n}") if offset < 0: raise ValueError(f"offset must be non-negative, got {offset}") super().__init__(source=Relation.__coerce__(source), n=n, offset=offset) @@ -5872,12 +6199,18 @@ def _merge_fields_with_prefixing( merged_fields = {} + # Sample the first root with declared fields — not all_roots[0] + # unconditionally. When the fact table declares no dimensions, an + # empty first sample would leave ``is_dimensions`` False, skip the + # rename map, and let a colliding right-table dimension silently + # read the LEFT table's column after the join. is_dimensions = False - if all_roots: - sample_fields = field_accessor(all_roots[0]) + for root in all_roots: + sample_fields = field_accessor(root) if sample_fields: first_val = next(iter(sample_fields.values()), None) is_dimensions = isinstance(first_val, Dimension) + break column_rename_map = {} if is_dimensions: diff --git a/src/boring_semantic_layer/predicate.py b/src/boring_semantic_layer/predicate.py index edde3cd4..a3d3843f 100644 --- a/src/boring_semantic_layer/predicate.py +++ b/src/boring_semantic_layer/predicate.py @@ -17,6 +17,7 @@ from __future__ import annotations +import datetime from collections.abc import Callable, Iterable from typing import Any, ClassVar, Literal @@ -194,6 +195,12 @@ def _require_values(spec: dict, op: str) -> tuple: values = spec.get("values") if values is None: raise ValueError(f"Operator {op!r} requires 'values' field") + if isinstance(values, (str, bytes)): + raise ValueError( + f"Operator {op!r} requires a list of values, got the string " + f"{values!r}; iterating it would match individual characters. " + f"Use 'values': [{values!r}] instead" + ) return tuple(values) @@ -203,13 +210,16 @@ def _reject_value_keys(spec: dict, op: str) -> None: def _convert_literal(value: Any, ibis_module) -> Any: - """Convert string date/timestamp values to typed ibis literals. - - Mirrors ``query.Filter._convert_filter_value``: backends like Athena - require typed date literals or fail with TYPE_MISMATCH. Returns the - value unchanged when it is not a date/timestamp string. + """Convert complete ISO date/timestamp strings to typed ibis literals. + + Backends like Athena require typed date literals or fail with + TYPE_MISMATCH. Only complete ISO dates/datetimes are coerced: ibis's + lenient parser fills fields missing from partial strings like "2024" + or "12:30" with *today's* date, so coercing them would make results + depend on the day the query runs. Other strings pass through + unchanged. """ - if not isinstance(value, str): + if not isinstance(value, str) or not _is_complete_iso_datetime(value): return value for dtype in ("timestamp", "date"): try: @@ -219,6 +229,16 @@ def _convert_literal(value: Any, ibis_module) -> Any: return value +def _is_complete_iso_datetime(value: str) -> bool: + for parse in (datetime.date.fromisoformat, datetime.datetime.fromisoformat): + try: + parse(value) + except ValueError: + continue + return True + return False + + def _field_accessor(table, name: str, *, post_agg: bool): """Resolve a field name on the table. diff --git a/src/boring_semantic_layer/query.py b/src/boring_semantic_layer/query.py index 22ab522c..023750c7 100644 --- a/src/boring_semantic_layer/query.py +++ b/src/boring_semantic_layer/query.py @@ -21,10 +21,13 @@ def _get_ibis_api(): """Return xorq's vendored ibis API if available, else plain ibis. Filter expressions built with ``ibis._`` / ``ibis.literal()`` must use the - same ibis implementation as the table they will be resolved against. Since - ``_ensure_xorq_table()`` converts tables to xorq when possible, we should - build filter expressions with xorq's ibis to match. For backends that - xorq does not support, plain ibis is used as the fallback. + same ibis implementation as the table they will be resolved against. This + helper only provides a *default* flavor for contexts where no table is in + scope yet (eager validation, value parsing without a target table). The + filter callables built in ``Filter.to_callable`` pick the flavor from the + actual table at resolve time via ``get_ibis_module`` — tables on backends + that xorq does not support stay plain ibis even when xorq is installed, + and mixing flavors silently produces wrong predicates. """ try: from ._xorq import api as xo @@ -130,9 +133,10 @@ def _validate_time_grain( if not smallest_allowed_grain: return - smallest_grain = _make_grain_id(smallest_allowed_grain) - if smallest_grain not in TIME_GRAIN_ORDER: - return + # _normalize_grain accepts both spellings ("day" and "TIME_GRAIN_DAY") + # and raises on anything else — a silently-skipped validation here would + # let queries run at grains the model forbids. + smallest_grain = _normalize_grain(smallest_allowed_grain) requested_idx = TIME_GRAIN_ORDER.index(time_grain) smallest_idx = TIME_GRAIN_ORDER.index(smallest_grain) @@ -199,20 +203,38 @@ def _convert_filter_value(self, value: Any) -> Any: return value def to_callable(self) -> Callable: - """Convert filter to callable that can be used with SemanticTable.filter().""" + """Convert filter to callable that can be used with SemanticTable.filter(). + + The ibis flavor (plain vs xorq-vendored) is picked from the table the + filter actually resolves against, not from whether xorq is importable: + on backends xorq can't wrap, the table stays plain ibis, and literals + built with the other flavor mis-compose (equality silently yields a + constant-false predicate; ordering raises TypeError). + """ from . import predicate as pred_mod + from .nested_compile import get_ibis_module from .ops import _ensure_xorq_table + def _resolve_target(t): + # Dimension-aware resolver proxies must be resolved against + # directly: _ensure_xorq_table would silently unwrap them (the + # proxy forwards .op()), resolving field names to raw columns + # and bypassing same-named dimension expressions. + if type(t).__module__.startswith("boring_semantic_layer"): + return t + return _ensure_xorq_table(t) + if isinstance(self.filter, dict): pred = pred_mod.from_dict(self.filter) - ibis_module = _get_ibis_api() def _dict_filter(t): + tbl = _resolve_target(t) + ibis_module = get_ibis_module(tbl) return pred_mod.compile( pred, ibis_module._, ibis_module=ibis_module, - ).resolve(_ensure_xorq_table(t)) + ).resolve(tbl) # Deferred resolution: columns can't be statically introspected # (see ops._dimension_only_source_table). Marked so callers can opt @@ -221,14 +243,24 @@ def _dict_filter(t): _dict_filter.__bsl_deferred_resolution__ = True return _dict_filter elif isinstance(self.filter, str): - _ibis = _get_ibis_api() - expr = safe_eval( - self.filter, - context={"_": _ibis._, "ibis": _ibis}, - ).unwrap() + filter_str = self.filter + # Validate eagerly (syntax, allowed names) so bad filter strings + # fail at build time; the flavor-matched expression is built per + # table at resolve time and memoized per ibis module. + _default = _get_ibis_api() + safe_eval(filter_str, context={"_": _default._, "ibis": _default}).unwrap() + _expr_cache: dict[int, Any] = {} def _str_filter(t): - return expr.resolve(_ensure_xorq_table(t)) + tbl = _resolve_target(t) + _ibis = get_ibis_module(tbl) + key = id(_ibis) + if key not in _expr_cache: + _expr_cache[key] = safe_eval( + filter_str, + context={"_": _ibis._, "ibis": _ibis}, + ).unwrap() + return _expr_cache[key].resolve(tbl) _str_filter.__bsl_deferred_resolution__ = True return _str_filter @@ -259,7 +291,15 @@ def _normalize_filter( @curry def _make_order_key(field: str, direction: str): """Create order key for sorting (curried).""" - return ibis.desc(field) if direction.lower() == "desc" else field + normalized = direction.lower() if isinstance(direction, str) else direction + if normalized in ("desc", "descending"): + return ibis.desc(field) + if normalized in ("asc", "ascending"): + return field + raise ValueError( + f"Invalid order_by direction {direction!r} for field {field!r}. " + "Valid directions: 'asc', 'ascending', 'desc', 'descending'" + ) def _normalize_field_name( @@ -431,7 +471,7 @@ def _build_time_range_filters(semantic_table: Any, time_dimension: str, time_ran if not isinstance(time_range, dict) or "start" not in time_range or "end" not in time_range: raise ValueError("time_range must be a dict with 'start' and 'end' keys") - from datetime import datetime + from datetime import datetime, timedelta dim_obj = semantic_table.get_dimensions().get(time_dimension) if dim_obj is None: @@ -449,12 +489,34 @@ def _build_time_range_filters(semantic_table: Any, time_dimension: str, time_ran end_dt = datetime.fromisoformat(time_range["end"]) if end_dt < start_dt: raise ValueError("time_range end must be greater than or equal to start") + + # A date-only end means "through the end of that day" (the documented + # usage: end "2000-12-31" covers the whole year). Parsing it as midnight + # and comparing <= would silently drop end-date rows with intra-day + # times, so use an exclusive bound at the next midnight instead. An end + # with an explicit time component keeps inclusive <= semantics. + end_is_date_only = _is_date_only(time_range["end"]) + if end_is_date_only: + end_bound = end_dt + timedelta(days=1) + end_filter = lambda t, dim=dim_obj, end=end_bound: dim(t) < end # noqa: E731 + else: + end_filter = lambda t, dim=dim_obj, end=end_dt: dim(t) <= end # noqa: E731 return [ lambda t, dim=dim_obj, start=start_dt: dim(t) >= start, - lambda t, dim=dim_obj, end=end_dt: dim(t) <= end, + end_filter, ] +def _is_date_only(value: str) -> bool: + from datetime import date + + try: + date.fromisoformat(value) + except ValueError: + return False + return True + + def compare_periods( semantic_table: Any, dimensions: Sequence[str] | None = None, @@ -687,7 +749,22 @@ def query( filters.extend(_build_time_range_filters(result, time_dim_name, time_range)) - # Step 1: Handle time grain transformations + # Step 1: Apply row filters — separate pre-agg (dimension) from post-agg + # (measure). Pre-agg filters must run BEFORE any grain transformation: + # they reference dimensions as the queried model defines them, and once + # the time dimension is swapped for its truncated version a range filter + # would compare truncated bucket starts instead of raw values, silently + # dropping in-range rows (and, at week grain, including out-of-range ones). + pre_agg_filters = [] + post_agg_filters = list(having or []) + for filter_spec in filters: + _split_filter(filter_spec, known_measures, model_name, pre_agg_filters, post_agg_filters) + + for filter_spec in pre_agg_filters: + filter_fn = _normalize_filter(filter_spec) + result = result.filter(filter_fn) + + # Step 2: Handle time grain transformations if time_grain and time_grains: raise ValueError( "Cannot specify both 'time_grain' and 'time_grains'. " @@ -739,16 +816,6 @@ def query( if time_dims_to_transform: result = result.with_dimensions(**time_dims_to_transform) - # Step 2: Apply filters — separate pre-agg (dimension) from post-agg (measure) - pre_agg_filters = [] - post_agg_filters = list(having or []) - for filter_spec in filters: - _split_filter(filter_spec, known_measures, model_name, pre_agg_filters, post_agg_filters) - - for filter_spec in pre_agg_filters: - filter_fn = _normalize_filter(filter_spec) - result = result.filter(filter_fn) - # Step 3: Group by and aggregate if dimensions: result = result.group_by(*dimensions) @@ -769,8 +836,11 @@ def query( order_keys = [_make_order_key(field, direction) for field, direction in order_by] result = result.order_by(*order_keys) - # Step 5: Apply limit - if limit: + # Step 5: Apply limit. `limit=0` is a real LIMIT 0 (zero rows), not + # "no limit" — truthiness would silently return the full result set. + if limit is not None: + if isinstance(limit, bool): + raise ValueError(f"limit must be an integer, got {limit!r}") result = result.limit(limit) return result diff --git a/src/boring_semantic_layer/serialization/reconstruct.py b/src/boring_semantic_layer/serialization/reconstruct.py index b65e7c1b..1b74e0f8 100644 --- a/src/boring_semantic_layer/serialization/reconstruct.py +++ b/src/boring_semantic_layer/serialization/reconstruct.py @@ -339,7 +339,9 @@ def _reconstruct_join( _validate_join_leaf(left_model, left_metadata, "left") _validate_join_leaf(right_model, right_metadata, "right") - how = metadata.get("how", "inner") + # Payloads written before ``how`` was serialized must preserve left-side + # rows rather than silently treating the relationship as an inner join. + how = metadata.get("how", "left") # Default to "many" for payloads serialized before cardinality was # emitted — join_many is a safe superset of join_one behaviour, while # the reverse silently skips pre-aggregation. (Fixes #223.) diff --git a/src/boring_semantic_layer/tests/test_adversarial_semantic_model.py b/src/boring_semantic_layer/tests/test_adversarial_semantic_model.py new file mode 100644 index 00000000..9ad64f48 --- /dev/null +++ b/src/boring_semantic_layer/tests/test_adversarial_semantic_model.py @@ -0,0 +1,824 @@ +"""An intentionally hostile, end-to-end semantic model. + +This is not a collection of isolated feature tests. It combines the failure +modes that are most likely to produce believable, incorrect BI results: + +* eleven sources at six different grains; +* a four-arm chasm plus two nested one-to-many chains; +* compound keys whose second component is deliberately non-unique; +* NULL and unmatched foreign keys at every many-side boundary; +* snowflaked dimensions reached through a fact table; +* additive, conditional, distinct, average, and calculated measures; +* cross-source filters, calculated measures, totals, and output windows. + +Every result is checked against a pandas oracle built by explicitly following +the left-join participation path. Keeping the oracle relational (instead of +copying constants from BSL output) makes this useful as a stress harness when +the fixture is extended. +""" + +from __future__ import annotations + +from dataclasses import dataclass +from functools import reduce + +import ibis +import pandas as pd +import pytest +from ibis import _ + +from boring_semantic_layer import Dimension, to_semantic_table + + +@dataclass(frozen=True) +class AdversarialCommerce: + """The joined model, its component models, and raw oracle inputs.""" + + model: object + sources: dict[str, object] + frames: dict[str, pd.DataFrame] + + +def _fixture_frames() -> dict[str, pd.DataFrame]: + """Build deterministic data with repeated compound-key components.""" + tenants = pd.DataFrame( + { + "tenant_id": [1, 2, 3], + "region": ["east", "west", None], + "market": ["enterprise", "smb", "enterprise"], + } + ) + accounts = pd.DataFrame( + { + # account_id=10 intentionally exists in two tenants. + "tenant_id": [1, 1, 1, 2, 2, 3, 99, None], + "account_id": [10, 20, 50, 10, 30, 40, 90, 91], + "tier": ["gold", "silver", "dormant", "gold", None, "platinum", "ghost", "ghost"], + "seats": [10, 5, 1, 8, 12, 20, 999, 999], + } + ) + orders = pd.DataFrame( + { + # order_id=101 also repeats across tenants. + "tenant_id": [1, 1, 1, 2, 2, 2, 3, 3, 9, 1, None], + "account_id": [10, 10, 20, 10, 30, 30, 40, 40, 99, 999, 10], + "order_id": [101, 102, 103, 101, 104, 105, 106, 107, 999, 998, 997], + "created_at": pd.to_datetime( + [ + "2025-01-05", + "2025-02-06", + "2025-01-20", + "2025-01-07", + "2025-02-08", + "2025-03-09", + "2025-03-10", + "2025-03-11", + "2025-01-01", + "2025-01-02", + "2025-01-03", + ] + ), + "status": [ + "paid", + "paid", + "cancelled", + "paid", + "paid", + "pending", + "paid", + None, + "paid", + "paid", + "paid", + ], + "gross": [ + 100.0, + 220.0, + 80.0, + 150.0, + 300.0, + 90.0, + 400.0, + 50.0, + 5_000.0, + 2_000.0, + 1_000.0, + ], + "discount": [10.0, 20.0, 0.0, 15.0, 30.0, 0.0, 40.0, 0.0, 0.0, 0.0, 0.0], + } + ) + + products = pd.DataFrame( + { + # product_id values repeat by tenant; (3, 99) is deliberately absent. + "tenant_id": [1, 1, 1, 2, 2, 2, 3, 3], + "product_id": [1, 2, 3, 1, 2, 4, 1, 2], + "category_id": [10, 20, 20, 10, 30, 30, 10, 40], + "product_name": [ + "server", + "seat", + "training", + "sensor", + "plan", + "service", + "compute", + "storage", + ], + } + ) + categories = pd.DataFrame( + { + "tenant_id": [1, 1, 2, 2, 3, 3], + "category_id": [10, 20, 10, 30, 10, 40], + "category_name": ["Hardware", "Services", "Hardware", "Software", "Hardware", None], + } + ) + + valid_order_keys = list( + orders.iloc[:8][["tenant_id", "account_id", "order_id"]].itertuples(index=False, name=None) + ) + lines_rows = [] + line_id = 1 + for order_index, (tenant_id, _account_id, order_id) in enumerate(valid_order_keys): + for offset in range(1 + order_index % 3): + product_options = { + 1: [1, 2, 3], + 2: [1, 2, 4], + 3: [1, 2, 99], + }[int(tenant_id)] + product_id = product_options[(order_index + offset) % len(product_options)] + quantity = 1 + (line_id % 4) + unit_price = float(12 + 3 * line_id) + unit_cost = float(5 + line_id) + lines_rows.append( + ( + tenant_id, + order_id, + line_id, + product_id, + quantity, + unit_price, + unit_cost, + line_id % 5 == 0, + ) + ) + line_id += 1 + # Orphans that must not leak into any aggregate. + lines_rows.extend( + [ + (1, 9_999, line_id, 1, 100, 1_000.0, 1.0, False), + (None, 101, line_id + 1, 1, 100, 1_000.0, 1.0, False), + ] + ) + lines = pd.DataFrame( + lines_rows, + columns=[ + "tenant_id", + "order_id", + "line_id", + "product_id", + "quantity", + "unit_price", + "unit_cost", + "returned", + ], + ) + + payment_rows = [] + payment_id = 1 + for order_index, (tenant_id, _account_id, order_id) in enumerate(valid_order_keys): + installments = 2 if order_index % 3 == 0 else 1 + for installment in range(installments): + amount = float(40 + 10 * order_index + 5 * installment) + payment_rows.append( + ( + tenant_id, + order_id, + payment_id, + amount, + "captured" if installment == 0 else "failed", + ) + ) + payment_id += 1 + payment_rows.extend([(9, 999, 900, 9_000.0, "captured"), (None, 101, 901, 9_000.0, "captured")]) + payments = pd.DataFrame( + payment_rows, + columns=["tenant_id", "order_id", "payment_id", "amount", "payment_status"], + ) + + refund_rows = [] + refund_id = 1 + for row_index, payment in payments.iloc[:-2].iterrows(): + if row_index % 2 == 0: + refund_rows.append( + ( + payment.tenant_id, + payment.payment_id, + refund_id, + float(3 + row_index), + "approved" if row_index % 4 else "rejected", + ) + ) + refund_id += 1 + refund_rows.extend([(9, 900, 900, 8_000.0, "approved"), (1, 9999, 901, 8_000.0, "approved")]) + refunds = pd.DataFrame( + refund_rows, + columns=["tenant_id", "payment_id", "refund_id", "amount", "refund_status"], + ) + + valid_accounts = accounts.iloc[:6] + ticket_rows = [] + ticket_id = 1 + for account_index, account in valid_accounts.iterrows(): + for offset in range(1 + account_index % 2): + ticket_rows.append( + ( + account.tenant_id, + account.account_id, + ticket_id, + ["high", "low", None][(account_index + offset) % 3], + 1 + ((ticket_id * 3) % 12), + ) + ) + ticket_id += 1 + ticket_rows.extend([(99, 90, 900, "high", 999), (1, 999, 901, "high", 999)]) + tickets = pd.DataFrame( + ticket_rows, + columns=["tenant_id", "account_id", "ticket_id", "priority", "resolution_hours"], + ) + + event_rows = [] + event_id = 1 + for ticket_index, ticket in tickets.iloc[:-2].iterrows(): + for offset in range(1 + ticket_index % 3): + event_rows.append( + ( + ticket.tenant_id, + ticket.ticket_id, + event_id, + ["opened", "reply", "closed"][offset], + 2 + event_id, + ) + ) + event_id += 1 + event_rows.extend([(99, 900, 900, "reply", 999), (1, 9999, 901, "reply", 999)]) + ticket_events = pd.DataFrame( + event_rows, + columns=["tenant_id", "ticket_id", "event_id", "event_type", "agent_minutes"], + ) + + subscription_rows = [] + subscription_id = 1 + for account_index, account in valid_accounts.iterrows(): + for offset in range(1 + (account_index % 3 == 0)): + subscription_rows.append( + ( + account.tenant_id, + account.account_id, + subscription_id, + ["active", "paused", "cancelled"][(account_index + offset) % 3], + float(1_200 + account_index * 100 + offset * 50), + ) + ) + subscription_id += 1 + subscription_rows.extend([(99, 90, 900, "active", 99_000.0), (1, 999, 901, "active", 99_000.0)]) + subscriptions = pd.DataFrame( + subscription_rows, + columns=["tenant_id", "account_id", "subscription_id", "subscription_status", "arr"], + ) + + return { + "tenants": tenants, + "accounts": accounts, + "orders": orders, + "lines": lines, + "products": products, + "categories": categories, + "payments": payments, + "refunds": refunds, + "tickets": tickets, + "ticket_events": ticket_events, + "subscriptions": subscriptions, + } + + +def build_adversarial_commerce() -> AdversarialCommerce: + """Create the ten-source model on an in-memory DuckDB connection.""" + frames = _fixture_frames() + con = ibis.duckdb.connect(":memory:") + tables = {name: con.create_table(f"nightmare_{name}", frame) for name, frame in frames.items()} + + tenants = ( + to_semantic_table(tables["tenants"], "tenants", description="Tenant grain root") + .with_dimensions( + tenant_id=Dimension(expr=lambda t: t.tenant_id, is_entity=True), + region=lambda t: t.region, + market=lambda t: t.market, + ) + .with_measures(tenant_count=_.count()) + ) + accounts = ( + to_semantic_table( + tables["accounts"], "accounts", description="Accounts repeat across tenants" + ) + .with_dimensions( + tenant_id=Dimension(expr=lambda t: t.tenant_id, is_entity=True), + account_id=Dimension(expr=lambda t: t.account_id, is_entity=True), + tier=lambda t: t.tier, + ) + .with_measures(account_count=_.count(), licensed_seats=_.seats.sum()) + ) + orders = ( + to_semantic_table(tables["orders"], "orders", description="Order fact") + .with_dimensions( + tenant_id=Dimension(expr=lambda t: t.tenant_id, is_entity=True), + account_id=Dimension(expr=lambda t: t.account_id, is_entity=True), + order_id=Dimension(expr=lambda t: t.order_id, is_entity=True), + order_month={"expr": _.created_at.truncate("M"), "is_time_dimension": True}, + order_status=_.status, + ) + .with_measures( + order_count=_.count(), + distinct_buyers=_.account_id.nunique(), + gross_revenue=_.gross.sum(), + net_revenue=(_.gross - _.discount).sum(), + paid_revenue=(_.status == "paid").ifelse(_.gross - _.discount, 0).sum(), + average_order_value=_.gross.mean(), + ) + .with_measures(discount_rate=(_.gross_revenue - _.net_revenue) / _.gross_revenue.nullif(0)) + ) + lines = ( + to_semantic_table(tables["lines"], "lines", description="Order line fact") + .with_dimensions( + tenant_id=Dimension(expr=lambda t: t.tenant_id, is_entity=True), + order_id=Dimension(expr=lambda t: t.order_id, is_entity=True), + line_id=Dimension(expr=lambda t: t.line_id, is_entity=True), + product_id=lambda t: t.product_id, + ) + .with_measures( + line_count=_.count(), + distinct_products=_.product_id.nunique(), + units=_.quantity.sum(), + line_revenue=(_.quantity * _.unit_price).sum(), + line_cost=(_.quantity * _.unit_cost).sum(), + returned_units=_.returned.ifelse(_.quantity, 0).sum(), + ) + ) + products = to_semantic_table(tables["products"], "products").with_dimensions( + tenant_id=Dimension(expr=lambda t: t.tenant_id, is_entity=True), + product_id=Dimension(expr=lambda t: t.product_id, is_entity=True), + category_id=lambda t: t.category_id, + product_name=lambda t: t.product_name, + ) + categories = to_semantic_table(tables["categories"], "categories").with_dimensions( + tenant_id=Dimension(expr=lambda t: t.tenant_id, is_entity=True), + category_id=Dimension(expr=lambda t: t.category_id, is_entity=True), + category_name=lambda t: t.category_name, + ) + payments = ( + to_semantic_table(tables["payments"], "payments", description="Payment attempt fact") + .with_dimensions( + tenant_id=Dimension(expr=lambda t: t.tenant_id, is_entity=True), + order_id=Dimension(expr=lambda t: t.order_id, is_entity=True), + payment_id=Dimension(expr=lambda t: t.payment_id, is_entity=True), + payment_status=lambda t: t.payment_status, + ) + .with_measures( + payment_count=_.count(), + captured_count=(_.payment_status == "captured").ifelse(1, 0).sum(), + collected=(_.payment_status == "captured").ifelse(_.amount, 0).sum(), + ) + ) + refunds = ( + to_semantic_table(tables["refunds"], "refunds", description="Refund fact below payments") + .with_dimensions( + tenant_id=Dimension(expr=lambda t: t.tenant_id, is_entity=True), + payment_id=Dimension(expr=lambda t: t.payment_id, is_entity=True), + refund_id=Dimension(expr=lambda t: t.refund_id, is_entity=True), + refund_status=lambda t: t.refund_status, + ) + .with_measures( + refund_count=_.count(), + approved_refunds=(_.refund_status == "approved").ifelse(1, 0).sum(), + refunded=(_.refund_status == "approved").ifelse(_.amount, 0).sum(), + ) + ) + tickets = ( + to_semantic_table(tables["tickets"], "tickets", description="Support ticket fact") + .with_dimensions( + tenant_id=Dimension(expr=lambda t: t.tenant_id, is_entity=True), + account_id=Dimension(expr=lambda t: t.account_id, is_entity=True), + ticket_id=Dimension(expr=lambda t: t.ticket_id, is_entity=True), + priority=lambda t: t.priority, + ) + .with_measures( + ticket_count=_.count(), + resolution_hours=_.resolution_hours.sum(), + sla_breaches=(_.resolution_hours > 8).ifelse(1, 0).sum(), + ) + ) + ticket_events = ( + to_semantic_table(tables["ticket_events"], "ticket_events", description="Ticket event fact") + .with_dimensions( + tenant_id=Dimension(expr=lambda t: t.tenant_id, is_entity=True), + ticket_id=Dimension(expr=lambda t: t.ticket_id, is_entity=True), + event_id=Dimension(expr=lambda t: t.event_id, is_entity=True), + event_type=lambda t: t.event_type, + ) + .with_measures(event_count=_.count(), agent_minutes=_.agent_minutes.sum()) + ) + subscriptions = ( + to_semantic_table(tables["subscriptions"], "subscriptions", description="Subscription fact") + .with_dimensions( + tenant_id=Dimension(expr=lambda t: t.tenant_id, is_entity=True), + account_id=Dimension(expr=lambda t: t.account_id, is_entity=True), + subscription_id=Dimension(expr=lambda t: t.subscription_id, is_entity=True), + subscription_status=lambda t: t.subscription_status, + ) + .with_measures( + subscription_count=_.count(), + active_subscriptions=(_.subscription_status == "active").ifelse(1, 0).sum(), + annual_recurring_revenue=(_.subscription_status == "active").ifelse(_.arr, 0).sum(), + ) + ) + + sources = { + "tenants": tenants, + "accounts": accounts, + "orders": orders, + "lines": lines, + "products": products, + "categories": categories, + "payments": payments, + "refunds": refunds, + "tickets": tickets, + "ticket_events": ticket_events, + "subscriptions": subscriptions, + } + model = ( + tenants.join_many(accounts, on="tenant_id") + .join_many(orders, on=["tenant_id", "account_id"]) + .join_many(lines, on=["tenant_id", "order_id"]) + .join_one(products, on=["tenant_id", "product_id"]) + .join_one(categories, on=["tenant_id", "category_id"]) + .join_many(payments, on=["tenant_id", "order_id"]) + .join_many(refunds, on=["tenant_id", "payment_id"]) + .join_many(tickets, on=["tenant_id", "account_id"]) + .join_many(ticket_events, on=["tenant_id", "ticket_id"]) + .join_many(subscriptions, on=["tenant_id", "account_id"]) + .with_measures( + net_cash=lambda t: t["payments.collected"] - t["refunds.refunded"], + revenue_per_account=lambda t: t["orders.net_revenue"] + / t["accounts.account_count"].nullif(0), + support_minutes_per_order=lambda t: t["ticket_events.agent_minutes"] + / t["orders.order_count"].nullif(0), + gross_margin=lambda t: (t["lines.line_revenue"] - t["lines.line_cost"]) + / t["lines.line_revenue"].nullif(0), + ) + ) + return AdversarialCommerce(model=model, sources=sources, frames=frames) + + +@pytest.fixture(scope="module") +def commerce(): + return build_adversarial_commerce() + + +def _participating_frames(frames): + """Return each fact decorated with its reachable root dimensions.""" + root_accounts = frames["accounts"].merge(frames["tenants"], on="tenant_id", how="inner") + orders = frames["orders"].merge( + root_accounts[["tenant_id", "account_id", "tier", "region", "market"]], + on=["tenant_id", "account_id"], + how="inner", + ) + lines = frames["lines"].merge( + orders[ + [ + "tenant_id", + "order_id", + "account_id", + "tier", + "region", + "market", + "status", + "created_at", + ] + ], + on=["tenant_id", "order_id"], + how="inner", + ) + payments = frames["payments"].merge( + orders[["tenant_id", "order_id", "account_id", "tier", "region", "market", "status"]], + on=["tenant_id", "order_id"], + how="inner", + ) + refunds = frames["refunds"].merge( + payments[ + [ + "tenant_id", + "payment_id", + "order_id", + "account_id", + "tier", + "region", + "market", + "status", + ] + ], + on=["tenant_id", "payment_id"], + how="inner", + ) + tickets = frames["tickets"].merge( + root_accounts[["tenant_id", "account_id", "tier", "region", "market"]], + on=["tenant_id", "account_id"], + how="inner", + ) + ticket_events = frames["ticket_events"].merge( + tickets[["tenant_id", "ticket_id", "account_id", "tier", "region", "market"]], + on=["tenant_id", "ticket_id"], + how="inner", + ) + subscriptions = frames["subscriptions"].merge( + root_accounts[["tenant_id", "account_id", "tier", "region", "market"]], + on=["tenant_id", "account_id"], + how="inner", + ) + return { + "accounts": root_accounts, + "orders": orders, + "lines": lines, + "payments": payments, + "refunds": refunds, + "tickets": tickets, + "ticket_events": ticket_events, + "subscriptions": subscriptions, + } + + +def _group_sum(frame, keys, **expressions): + work = frame.copy() + for name, expression in expressions.items(): + work[name] = expression(work) + return work.groupby(keys, dropna=False)[list(expressions)].sum().reset_index() + + +def _outer_merge(parts, keys): + return reduce(lambda left, right: left.merge(right, on=keys, how="outer"), parts) + + +def _normalize_result(frame, keys): + frame = frame.copy() + # DuckDB returns SQL NULL strings as None while pandas merges normally use + # NaN. Normalize both to one nullable-string representation before an + # exact frame comparison. + for key in keys: + if pd.api.types.is_object_dtype(frame[key].dtype): + frame[key] = frame[key].astype("string") + for column in frame.columns.difference(keys): + frame[column] = pd.to_numeric(frame[column]) + return frame.sort_values(keys, na_position="last").reset_index(drop=True) + + +def test_nightmare_model_exposes_all_sources_and_metadata(commerce): + """The fixture itself should stay difficult as the implementation evolves.""" + model = commerce.model + for source in commerce.sources: + assert any(name.startswith(f"{source}.") for name in model.dimensions + model.measures) + assert len(model.dimensions) >= 35 + assert len(model.measures) >= 30 + assert model.get_dimensions()["orders.order_month"].is_time_dimension is True + assert model.get_dimensions()["ticket_events.event_id"].is_entity is True + + +def test_grand_totals_survive_six_grains_and_four_chasm_arms(commerce): + """No source may be multiplied by any of its sibling or child facts.""" + p = _participating_frames(commerce.frames) + actual = ( + commerce.model.aggregate( + "tenants.tenant_count", + "accounts.account_count", + "accounts.licensed_seats", + "orders.order_count", + "orders.distinct_buyers", + "orders.net_revenue", + "lines.line_count", + "lines.units", + "lines.line_revenue", + "payments.payment_count", + "payments.collected", + "refunds.refund_count", + "refunds.refunded", + "tickets.ticket_count", + "tickets.sla_breaches", + "ticket_events.event_count", + "ticket_events.agent_minutes", + "subscriptions.subscription_count", + "subscriptions.annual_recurring_revenue", + "net_cash", + "revenue_per_account", + "support_minutes_per_order", + "gross_margin", + ) + .execute() + .iloc[0] + ) + + expected = { + "tenants.tenant_count": len(commerce.frames["tenants"]), + "accounts.account_count": len(p["accounts"]), + "accounts.licensed_seats": p["accounts"].seats.sum(), + "orders.order_count": len(p["orders"]), + "orders.distinct_buyers": p["orders"].account_id.nunique(), + "orders.net_revenue": (p["orders"].gross - p["orders"].discount).sum(), + "lines.line_count": len(p["lines"]), + "lines.units": p["lines"].quantity.sum(), + "lines.line_revenue": (p["lines"].quantity * p["lines"].unit_price).sum(), + "payments.payment_count": len(p["payments"]), + "payments.collected": p["payments"] + .amount.where(p["payments"].payment_status == "captured", 0) + .sum(), + "refunds.refund_count": len(p["refunds"]), + "refunds.refunded": p["refunds"] + .amount.where(p["refunds"].refund_status == "approved", 0) + .sum(), + "tickets.ticket_count": len(p["tickets"]), + "tickets.sla_breaches": (p["tickets"].resolution_hours > 8).sum(), + "ticket_events.event_count": len(p["ticket_events"]), + "ticket_events.agent_minutes": p["ticket_events"].agent_minutes.sum(), + "subscriptions.subscription_count": len(p["subscriptions"]), + "subscriptions.annual_recurring_revenue": p["subscriptions"] + .arr.where(p["subscriptions"].subscription_status == "active", 0) + .sum(), + } + expected["net_cash"] = expected["payments.collected"] - expected["refunds.refunded"] + expected["revenue_per_account"] = ( + expected["orders.net_revenue"] / expected["accounts.account_count"] + ) + expected["support_minutes_per_order"] = ( + expected["ticket_events.agent_minutes"] / expected["orders.order_count"] + ) + line_cost = (p["lines"].quantity * p["lines"].unit_cost).sum() + expected["gross_margin"] = (expected["lines.line_revenue"] - line_cost) / expected[ + "lines.line_revenue" + ] + + for column, value in expected.items(): + assert float(actual[column]) == pytest.approx(float(value)), column + + +def test_grouped_multi_fact_result_matches_independent_oracle(commerce): + """Group on two ancestor dimensions while reading every fact arm.""" + keys = ["region", "tier"] + p = _participating_frames(commerce.frames) + expected = _outer_merge( + [ + _group_sum( + p["accounts"], keys, account_count=lambda x: 1, licensed_seats=lambda x: x.seats + ), + _group_sum( + p["orders"], + keys, + order_count=lambda x: 1, + net_revenue=lambda x: x.gross - x.discount, + ), + _group_sum( + p["lines"], + keys, + units=lambda x: x.quantity, + line_revenue=lambda x: x.quantity * x.unit_price, + ), + _group_sum( + p["payments"], + keys, + payment_count=lambda x: 1, + collected=lambda x: x.amount.where(x.payment_status == "captured", 0), + ), + _group_sum( + p["refunds"], + keys, + refund_count=lambda x: 1, + refunded=lambda x: x.amount.where(x.refund_status == "approved", 0), + ), + _group_sum( + p["tickets"], + keys, + ticket_count=lambda x: 1, + sla_breaches=lambda x: (x.resolution_hours > 8).astype(int), + ), + _group_sum( + p["ticket_events"], + keys, + event_count=lambda x: 1, + agent_minutes=lambda x: x.agent_minutes, + ), + _group_sum( + p["subscriptions"], + keys, + subscription_count=lambda x: 1, + annual_recurring_revenue=lambda x: x.arr.where( + x.subscription_status == "active", 0 + ), + ), + ], + keys, + ).fillna(0) + actual = ( + commerce.model.group_by("tenants.region", "accounts.tier") + .aggregate( + "accounts.account_count", + "accounts.licensed_seats", + "orders.order_count", + "orders.net_revenue", + "lines.units", + "lines.line_revenue", + "payments.payment_count", + "payments.collected", + "refunds.refund_count", + "refunds.refunded", + "tickets.ticket_count", + "tickets.sla_breaches", + "ticket_events.event_count", + "ticket_events.agent_minutes", + "subscriptions.subscription_count", + "subscriptions.annual_recurring_revenue", + ) + .execute() + .rename(columns=lambda c: c.split(".")[-1]) + .fillna(0) + ) + expected = _normalize_result(expected, keys) + actual = _normalize_result(actual, keys) + pd.testing.assert_frame_equal(actual, expected, check_dtype=False, rtol=1e-12) + + +def test_cross_source_filter_totals_and_output_windows(commerce): + """Filter through a snowflake, then calculate share/rank after aggregation.""" + p = _participating_frames(commerce.frames) + enriched_lines = ( + p["lines"] + .merge(commerce.frames["products"], on=["tenant_id", "product_id"], how="left") + .merge(commerce.frames["categories"], on=["tenant_id", "category_id"], how="left") + ) + eligible = enriched_lines[ + (enriched_lines.status == "paid") & (enriched_lines.category_name == "Hardware") + ] + expected = _group_sum( + eligible, + ["region"], + hardware_revenue=lambda x: x.quantity * x.unit_price, + hardware_units=lambda x: x.quantity, + ) + expected["revenue_share"] = expected.hardware_revenue / expected.hardware_revenue.sum() + expected["revenue_rank"] = expected.hardware_revenue.rank(method="min").astype(int) - 1 + + actual = ( + commerce.model.filter( + lambda t: (t["orders.order_status"] == "paid") + & (t["categories.category_name"] == "Hardware") + ) + .group_by("tenants.region") + .aggregate( + hardware_revenue=lambda t: t["lines.line_revenue"], + hardware_units=lambda t: t["lines.units"], + ) + .mutate(revenue_share=lambda t: t.hardware_revenue / t.all(t.hardware_revenue)) + .mutate(revenue_rank=lambda t: t.hardware_revenue.rank()) + .execute() + .rename(columns={"tenants.region": "region"}) + ) + expected = _normalize_result(expected, ["region"]) + actual = _normalize_result(actual, ["region"]) + pd.testing.assert_frame_equal(actual, expected, check_dtype=False, rtol=1e-12) + + +def test_deep_snowflake_time_grain_and_null_dimension(commerce): + """A two-fact-key/two-dimension-key query keeps unmatched and NULL dims.""" + frames = commerce.frames + truth = ( + frames["tenants"] + .merge(frames["accounts"], on="tenant_id", how="left") + .merge(frames["orders"], on=["tenant_id", "account_id"], how="left") + .merge(frames["lines"], on=["tenant_id", "order_id"], how="left") + .merge(frames["products"], on=["tenant_id", "product_id"], how="left") + .merge(frames["categories"], on=["tenant_id", "category_id"], how="left") + ) + truth["order_month"] = truth.created_at.dt.to_period("M").dt.to_timestamp() + truth["line_revenue"] = truth.quantity * truth.unit_price + expected = ( + truth.groupby(["order_month", "category_name"], dropna=False) + .agg( + line_revenue=("line_revenue", lambda x: x.sum(min_count=1)), + units=("quantity", lambda x: x.sum(min_count=1)), + ) + .reset_index() + ) + actual = ( + commerce.model.group_by("orders.order_month", "categories.category_name") + .aggregate("lines.line_revenue", "lines.units") + .execute() + .rename(columns=lambda c: c.split(".")[-1]) + ) + expected = _normalize_result(expected, ["order_month", "category_name"]) + actual = _normalize_result(actual, ["order_month", "category_name"]) + pd.testing.assert_frame_equal(actual, expected, check_dtype=False, rtol=1e-12) diff --git a/src/boring_semantic_layer/tests/test_flavor_routing.py b/src/boring_semantic_layer/tests/test_flavor_routing.py new file mode 100644 index 00000000..8dc6461c --- /dev/null +++ b/src/boring_semantic_layer/tests/test_flavor_routing.py @@ -0,0 +1,253 @@ +"""Regression tests for ibis/xorq flavor routing and cross-flavor detection. + +Two classes of defect are covered: + +1. Cross-flavor comparisons (plain-ibis literal vs xorq-vendored column) + evaluate to a Python bool via identity fallback, which used to compile + into a constant-false predicate and silently return wrong results. + ``ops._reject_bool_resolution`` now raises TypeError instead. + +2. The ibis flavor used to build filter literals was chosen by "is xorq + importable" rather than by the flavor of the table being filtered. On + backends xorq can't wrap (the table stays plain ibis), date filters + raised TypeError (ordering) or silently returned empty (equality). + ``Filter.to_callable`` now picks the flavor from the resolved table. +""" + +import ibis +import pandas as pd +import pytest + +from boring_semantic_layer import Dimension, SemanticModel +from boring_semantic_layer._xorq import HAS_XORQ + + +@pytest.fixture(scope="module") +def con(): + return ibis.duckdb.connect() + + +@pytest.fixture(scope="module") +def flights_table(con): + df = pd.DataFrame( + {"carrier": ["AA", "UA", "AA", "DL"], "dep_delay": [5.0, 10.0, 15.0, 2.0]} + ) + return con.create_table("flavor_flights", df) + + +@pytest.fixture(scope="module") +def events_table(con): + df = pd.DataFrame( + { + "d": pd.to_datetime(["2024-01-01", "2024-06-01", "2025-03-01"]), + "v": [1.0, 2.0, 4.0], + } + ) + return con.create_table("flavor_events", df) + + +def _flights_model(table): + return SemanticModel( + table=table, + dimensions={"carrier": lambda t: t.carrier}, + measures={ + "avg_delay": lambda t: t.dep_delay.mean(), + "cnt": lambda t: t.count(), + }, + name="flights", + ) + + +def _events_model(table): + return SemanticModel( + table=table, + dimensions={"d": Dimension(expr=lambda t: t.d, is_time_dimension=True)}, + measures={"total": lambda t: t.v.sum()}, + name="events", + ) + + +@pytest.fixture +def unsupported_backend(monkeypatch): + """Simulate a backend xorq can't wrap: from_ibis raises, tables stay plain.""" + import boring_semantic_layer._xorq as bsl_xorq + + def _raise(table): + raise RuntimeError("simulated backend unsupported by xorq") + + monkeypatch.setattr(bsl_xorq, "from_ibis", _raise) + + +class TestCrossFlavorBoolTrap: + """A predicate/expression resolving to a Python bool must raise, not + silently compile into a constant predicate.""" + + def test_constant_bool_filter_raises(self, flights_table): + sm = _flights_model(flights_table) + with pytest.raises(TypeError, match="resolved to the Python bool"): + sm.filter(lambda t: True).group_by("carrier").aggregate("cnt").execute() + + @pytest.mark.skipif(not HAS_XORQ, reason="cross-flavor mixing requires xorq") + def test_cross_flavor_equality_filter_raises(self, flights_table): + # Plain-ibis literal vs xorq-backed column: Python identity fallback + # yields False. Previously executed and returned an empty frame. + sm = _flights_model(flights_table) + with pytest.raises(TypeError, match="resolved to the Python bool"): + ( + sm.filter(lambda t: t.carrier == ibis.literal("AA")) + .group_by("carrier") + .aggregate("cnt") + .execute() + ) + + @pytest.mark.skipif(not HAS_XORQ, reason="cross-flavor mixing requires xorq") + def test_cross_flavor_dimension_raises(self, flights_table): + sm = SemanticModel( + table=flights_table, + dimensions={"is_aa": lambda t: t.carrier == ibis.literal("AA")}, + measures={"cnt": lambda t: t.count()}, + name="flights", + ) + with pytest.raises(TypeError, match="resolved to the Python bool"): + sm.group_by("is_aa").aggregate("cnt").execute() + + def test_plain_value_comparison_still_works(self, flights_table): + sm = _flights_model(flights_table) + out = ( + sm.filter(lambda t: t.carrier == "AA") + .group_by("carrier") + .aggregate("cnt") + .execute() + ) + assert len(out) == 1 + assert out["cnt"].iloc[0] == 2 + + +class TestFilterFlavorByTable: + """Filter literals must be built with the flavor of the table they + resolve against, not the flavor that happens to be importable.""" + + def test_dict_date_filter_converted_table(self, events_table): + sm = _events_model(events_table) + out = sm.query( + dimensions=["d"], + measures=["total"], + filters=[{"field": "d", "operator": ">=", "value": "2024-02-01"}], + ).execute() + assert len(out) == 2 + + @pytest.mark.skipif(not HAS_XORQ, reason="fallback only differs with xorq installed") + def test_dict_date_ordering_filter_fallback_backend( + self, events_table, unsupported_backend + ): + # Previously: xorq-flavored timestamp literal vs plain column -> TypeError. + sm = _events_model(events_table) + assert "xorq" not in type(sm.op().table).__module__ + out = sm.query( + dimensions=["d"], + measures=["total"], + filters=[{"field": "d", "operator": ">=", "value": "2024-02-01"}], + ).execute() + assert len(out) == 2 + + @pytest.mark.skipif(not HAS_XORQ, reason="fallback only differs with xorq installed") + def test_dict_date_equality_filter_fallback_backend( + self, events_table, unsupported_backend + ): + # Previously: silent constant-false predicate -> empty result. + sm = _events_model(events_table) + out = sm.query( + dimensions=["d"], + measures=["total"], + filters=[{"field": "d", "operator": "=", "value": "2024-01-01"}], + ).execute() + assert len(out) == 1 + assert out["total"].iloc[0] == 1.0 + + @pytest.mark.skipif(not HAS_XORQ, reason="fallback only differs with xorq installed") + def test_string_filter_with_literal_fallback_backend( + self, events_table, unsupported_backend + ): + # ibis.literal inside a string filter must use the table's flavor. + sm = _events_model(events_table) + out = sm.query( + dimensions=["d"], + measures=["total"], + filters=["_.d >= ibis.literal('2024-02-01', type='timestamp')"], + ).execute() + assert len(out) == 2 + + def test_string_filter_with_literal_converted_table(self, events_table): + sm = _events_model(events_table) + out = sm.query( + dimensions=["d"], + measures=["total"], + filters=["_.d >= ibis.literal('2024-02-01', type='timestamp')"], + ).execute() + assert len(out) == 2 + + def test_invalid_string_filter_fails_at_build_time(self): + from returns.primitives.exceptions import UnwrapFailedError + + from boring_semantic_layer.query import Filter + + with pytest.raises(UnwrapFailedError): + Filter(filter="__import__('os').system('true')").to_callable() + + +class TestAgentContextFlavor: + """Agent query contexts must expose the ibis module matching the models.""" + + def test_models_ibis_module_matches_table_flavor(self, flights_table): + from boring_semantic_layer.agents.tools import _models_ibis_module + from boring_semantic_layer.nested_compile import get_ibis_module + + sm = _flights_model(flights_table) + module = _models_ibis_module({"flights": sm}) + assert module is get_ibis_module(sm.table) + + def test_models_ibis_module_empty_falls_back_to_plain(self): + from boring_semantic_layer.agents.tools import _models_ibis_module + + assert _models_ibis_module({}) is ibis + + def test_agent_query_with_module_literal(self, flights_table): + # End-to-end shape of tools._query_model: literal comparison built + # from the flavor-matched module returns correct (non-empty) results. + from boring_semantic_layer.agents.tools import _models_ibis_module + from boring_semantic_layer.utils import safe_eval + + sm = _flights_model(flights_table) + models = {"flights": sm} + module = _models_ibis_module(models) + query = "flights.filter(_.carrier == ibis.literal('AA')).group_by('carrier').aggregate('cnt')" + result = safe_eval( + query, context={**models, "ibis": module, "_": module._} + ).unwrap() + out = result.execute() + assert len(out) == 1 + assert out["cnt"].iloc[0] == 2 + + +class TestIbisStringToExprFlavor: + """ibis_string_to_expr lambdas re-bind ``ibis`` to the flavor of the + table they are called with.""" + + def test_literal_expression_against_converted_table(self, flights_table): + from boring_semantic_layer.utils import ibis_string_to_expr + + sm = _flights_model(flights_table) + fn = ibis_string_to_expr("_.dep_delay >= ibis.literal(8.0)").unwrap() + resolved = fn(sm.table) + # Must be a real boolean expression of the table's own flavor, + # not a Python bool from identity comparison. + assert not isinstance(resolved, bool) + assert type(resolved).__module__.split(".")[0] == type(sm.table).__module__.split(".")[0] + + def test_literal_expression_against_plain_table(self, flights_table): + from boring_semantic_layer.utils import ibis_string_to_expr + + fn = ibis_string_to_expr("_.dep_delay >= ibis.literal(8.0)").unwrap() + resolved = fn(flights_table) + assert not isinstance(resolved, bool) + assert type(resolved).__module__.startswith("ibis.") diff --git a/src/boring_semantic_layer/tests/test_flights_schemas.py b/src/boring_semantic_layer/tests/test_flights_schemas.py index 8e137bd5..79617f51 100644 --- a/src/boring_semantic_layer/tests/test_flights_schemas.py +++ b/src/boring_semantic_layer/tests/test_flights_schemas.py @@ -157,14 +157,24 @@ def star(self, semantic_tables): ) def test_scalar_flight_count(self, star): - """Total flight count across the star should match raw data.""" + """Total flight count across the star matches the LEFT JOIN output. + + 14 flights originate from SCE, which is missing from the airports + table; the join can never produce them, so they must not count + (raw table has 344827) — otherwise the grand total disagrees with + the sum over any grouped query on the same model. + """ df = star.aggregate("flights.flight_count").execute() - assert df["flights.flight_count"].iloc[0] == 344827 + assert df["flights.flight_count"].iloc[0] == 344813 def test_scalar_total_distance(self, star): - """Total distance should not be inflated by join fan-out.""" + """Total distance should not be inflated by join fan-out. + + Joined truth excludes the 14 SCE-origin orphan flights + (raw table total is 255337195). + """ df = star.aggregate("flights.total_distance").execute() - assert df["flights.total_distance"].iloc[0] == 255337195 + assert df["flights.total_distance"].iloc[0] == 255331833 def test_scalar_avg_distance(self, star): """Mean distance must use sum/count decomposition correctly.""" @@ -427,9 +437,13 @@ def full_schema(self, semantic_tables): ) def test_scalar_flight_count(self, full_schema): - """344k+ flights through 4-table schema.""" + """344k+ flights through 4-table schema. + + Joined truth: the 14 SCE-origin flights match no airport row, + so the airports -< flights join can never produce them. + """ df = full_schema.aggregate("flights.flight_count").execute() - assert df["flights.flight_count"].iloc[0] == 344827 + assert df["flights.flight_count"].iloc[0] == 344813 def test_one_side_airport_count(self, full_schema): """airports.airport_count at global level.""" @@ -603,7 +617,12 @@ def star(self, semantic_tables): ) def test_all_agg_types_scalar(self, star): - """All agg types in one scalar query should be correct.""" + """All agg types in one scalar query should be correct. + + Ground truth is the LEFT JOIN output: the 14 SCE-origin orphan + flights match no airport row and never count (raw totals would + be 344827 / 255337195). + """ df = star.aggregate( "flights.flight_count", "flights.total_distance", @@ -614,9 +633,9 @@ def test_all_agg_types_scalar(self, star): "flights.max_dep_delay", ).execute() - assert df["flights.flight_count"].iloc[0] == 344827 - assert df["flights.total_distance"].iloc[0] == 255337195 - assert df["flights.avg_distance"].iloc[0] == pytest.approx(740.48, abs=0.1) + assert df["flights.flight_count"].iloc[0] == 344813 + assert df["flights.total_distance"].iloc[0] == 255331833 + assert df["flights.avg_distance"].iloc[0] == pytest.approx(740.49, abs=0.1) assert df["flights.min_dep_delay"].iloc[0] == -1133 assert df["flights.max_dep_delay"].iloc[0] == 1433 diff --git a/src/boring_semantic_layer/tests/test_grain.py b/src/boring_semantic_layer/tests/test_grain.py index d17056d1..fe285d0f 100644 --- a/src/boring_semantic_layer/tests/test_grain.py +++ b/src/boring_semantic_layer/tests/test_grain.py @@ -204,6 +204,66 @@ def test_multi_fact_scalar_aggregate(self, multi_fact_tables): # Total hours: 80+85+90+75+88+82 = 500 assert result["hours.total_hours"].iloc[0] == 500.0 + def test_derived_group_dimension_rebinds_countstar(self, con): + """Derived dimensions added during pre-agg must rebind reductions. + + CountStar stores its source relation directly. Materializing a + derived group key creates a new relation, so the reduction must be + rebound before aggregation rather than remaining attached to the + pre-mutation table. + """ + appearances_tbl = con.create_table( + "grain_derived_appearances", + pd.DataFrame( + { + "match_id": [1, 2, 3, 4], + "team_id": [10, 10, 20, 30], + "team_name": ["West Germany", "Germany", "Brazil", "Brazil"], + } + ), + ) + teams_tbl = con.create_table( + "grain_derived_teams", + pd.DataFrame( + { + "team_id": [10, 20, 30], + "region": ["Europe", "South America", "South America"], + } + ), + ) + + appearances = ( + to_semantic_table(appearances_tbl, name="appearances") + .with_dimensions( + match_id=Dimension(expr=lambda t: t.match_id, is_entity=True), + team_id=Dimension(expr=lambda t: t.team_id, is_entity=True), + canonical_team_name=lambda t: (t.team_name == "West Germany").ifelse( + "Germany", t.team_name + ), + ) + .with_measures(game_count=lambda t: t.count()) + ) + teams = ( + to_semantic_table(teams_tbl, name="teams") + .with_dimensions( + team_id=Dimension(expr=lambda t: t.team_id, is_entity=True), + region=lambda t: t.region, + ) + .with_measures(team_count=lambda t: t.count()) + ) + + with warnings.catch_warnings(): + warnings.simplefilter("ignore") + joined = appearances.join_one(teams, on="team_id") + + query = joined.group_by("appearances.canonical_team_name").aggregate( + "appearances.game_count" + ) + result = query.execute().set_index("appearances.canonical_team_name") + + assert result.loc["Germany", "appearances.game_count"] == 2 + assert result.loc["Brazil", "appearances.game_count"] == 2 + class TestMissingCardinalityDefault: """Missing cardinality in serialized metadata must default to 'many'. diff --git a/src/boring_semantic_layer/tests/test_join_pruning.py b/src/boring_semantic_layer/tests/test_join_pruning.py index 9c0bf014..7cb8fb5d 100644 --- a/src/boring_semantic_layer/tests/test_join_pruning.py +++ b/src/boring_semantic_layer/tests/test_join_pruning.py @@ -69,9 +69,7 @@ def _build_star_model(star_schema): """Build a joined model with fact + 3 dimension tables using LEFT joins. Left joins are safe for pruning — they don't filter unmatched left rows. - Chained .join_one() defaults to how="inner" which cannot be pruned - since inner joins act as filters on unmatched rows, so we pass - how="left" explicitly. + Semantic joins are always left joins, including when chained. """ facts = ( to_semantic_table(star_schema["facts"], name="facts") @@ -117,6 +115,24 @@ def _build_star_model(star_schema): ) +@pytest.mark.parametrize("join_method", ["join_one", "join_many"]) +@pytest.mark.parametrize("how", ["inner", "right", "outer", "cross"]) +def test_semantic_joins_reject_non_left_join_types(star_schema, join_method, how): + """Semantic relationships preserve left rows; cross joins use join_cross().""" + facts = to_semantic_table(star_schema["facts"], name="reject_facts") + dates = to_semantic_table(star_schema["dates"], name="reject_dates") + + with pytest.raises(ValueError, match="only support how='left'"): + getattr(facts, join_method)(dates, on="date_id", how=how) + + +def test_join_cross_remains_supported(star_schema): + facts = to_semantic_table(star_schema["facts"], name="cross_facts") + dates = to_semantic_table(star_schema["dates"], name="cross_dates") + + assert facts.join_cross(dates).how == "cross" + + class TestJoinPruningMeasureOnly: """Measure-only queries should not join any dimension tables.""" @@ -261,10 +277,10 @@ def test_pruned_matches_unpruned_grouped(self, star_schema): class TestJoinPruningEdgeCases: - """Edge cases: inner joins, orphan rows, filters, join_many.""" + """Edge cases: orphan rows, filters, and join_many.""" - def test_inner_join_not_pruned(self, con): - """Inner joins must NOT be pruned — they filter unmatched left rows.""" + def test_explicit_match_filter_not_pruned(self, con): + """An explicit right-side match filter must prevent join pruning.""" facts_with_orphan = con.create_table( "facts_orphan", pd.DataFrame( @@ -288,12 +304,14 @@ def test_inner_join_not_pruned(self, con): date_id=lambda t: t.date_id, date_name=lambda t: t.date_name ) - # Inner join: the orphan row (date_id=999) should be excluded. - # Use how="inner" — pruning must NOT remove this join. - model = f.join_one(d, on=lambda l, r: l.date_id == r.date_id, how="inner") + # Express inner semantics visibly: preserve rows in the semantic join, + # then require a non-nullable field from the right side. + model = f.join_one(d, on=lambda left, right: left.date_id == right.date_id).filter( + lambda t: t["dates_i.date_name"].notnull() + ) result = model.aggregate("facts_o.total_sales").execute() - # Inner join filters out the orphan row — total should be 10, not 110 + # The explicit filter removes the orphan row — total should be 10, not 110. assert result["facts_o.total_sales"].iloc[0] == 10.0 def test_filter_prevents_pruning(self, star_schema): diff --git a/src/boring_semantic_layer/tests/test_soundness_round4.py b/src/boring_semantic_layer/tests/test_soundness_round4.py new file mode 100644 index 00000000..555a9f30 --- /dev/null +++ b/src/boring_semantic_layer/tests/test_soundness_round4.py @@ -0,0 +1,408 @@ +"""Regression tests for the July 2026 round-4 soundness evaluation. + +Each test pins a confirmed silent-wrong-answer defect (or its loud-error +replacement) against ground truth. Finding numbers reference the round-4 +soundness report: + +- R4-2 query() time filters evaluated against grain-truncated dimensions +- R4-3 date-only time_range ends excluded intra-day end-date rows +- R4-4 in/not-in dict filters iterating bare-string values as characters +- R4-5 partial date strings coerced with today's month/day +- R4-6 dimension shadowing a raw column: double-applied lambda filters, + spelling-dependent results, measures over the mutated column +- R4-7 join_cross silently dropping requested group keys +- R4-9 base->calc measure redefinition silently serving the old measure +- R4-11 smallest_time_grain long form bypassing grain validation +- R4-12 limit=0 treated as "no limit" +- R4-13 order_by directions other than "desc" silently sorting ascending +- R4-14 bool-guard remediation unusable inside filter lambdas + +The calc-window, nest=, and join_many-bridge findings are pinned in their +own suites (test_soundness_round4_{calc,nest,join_many}.py). +""" + +import ibis +import pandas as pd +import pytest + +from boring_semantic_layer import to_semantic_table + + +@pytest.fixture +def con(): + return ibis.duckdb.connect(":memory:") + + +@pytest.fixture +def events(con): + """Timestamps spanning month boundaries with intra-day times.""" + tbl = con.create_table( + "events", + pd.DataFrame( + { + "ts": pd.to_datetime( + [ + "2025-01-05 09:00:00", + "2025-01-31 12:00:00", + "2025-02-10 08:00:00", + "2025-03-02 00:00:00", + ] + ), + "v": [1.0, 2.0, 3.0, 4.0], + } + ), + ) + return ( + to_semantic_table(tbl, name="events") + .with_dimensions(ts={"expr": lambda t: t.ts, "is_time_dimension": True}) + .with_measures(cnt=lambda t: t.count(), total=lambda t: t.v.sum()) + ) + + +@pytest.fixture +def orders(con): + tbl = con.create_table( + "orders", + pd.DataFrame( + { + "amount": [10.0, 20.0, 30.0, 40.0, 50.0, 60.0], + "qty": [1, 2, 3, 4, 5, 6], + "status": list("aabbcc"), + } + ), + ) + return tbl + + +class TestTimeRangeVsGrain: + """R4-2: range filters must compare raw values, not truncated buckets.""" + + def test_month_grain_keeps_partially_covered_buckets(self, events): + result = ( + events.query( + dimensions=["ts"], + measures=["cnt"], + time_grain="TIME_GRAIN_MONTH", + time_range={"start": "2025-01-15", "end": "2025-02-15"}, + ) + .execute() + .sort_values("ts") + ) + # Jan 31 12:00 is inside the range; its month bucket (Jan 1) is not. + # The row must survive and appear in the January bucket. + assert [str(d)[:10] for d in result["ts"]] == ["2025-01-01", "2025-02-01"] + assert result["cnt"].tolist() == [1, 1] + + def test_lambda_filter_matches_chained_spelling(self, events): + via_query = ( + events.query( + dimensions=["ts"], + measures=["cnt"], + time_grain="TIME_GRAIN_MONTH", + filters=[lambda t: (t.ts >= "2025-01-15") & (t.ts <= "2025-02-15")], + ) + .execute() + .sort_values("ts") + .reset_index(drop=True) + ) + chained = ( + events.with_dimensions(month=lambda t: t.ts.truncate("M")) + .filter(lambda t: (t.ts >= "2025-01-15") & (t.ts <= "2025-02-15")) + .group_by("month") + .aggregate("cnt") + .execute() + .sort_values("month") + .reset_index(drop=True) + ) + assert via_query["cnt"].tolist() == chained["cnt"].tolist() + assert len(via_query) == 2 + + +class TestTimeRangeEndInclusivity: + """R4-3: a date-only end covers the whole end day.""" + + def test_date_only_end_includes_intraday_rows(self, events): + result = events.query( + dimensions=["ts"], + measures=["cnt"], + time_range={"start": "2025-01-01", "end": "2025-01-31"}, + ).execute() + assert len(result) == 2 # Jan 5 and Jan 31 12:00 + + def test_explicit_time_end_stays_inclusive_at_instant(self, events): + result = events.query( + dimensions=["ts"], + measures=["cnt"], + time_range={"start": "2025-01-01", "end": "2025-01-31 11:00:00"}, + ).execute() + assert len(result) == 1 # only Jan 5 + + +class TestDictFilterValueCoercion: + def test_bare_string_values_rejected(self, con, orders): + sm = ( + to_semantic_table(orders, name="orders") + .with_dimensions(status=lambda t: t.status) + .with_measures(cnt=lambda t: t.count()) + ) + # R4-4: 'not in "gold"' previously excluded the characters g,o,l,d — + # returning exactly the rows the user asked to remove. + with pytest.raises(ValueError, match="list of values"): + sm.query( + measures=["cnt"], + filters=[{"field": "status", "operator": "in", "values": "aa"}], + ) + with pytest.raises(ValueError, match="list of values"): + sm.query( + measures=["cnt"], + filters=[{"field": "status", "operator": "not in", "values": "aa"}], + ) + + def test_partial_date_strings_not_coerced(self, events): + import duckdb + + # R4-5: "2024" was parsed with today's month/day, so results changed + # depending on the day the query ran. Now it reaches the backend as a + # plain string and fails loudly instead. + with pytest.raises(duckdb.Error, match="timestamp"): + events.query( + dimensions=["ts"], + measures=["cnt"], + filters=[{"field": "ts", "operator": ">=", "value": "2024"}], + ).execute() + + def test_full_iso_dates_still_coerced(self, events): + result = events.query( + dimensions=["ts"], + measures=["cnt"], + filters=[{"field": "ts", "operator": ">=", "value": "2025-02-01"}], + ).execute() + assert len(result) == 2 + + +class TestDimensionShadowing: + """R4-6: dimension sharing a raw column's name.""" + + @pytest.fixture + def shadowed(self, orders): + return ( + to_semantic_table(orders, name="orders") + .with_dimensions(amount=lambda t: t.amount * 2, status=lambda t: t.status) + .with_measures( + total=lambda t: t.amount.sum(), + cnt=lambda t: t.count(), + qty_sum=lambda t: t.qty.sum(), + ) + ) + + def test_filter_spellings_agree_and_apply_dimension_once(self, shadowed): + # dim value = amount*2; > 55 keeps raw rows 30,40,50,60. + # Measures aggregate RAW amounts: b -> 30+40, c -> 50+60. + expected = [["b", 70.0], ["c", 110.0]] + lam = ( + shadowed.filter(lambda t: t.amount > 55) + .group_by("status") + .aggregate("total") + .execute() + ) + dic = shadowed.query( + dimensions=["status"], + measures=["total"], + filters=[{"field": "amount", "operator": ">", "value": 55}], + ).execute() + sst = shadowed.query( + dimensions=["status"], measures=["total"], filters=["_.amount > 55"] + ).execute() + for frame in (lam, dic, sst): + assert sorted(frame.values.tolist()) == expected + + def test_unfiltered_totals_use_raw_column(self, shadowed): + result = shadowed.group_by().aggregate("total").execute() + assert result["total"].tolist() == [210.0] + + def test_group_by_shadow_dim_with_conflicting_measure_raises(self, shadowed): + with pytest.raises(ValueError, match="redefines column"): + shadowed.group_by("amount").aggregate("total").execute() + + def test_group_by_shadow_dim_with_unrelated_measures_allowed(self, shadowed): + result = ( + shadowed.group_by("amount").aggregate("cnt", "qty_sum").execute() + ).sort_values("amount") + assert result["amount"].tolist() == [20.0, 40.0, 60.0, 80.0, 100.0, 120.0] + assert result["qty_sum"].tolist() == [1, 2, 3, 4, 5, 6] + + def test_identity_dimension_group_key_still_allowed(self, orders): + sm = ( + to_semantic_table(orders, name="orders") + .with_dimensions(amount=lambda t: t.amount) + .with_measures(total=lambda t: t.amount.sum()) + ) + result = sm.group_by("amount").aggregate("total").execute() + assert sorted(result["total"].tolist()) == [10.0, 20.0, 30.0, 40.0, 50.0, 60.0] + + def test_non_raw_dim_key_with_filter_and_mutate_not_rejected(self, orders): + """A group key that shadows no raw column passes the guard. + + With a pre-aggregation filter, the dimension is materialized into + the source table before the aggregate compiles, so the key IS a + table column there — but it is the dimension's own output, not a + shadowed raw column. mutate() entries (desugared onto the measure + path) that read the group key must not trip the shadowing guard + (Malloy cohorts-query regression). + """ + sm = ( + to_semantic_table(orders, name="orders") + .with_dimensions(**{"Qty Bucket": lambda t: (t.qty > 3).ifelse("hi", "lo")}) + .with_measures(total=lambda t: t.amount.sum()) + ) + result = ( + sm.filter(lambda t: t.amount > 15) + .group_by("Qty Bucket") + .aggregate("total") + .mutate(**{"Qty Bucket": lambda t: t["Qty Bucket"].upper()}) + .execute() + ) + got = dict(zip(result["Qty Bucket"], result["total"], strict=True)) + # amount > 15 keeps rows (qty, amount): (2,20) (3,30) lo; (4,40) (5,50) (6,60) hi + assert got == {"LO": 50.0, "HI": 150.0} + + +class TestJoinCrossGroupKeys: + """R4-7: requested group keys must never be silently dropped.""" + + @pytest.fixture + def crossed(self, con): + orders = con.create_table( + "xorders", + pd.DataFrame({"status": ["open", "open", "closed"], "amount": [10.0, 20.0, 30.0]}), + ) + custs = con.create_table( + "xcusts", pd.DataFrame({"cid": [1, 2, 3, 4], "region": ["e", "e", "w", "w"]}) + ) + o = ( + to_semantic_table(orders, name="orders") + .with_dimensions(status=lambda t: t.status) + .with_measures(order_total=lambda t: t.amount.sum()) + ) + c = ( + to_semantic_table(custs, name="customers") + .with_dimensions(region=lambda t: t.region) + .with_measures(cust_count=lambda t: t.count()) + ) + return o.join_cross(c) + + def test_cross_group_key_with_other_side_measure_raises(self, crossed): + with pytest.raises(ValueError, match="could not attach"): + crossed.group_by("orders.status").aggregate("customers.cust_count").execute() + + def test_cross_grand_totals_stay_defanned(self, crossed): + result = ( + crossed.group_by() + .aggregate("orders.order_total", "customers.cust_count") + .execute() + ) + assert result["orders.order_total"].tolist() == [60.0] + assert result["customers.cust_count"].tolist() == [4] + + +class TestMeasureRedefinition: + """R4-9: redefinitions must take effect regardless of classification.""" + + @pytest.fixture + def base(self, orders): + return to_semantic_table(orders, name="orders").with_measures( + total=lambda t: t.amount.sum(), qty_sum=lambda t: t.qty.sum() + ) + + def test_base_to_calc_redefinition_wins(self, base): + redefined = base.with_measures(total=lambda t: t.qty_sum * 1000) + result = redefined.group_by().aggregate("total").execute() + assert result["total"].tolist() == [21000] + + def test_self_referential_redefinition_raises(self, base): + with pytest.raises(ValueError, match="in terms of itself"): + base.with_measures(total=lambda t: t.total / 2) + + def test_same_kind_redefinition_still_wins(self, base): + result = ( + base.with_measures(total=lambda t: t.amount.max()) + .group_by() + .aggregate("total") + .execute() + ) + assert result["total"].tolist() == [60.0] + + +class TestGrainValidation: + """R4-11: both smallest_time_grain spellings must validate.""" + + @pytest.mark.parametrize("smallest", ["day", "TIME_GRAIN_DAY"]) + def test_finer_grain_rejected(self, con, smallest): + tbl = con.create_table( + f"g_{smallest.lower()}", + pd.DataFrame({"ts": pd.to_datetime(["2025-01-01"]), "v": [1.0]}), + ) + sm = ( + to_semantic_table(tbl, name="g") + .with_dimensions( + ts={ + "expr": lambda t: t.ts, + "is_time_dimension": True, + "smallest_time_grain": smallest, + } + ) + .with_measures(cnt=lambda t: t.count()) + ) + with pytest.raises(ValueError, match="finer than the smallest"): + sm.query(dimensions=["ts"], measures=["cnt"], time_grain="TIME_GRAIN_HOUR") + + +class TestLimitAndOrderValidation: + def test_limit_zero_returns_zero_rows(self, events): + # R4-12: `if limit:` treated 0 as "no limit" and returned everything. + result = events.query(dimensions=["ts"], measures=["cnt"], limit=0).execute() + assert len(result) == 0 + + def test_bool_limit_rejected(self, events): + with pytest.raises(ValueError, match="integer"): + events.query(dimensions=["ts"], measures=["cnt"], limit=True) + + def test_descending_direction_accepted(self, events): + result = events.query( + dimensions=["ts"], measures=["total"], order_by=[("total", "descending")] + ).execute() + assert result["total"].tolist() == sorted(result["total"].tolist(), reverse=True) + + def test_unknown_direction_rejected(self, events): + # R4-13: anything != "desc" silently sorted ascending. + with pytest.raises(ValueError, match="Invalid order_by direction"): + events.query(dimensions=["ts"], measures=["cnt"], order_by=[("cnt", "dsc")]) + + +class TestBoolGuardRemediation: + """R4-14: the guard's suggested escape hatch must work where it fires.""" + + def test_flavored_constant_predicate_works_in_filter(self, orders): + from boring_semantic_layer.nested_compile import get_ibis_module + + sm = ( + to_semantic_table(orders, name="orders") + .with_dimensions(status=lambda t: t.status) + .with_measures(cnt=lambda t: t.count()) + ) + result = ( + sm.filter(lambda t: get_ibis_module(t).literal(True)) + .group_by("status") + .aggregate("cnt") + .execute() + ) + assert result["cnt"].sum() == 6 + + def test_bare_python_bool_still_raises_with_usable_advice(self, orders): + sm = ( + to_semantic_table(orders, name="orders") + .with_dimensions(status=lambda t: t.status) + .with_measures(cnt=lambda t: t.count()) + ) + with pytest.raises(TypeError, match="literal"): + sm.filter(lambda t: True).group_by("status").aggregate("cnt").execute() diff --git a/src/boring_semantic_layer/tests/test_soundness_round4_calc.py b/src/boring_semantic_layer/tests/test_soundness_round4_calc.py new file mode 100644 index 00000000..acbe8092 --- /dev/null +++ b/src/boring_semantic_layer/tests/test_soundness_round4_calc.py @@ -0,0 +1,154 @@ +"""Soundness round 4: windows over inline base-column reductions. + +``lift_inline_reductions`` used to rewrite *every* ``WindowFunction`` +whose ``func`` was a lifted base reduction to the grand-totals shape, +silently discarding the user's ``group_by=``/``order_by=`` window spec. +A partitioned share came back as the share of the GRAND total. + +The fix routes each windowed base reduction by shape: + +* empty window → grand totals (the ``t.all(...)`` shape; unchanged), +* non-empty window, decomposable reduction, window keys all group keys + → re-aggregate the lifted per-group value over the output rows with + the window keys remapped to output columns, +* anything else → :class:`WindowedBaseReductionError` (never a silent + wrong answer). +""" + +from __future__ import annotations + +import pandas as pd +import pytest + +pytest.importorskip("xorq", reason="xorq not installed") + +import xorq.api as xo # noqa: E402 + +from boring_semantic_layer import to_semantic_table # noqa: E402 +from boring_semantic_layer._xorq import ibis as xibis # noqa: E402 +from boring_semantic_layer.calc_compiler import ( # noqa: E402 + WindowedBaseReductionError, +) + + +@pytest.fixture(scope="module") +def orders_st(): + con = xo.duckdb.connect() + df = pd.DataFrame( + { + "status": ["a", "a", "b", "b", "c", "c"], + "region": ["e", "w", "e", "w", "e", "w"], + "day": [1, 1, 2, 2, 3, 3], + "amount": [10.0, 20.0, 30.0, 40.0, 100.0, 200.0], + } + ) + tbl = con.create_table("orders_r4", df) + return ( + to_semantic_table(tbl, name="orders") + .with_dimensions( + status=lambda t: t.status, + region=lambda t: t.region, + day=lambda t: t.day, + ) + .with_measures(total=lambda t: t.amount.sum()) + ) + + +def test_partitioned_window_uses_partition_totals(orders_st): + """``sum().over(window(group_by=region))`` divides by the REGION + total (e=140, w=260), not the grand total (400).""" + st = orders_st.with_measures( + region_share=lambda t: t.amount.sum() / t.amount.sum().over(xibis.window(group_by=t.region)) + ) + df = ( + st.group_by("status", "region") + .aggregate("region_share") + .order_by("status", "region") + .execute() + ) + expected = [10 / 140, 20 / 260, 30 / 140, 40 / 260, 100 / 140, 200 / 260] + assert df["region_share"].tolist() == pytest.approx(expected) + + +def test_ordered_window_running_total(orders_st): + """A cumulative window ordered by a group key accumulates per-group + sums in output order instead of collapsing to the grand total.""" + st = orders_st.with_measures( + running=lambda t: t.amount.sum().over( + xibis.window(order_by=t.day, preceding=None, following=0) + ) + ) + df = st.group_by("day").aggregate("running").order_by("day").execute() + assert df["running"].tolist() == pytest.approx([30.0, 100.0, 400.0]) + + +def test_partitioned_min_max_count_reaggregate(orders_st): + """min/max re-aggregate with min/max; count re-aggregates with sum.""" + st = orders_st.with_measures( + region_max=lambda t: t.amount.max().over(xibis.window(group_by=t.region)), + region_min=lambda t: t.amount.min().over(xibis.window(group_by=t.region)), + region_rows=lambda t: t.amount.count().over(xibis.window(group_by=t.region)), + ) + df = ( + st.group_by("status", "region") + .aggregate("region_max", "region_min", "region_rows") + .order_by("status", "region") + .execute() + ) + assert df["region_max"].tolist() == pytest.approx([100.0, 200.0] * 3) + assert df["region_min"].tolist() == pytest.approx([10.0, 20.0] * 3) + assert df["region_rows"].tolist() == [3] * 6 + + +def test_nondecomposable_windowed_reduction_raises(orders_st): + """mean() cannot be recomputed from per-group values — loud error, + pointing at the measure-reference form.""" + st = orders_st.with_measures( + region_avg=lambda t: t.amount.mean().over(xibis.window(group_by=t.region)) + ) + with pytest.raises(WindowedBaseReductionError, match="not decomposable"): + st.group_by("status", "region").aggregate("region_avg").execute() + + +def test_non_group_key_partition_raises(orders_st): + """Partitioning by a column that is not a group key of the query + cannot be computed at the query's grain — loud error.""" + st = orders_st.with_measures( + region_share=lambda t: t.amount.sum() / t.amount.sum().over(xibis.window(group_by=t.region)) + ) + with pytest.raises(WindowedBaseReductionError, match="not a group key"): + st.group_by("status").aggregate("region_share").execute() + + +def test_empty_window_totals_unchanged(orders_st): + """The ``t.all(...)`` grand-totals shape (empty window) keeps its + base-totals semantics.""" + st = orders_st.with_measures(pct=lambda t: t.amount.sum() / t.all(t.amount.sum())) + df = st.group_by("status").aggregate("pct").order_by("status").execute() + assert df["pct"].tolist() == pytest.approx([30 / 400, 70 / 400, 300 / 400]) + + +def test_mutate_empty_window_over_group_key_column_raises(orders_st): + """``t.status.count().over(window())`` where ``status`` is a group + key is ambiguous (base-row total vs. output-row count) — loud error + instead of silently returning the base-row count.""" + agg = orders_st.group_by("status").aggregate("total") + with pytest.raises(WindowedBaseReductionError, match="ambiguous"): + agg.mutate(n=lambda t: t.status.count().over(xibis.window())).execute() + + +def test_mutate_measure_ref_windows_unaffected(orders_st): + """Measure-reference windows (the documented alternative) keep + working: partitioned share-of-parent and output-row count.""" + agg = orders_st.group_by("status", "region").aggregate("total") + df = ( + agg.mutate(share=lambda t: t.total / t.total.sum().over(xibis.window(group_by=t.status))) + .order_by("status", "region") + .execute() + ) + expected = [10 / 30, 20 / 30, 30 / 70, 40 / 70, 100 / 300, 200 / 300] + assert df["share"].tolist() == pytest.approx(expected) + + agg2 = orders_st.group_by("status").aggregate("total") + df2 = agg2.mutate(n=lambda t: t.total.count().over(xibis.window())).execute() + assert df2["n"].tolist() == [3, 3, 3] diff --git a/src/boring_semantic_layer/tests/test_soundness_round4_join_many.py b/src/boring_semantic_layer/tests/test_soundness_round4_join_many.py new file mode 100644 index 00000000..0c7ae10b --- /dev/null +++ b/src/boring_semantic_layer/tests/test_soundness_round4_join_many.py @@ -0,0 +1,260 @@ +"""Regression tests for the July 2026 round-4 soundness evaluation. + +Pins the join_many participation defect: measures of a ``join_many`` +(many-side) table were pre-aggregated on the RAW unjoined table unless +cross-table filter routing happened to force a join-key bridge. Orphan +rows — a NULL foreign key, or a key matching no left-side row — were +silently counted in grand totals and in many-side-only group-bys, while +mixed group-bys excluded them, so the sum over groups stopped matching +the ungrouped grand total on the same model. + +Required semantics: a many-side row that the LEFT JOIN can never +produce is never counted, regardless of query shape, and the invariant +``sum over groups == ungrouped grand total`` holds for additive +measures. Every expectation below is checked against pandas ground +truth computed from an explicit LEFT JOIN. + +Round-2 C1 (NULL group KEYS preserved through the re-join) must keep +holding: rows that DO join but carry a NULL dimension value still form +a NULL group. NULL join keys and NULL dimension values are different +things. +""" + +import ibis +import pandas as pd +import pytest + +from boring_semantic_layer import to_semantic_table + + +@pytest.fixture +def con(): + return ibis.duckdb.connect(":memory:") + + +CUSTOMERS = pd.DataFrame( + { + "cust_id": [10, 20, 30, 40], + # cust 30 has a NULL tier (NULL dimension VALUE on the one side); + # cust 40 has no orders at all. + "tier": ["gold", "silver", None, "bronze"], + } +) + +ORDERS = pd.DataFrame( + { + "id": [1, 2, 3, 4, 5, 6], + # id 3: NULL FK orphan; id 4: FK matching no customer. + "cust_id": [10, 20, None, 99, 10, 30], + # 'weird' exists ONLY on the unmatched-FK orphan row; + # id 5 is a JOINED row with a NULL dimension value (C1). + "status": ["open", "closed", "open", "weird", None, "open"], + "amount": [10.0, 20.0, 40.0, 80.0, 5.0, 7.0], + } +) + + +def _left_join_truth(): + """Pandas ground truth: explicit customers LEFT JOIN orders.""" + return CUSTOMERS.merge(ORDERS, on="cust_id", how="left") + + +@pytest.fixture +def joined(con): + customers = con.create_table("customers", CUSTOMERS) + orders = con.create_table("orders", ORDERS) + c_st = ( + to_semantic_table(customers, name="customers") + .with_dimensions(tier=lambda t: t.tier) + .with_measures(n_cust=lambda t: t.count()) + ) + o_st = ( + to_semantic_table(orders, name="orders") + .with_dimensions(status=lambda t: t.status) + .with_measures(n=lambda t: t.count(), total=lambda t: t.amount.sum()) + ) + return c_st.join_many(o_st, lambda c, o: c.cust_id == o.cust_id) + + +class TestGrandTotalJoinParticipation: + """Ungrouped aggregates must count only rows the LEFT JOIN produces.""" + + def test_grand_total_matches_left_join(self, joined): + truth = _left_join_truth() + df = joined.aggregate("orders.n", "orders.total").execute() + assert df["orders.n"].iloc[0] == truth["id"].count() # 4, not 6 + assert df["orders.total"].iloc[0] == pytest.approx( + truth["amount"].sum() # 42.0, not 162.0 + ) + + def test_null_fk_orphan_excluded(self, con): + """A NULL foreign key alone (no unmatched-value orphan) is excluded.""" + orders = ORDERS[ORDERS.cust_id != 99] + c = con.create_table("customers_nf", CUSTOMERS) + o = con.create_table("orders_nf", orders) + jm = ( + to_semantic_table(c, name="customers") + .with_dimensions(tier=lambda t: t.tier) + .join_many( + to_semantic_table(o, name="orders").with_measures( + n=lambda t: t.count() + ), + lambda c, o: c.cust_id == o.cust_id, + ) + ) + truth = CUSTOMERS.merge(orders, on="cust_id", how="left") + df = jm.aggregate("orders.n").execute() + assert df["orders.n"].iloc[0] == truth["id"].count() # 4, not 5 + + def test_unmatched_fk_orphan_excluded(self, con): + """A non-NULL key matching no left-side row is excluded.""" + orders = ORDERS[ORDERS.cust_id.notna()] + c = con.create_table("customers_uf", CUSTOMERS) + o = con.create_table("orders_uf", orders) + jm = ( + to_semantic_table(c, name="customers") + .with_dimensions(tier=lambda t: t.tier) + .join_many( + to_semantic_table(o, name="orders").with_measures( + n=lambda t: t.count() + ), + lambda c, o: c.cust_id == o.cust_id, + ) + ) + truth = CUSTOMERS.merge(orders, on="cust_id", how="left") + df = jm.aggregate("orders.n").execute() + assert df["orders.n"].iloc[0] == truth["id"].count() # 4, not 5 + + +class TestGroupedJoinParticipation: + """Grouped aggregates use the same participating rows as grand totals.""" + + def test_group_by_many_side_dim_only(self, joined): + truth = ( + _left_join_truth() + .groupby("status", dropna=False)["id"] + .count() + .loc[lambda s: s > 0] + ) + df = ( + joined.group_by("orders.status") + .aggregate("orders.n") + .execute() + .set_index("orders.status") + ) + assert df.loc["open", "orders.n"] == truth["open"] # 2, not 3 + assert df.loc["closed", "orders.n"] == truth["closed"] + # C1: the joined row with a NULL dimension value forms a NULL group + assert df.loc[[pd.isna(i) for i in df.index], "orders.n"].iloc[0] == 1 + + def test_group_by_left_dim_only(self, joined): + truth = ( + _left_join_truth().groupby("tier", dropna=False)["id"].count() + ) + df = ( + joined.group_by("customers.tier") + .aggregate("orders.n") + .execute() + .set_index("customers.tier") + ) + # counts re-aggregated through the pre-agg path come back as + # Decimal — coerce before comparing against numpy integers + n = pd.to_numeric(df["orders.n"]) + assert n.loc["gold"] == truth["gold"] # 2, not 3 + assert n.loc["silver"] == truth["silver"] # 1 + # NULL tier (cust 30) is a real group with one participating order + assert n.loc[[pd.isna(i) for i in n.index]].iloc[0] == 1 + # cust 40 has no orders: NULL/0 either way, never a positive count + assert pd.isna(n.loc["bronze"]) or n.loc["bronze"] == 0 + + def test_orphan_only_dim_value_absent_everywhere(self, joined): + """'weird' lives only on an orphan row: absent from every result.""" + by_status = joined.group_by("orders.status").aggregate("orders.n").execute() + assert "weird" not in set(by_status["orders.status"].dropna()) + mixed = ( + joined.group_by("customers.tier", "orders.status") + .aggregate("orders.n") + .execute() + ) + assert "weird" not in set(mixed["orders.status"].dropna()) + # ...and its amount is absent from the grand total too + total = joined.aggregate("orders.total").execute()["orders.total"].iloc[0] + assert total == pytest.approx(_left_join_truth()["amount"].sum()) + + +class TestSumOverGroupsInvariant: + """sum over groups of an additive measure == ungrouped grand total.""" + + def test_many_side_dim_groups_sum_to_grand_total(self, joined): + grand = joined.aggregate("orders.n", "orders.total").execute() + by_status = ( + joined.group_by("orders.status") + .aggregate("orders.n", "orders.total") + .execute() + ) + assert by_status["orders.n"].sum() == grand["orders.n"].iloc[0] + assert by_status["orders.total"].sum() == pytest.approx( + grand["orders.total"].iloc[0] + ) + + def test_left_dim_groups_sum_to_grand_total(self, joined): + grand = joined.aggregate("orders.n", "orders.total").execute() + by_tier = ( + joined.group_by("customers.tier") + .aggregate("orders.n", "orders.total") + .execute() + ) + assert pd.to_numeric(by_tier["orders.n"]).fillna(0).sum() == grand[ + "orders.n" + ].iloc[0] + assert pd.to_numeric(by_tier["orders.total"]).fillna(0).sum() == pytest.approx( + grand["orders.total"].iloc[0] + ) + + def test_mixed_dim_groups_sum_to_grand_total(self, joined): + grand = joined.aggregate("orders.n").execute() + mixed = ( + joined.group_by("customers.tier", "orders.status") + .aggregate("orders.n") + .execute() + ) + assert pd.to_numeric(mixed["orders.n"]).fillna(0).sum() == grand[ + "orders.n" + ].iloc[0] + + +class TestNullDimValueStillGroups: + """C1 regression guard: NULL dimension VALUES on joined rows survive.""" + + def test_null_status_group_measures(self, joined): + truth = _left_join_truth() + null_truth = truth[truth["id"].notna() & truth["status"].isna()] + df = ( + joined.group_by("orders.status") + .aggregate("orders.n", "orders.total") + .execute() + ) + null_rows = df[df["orders.status"].isna()] + assert len(null_rows) == 1 + assert null_rows["orders.n"].iloc[0] == len(null_truth) # 1 + assert null_rows["orders.total"].iloc[0] == pytest.approx( + null_truth["amount"].sum() # 5.0 + ) + + def test_null_tier_by_null_status(self, joined): + """NULL keys on BOTH sides of a mixed group-by stay distinct groups.""" + df = ( + joined.group_by("customers.tier", "orders.status") + .aggregate("orders.n") + .execute() + ) + gold_null = df[ + (df["customers.tier"] == "gold") & (df["orders.status"].isna()) + ] + assert len(gold_null) == 1 + assert gold_null["orders.n"].iloc[0] == 1 # order id 5 + null_open = df[ + (df["customers.tier"].isna()) & (df["orders.status"] == "open") + ] + assert len(null_open) == 1 + assert null_open["orders.n"].iloc[0] == 1 # order id 6, cust 30 diff --git a/src/boring_semantic_layer/tests/test_soundness_round4_nest.py b/src/boring_semantic_layer/tests/test_soundness_round4_nest.py new file mode 100644 index 00000000..4e328548 --- /dev/null +++ b/src/boring_semantic_layer/tests/test_soundness_round4_nest.py @@ -0,0 +1,349 @@ +"""Regression tests for the round-4 nest= execution defect. + +``nest=`` lambdas used to receive a scope over raw columns only: the +canonical measure-name form (``t.group_by("sku").aggregate("total_qty")``) +raised a column-not-found error, and inner aggregations whose result +names collided with raw columns executed but were silently discarded in +favour of one raw struct per source row. + +Nest lambdas now receive the aggregation's semantic source table, so +measure names, dimension names, and inline lambdas resolve exactly like +a top-level aggregate. The inner query compiles at (outer keys + inner +keys) grain and is attached as an array-of-structs column via a +null-safe left join on the outer keys. Unsupported nested shapes raise +``NotImplementedError`` — never silent raw-row structs. +""" + +import ibis +import pandas as pd +import pytest + +from boring_semantic_layer import to_semantic_table + +ORDERS_DF = pd.DataFrame( + { + "id": [1, 2, 3, 4, 5, 6], + "sku": ["a", "a", "b", "b", "c", "c"], + "amount": [10.0, 20.0, 30.0, 40.0, 50.0, 60.0], + "qty": [1, 2, 3, 4, 5, 6], + "status": ["open", "closed", "open", "closed", "open", "open"], + "customer_id": [100, 100, 200, 200, 300, 999], + } +) + + +@pytest.fixture +def orders(): + con = ibis.duckdb.connect(":memory:") + tbl = con.create_table("orders", ORDERS_DF) + return ( + to_semantic_table(tbl, name="orders") + .with_dimensions(status=lambda t: t.status, sku=lambda t: t.sku) + .with_measures( + total_qty=lambda t: t.qty.sum(), + total=lambda t: t.amount.sum(), + ) + ) + + +def _nested_frames(result, nest_col): + """{outer key value: DataFrame of the nested structs} for easy comparison.""" + out = {} + for _, row in result.iterrows(): + items = row[nest_col] + out[row[result.columns[0]]] = ( + None if items is None else pd.DataFrame(list(items)) + ) + return out + + +def _ground_truth(df, outer_keys, inner_keys, value_col="qty"): + """Pandas ground truth: inner sums computed within each outer group.""" + grouped = df.groupby([*outer_keys, *inner_keys], dropna=False)[value_col].sum() + return grouped.reset_index() + + +def _assert_nested_matches(nested, truth, outer_key, inner_keys, measure_name): + for outer_val, frame in nested.items(): + expected = truth[truth[outer_key] == outer_val] + assert frame is not None, f"missing nested rows for {outer_val!r}" + got = frame.sort_values(inner_keys).reset_index(drop=True) + exp = expected.drop(columns=[outer_key]).sort_values(inner_keys).reset_index(drop=True) + assert list(got.columns) == [*inner_keys, measure_name] + for ik in inner_keys: + assert list(got[ik]) == list(exp[ik]), (outer_val, ik) + assert [float(v) for v in got[measure_name]] == [ + float(v) for v in exp["qty"] + ], outer_val + + +def test_nest_canonical_measure_name_form(orders): + """The Malloy-canonical form computes the inner measure per outer group.""" + result = ( + orders.group_by("status") + .aggregate( + "total", + nest={"by_sku": lambda t: t.group_by("sku").aggregate("total_qty")}, + ) + .execute() + ) + + assert list(result.columns) == ["status", "total", "by_sku"] + totals = dict(zip(result["status"], result["total"], strict=True)) + assert totals == {"open": 150.0, "closed": 60.0} + + truth = _ground_truth(ORDERS_DF, ["status"], ["sku"]) + _assert_nested_matches( + _nested_frames(result, "by_sku"), truth, "status", ["sku"], "total_qty" + ) + + +def test_nest_aliased_inline_lambda_form(orders): + """Inline lambdas inside the nested aggregate resolve like top-level ones.""" + result = ( + orders.group_by("status") + .aggregate( + "total", + nest={ + "by_sku": lambda t: t.group_by("sku").aggregate( + sumq=lambda x: x.qty.sum() + ) + }, + ) + .execute() + ) + + truth = _ground_truth(ORDERS_DF, ["status"], ["sku"]) + _assert_nested_matches( + _nested_frames(result, "by_sku"), truth, "status", ["sku"], "sumq" + ) + + +def test_nest_inline_name_colliding_with_raw_column(orders): + """The silent-discard shape: result names matching raw columns must now + return aggregated values, not one raw struct per source row.""" + result = ( + orders.group_by("status") + .aggregate( + "total", + nest={ + "by_sku": lambda t: t.group_by("sku").aggregate( + qty=lambda x: x.qty.sum() + ) + }, + ) + .execute() + ) + + open_rows = _nested_frames(result, "by_sku")["open"] + # Raw per-row structs would have 4 entries for "open" (two c rows). + assert len(open_rows) == 3 + truth = _ground_truth(ORDERS_DF, ["status"], ["sku"]) + _assert_nested_matches( + _nested_frames(result, "by_sku"), truth, "status", ["sku"], "qty" + ) + + +def test_nest_multi_key_inner_group_by(orders): + result = ( + orders.group_by("status") + .aggregate( + "total", + nest={ + "by_both": lambda t: t.group_by("sku", "customer_id").aggregate( + "total_qty" + ) + }, + ) + .execute() + ) + + truth = _ground_truth(ORDERS_DF, ["status"], ["sku", "customer_id"]) + _assert_nested_matches( + _nested_frames(result, "by_both"), + truth, + "status", + ["sku", "customer_id"], + "total_qty", + ) + + +def test_nest_within_filtered_outer_query(orders): + """The outer filter restricts the nested aggregation's rows too.""" + result = ( + orders.filter(lambda t: t.qty >= 2) + .group_by("status") + .aggregate( + "total", + nest={"by_sku": lambda t: t.group_by("sku").aggregate("total_qty")}, + ) + .execute() + ) + + totals = dict(zip(result["status"], result["total"], strict=True)) + assert totals == {"open": 140.0, "closed": 60.0} + + filtered = ORDERS_DF[ORDERS_DF["qty"] >= 2] + truth = _ground_truth(filtered, ["status"], ["sku"]) + _assert_nested_matches( + _nested_frames(result, "by_sku"), truth, "status", ["sku"], "total_qty" + ) + + +def test_nest_inner_filter_keeps_outer_groups(orders): + """A filter inside the nest lambda restricts only the nested rows; outer + groups with no surviving inner rows keep a NULL array.""" + result = ( + orders.group_by("status") + .aggregate( + "total", + nest={ + "big_skus": lambda t: t.filter(lambda x: x.qty >= 5) + .group_by("sku") + .aggregate("total_qty") + }, + ) + .execute() + ) + + totals = dict(zip(result["status"], result["total"], strict=True)) + assert totals == {"open": 150.0, "closed": 60.0} + + nested = _nested_frames(result, "big_skus") + assert nested["closed"] is None + open_rows = nested["open"] + assert len(open_rows) == 1 + assert open_rows.iloc[0]["sku"] == "c" + assert float(open_rows.iloc[0]["total_qty"]) == 11.0 + + +def test_nest_without_outer_keys(orders): + result = orders.aggregate( + "total", + nest={"by_sku": lambda t: t.group_by("sku").aggregate("total_qty")}, + ).execute() + + assert len(result) == 1 + assert float(result["total"].iloc[0]) == 210.0 + by_sku = pd.DataFrame(list(result["by_sku"].iloc[0])).sort_values("sku") + assert list(by_sku["sku"]) == ["a", "b", "c"] + assert [float(v) for v in by_sku["total_qty"]] == [3.0, 7.0, 11.0] + + +def test_nest_bare_group_by_keeps_per_row_structs(orders): + """Pinned historical semantics: bare group_by collects one struct per + source row, duplicates included.""" + result = ( + orders.group_by("status") + .aggregate("total", nest={"rows": lambda t: t.group_by(["sku", "qty"])}) + .execute() + ) + + nested = _nested_frames(result, "rows") + open_rows = nested["open"].sort_values(["sku", "qty"]).reset_index(drop=True) + assert len(open_rows) == 4 # both "c" rows survive + assert list(open_rows["sku"]) == ["a", "b", "c", "c"] + assert list(open_rows["qty"]) == [1, 3, 5, 6] + + +def test_nest_inner_order_by_orders_each_group_array(orders): + """order_by after the inner aggregate orders each group's struct array.""" + result = ( + orders.group_by("status") + .aggregate( + "total", + nest={ + "x": lambda t: t.group_by("sku") + .aggregate("total_qty") + .order_by(lambda t: t.total_qty.desc()) + }, + ) + .execute() + ) + nested = _nested_frames(result, "x") + open_qty = [float(v) for v in nested["open"]["total_qty"]] + assert open_qty == sorted(open_qty, reverse=True) + + +def test_nest_two_outer_keys_executes_on_xorq_datafusion(orders): + """Multiple null-safe join predicates must remain boolean expressions.""" + orders = orders.with_dimensions(customer=lambda t: t.customer_id) + result = ( + orders.group_by("status", "customer") + .aggregate( + "total", + nest={"by_sku": lambda t: t.group_by("sku").aggregate("total_qty")}, + ) + .execute() + ) + + assert len(result) == 6 + row = result[(result["status"] == "open") & (result["customer"] == 300)].iloc[0] + assert float(row["total"]) == 50.0 + assert list(row["by_sku"]) == [{"sku": "c", "total_qty": 5}] + + +def test_nest_within_nest_regrains_to_outer_keys(orders): + """A nest entry inside a nest entry widens to the full outer grain. + + The inner-most plan used to keep the middle lambda's grain (sku, + customer): compiling the middle level then raised column-not-found + on the outer key, and had it compiled it would have aggregated + customers across outer groups. + """ + orders = orders.with_dimensions(customer=lambda t: t.customer_id) + result = ( + orders.group_by("status") + .aggregate( + "total", + nest={ + "by_sku": lambda t: t.group_by("sku") + .aggregate( + "total_qty", + nest={ + "by_customer": lambda t2: t2.group_by("customer").aggregate( + "total_qty" + ) + }, + ) + .order_by(lambda t: t.total_qty.desc()) + .limit(2) + }, + ) + .execute() + ) + + assert list(result.columns) == ["status", "total", "by_sku"] + by_status = {row["status"]: row for _, row in result.iterrows()} + + # Middle level: ordered desc by total_qty, truncated to 2 per group. + open_skus = list(by_status["open"]["by_sku"]) + assert [s["sku"] for s in open_skus] == ["c", "b"] + assert [float(s["total_qty"]) for s in open_skus] == [11.0, 3.0] + + # Inner-most level: scoped to (status, sku). Customer 100 under + # ("closed", "a") must see qty 2 only, not the cross-status total 3. + closed_skus = {s["sku"]: s for s in by_status["closed"]["by_sku"]} + assert { + c["customer"]: float(c["total_qty"]) + for c in closed_skus["a"]["by_customer"] + } == {100: 2.0} + inner_c = { + c["customer"]: float(c["total_qty"]) + for c in dict((s["sku"], s) for s in open_skus)["c"]["by_customer"] + } + assert inner_c == {300: 5.0, 999: 6.0} + + +def test_nest_unsupported_shapes_raise(orders): + """Transformed bare group_by raises loudly instead of silently + collecting raw rows.""" + with pytest.raises(NotImplementedError, match="bare group_by"): + orders.group_by("status").aggregate( + "total", + nest={"x": lambda t: t.filter(lambda x: x.qty > 1).group_by("sku")}, + ) + + +if __name__ == "__main__": + pytest.main([__file__, "-v"]) diff --git a/src/boring_semantic_layer/tests/test_xorq_string_serialization.py b/src/boring_semantic_layer/tests/test_xorq_string_serialization.py index b5bc6a5b..1b277a8c 100644 --- a/src/boring_semantic_layer/tests/test_xorq_string_serialization.py +++ b/src/boring_semantic_layer/tests/test_xorq_string_serialization.py @@ -1389,8 +1389,8 @@ def test_tagged_roundtrip_join_filter_aggregate(): assert got["Bob"] == 70 -def test_tagged_roundtrip_join_inner(): - """Inner join survives round-trip and excludes non-matching rows.""" +def test_tagged_roundtrip_left_join_with_explicit_match_filter(): + """Explicit inner semantics survive round-trip and exclude unmatched rows.""" import pandas as pd from boring_semantic_layer.serialization import from_tagged, to_tagged @@ -1413,8 +1413,10 @@ def test_tagged_roundtrip_join_inner(): label=lambda t: t.label, ) - joined = left_st.join_many(right_st, lambda l, r: l.key == r.key, how="inner").with_dimensions( - label=lambda t: t.label + joined = ( + left_st.join_many(right_st, lambda left, right: left.key == right.key) + .with_dimensions(label=lambda t: t.label) + .filter(lambda t: t.label.notnull()) ) tagged = to_tagged(joined) diff --git a/src/boring_semantic_layer/tests/test_yaml.py b/src/boring_semantic_layer/tests/test_yaml.py index b3c08835..f3363042 100644 --- a/src/boring_semantic_layer/tests/test_yaml.py +++ b/src/boring_semantic_layer/tests/test_yaml.py @@ -221,6 +221,7 @@ def test_load_model_with_join_one(sample_tables): try: models = from_yaml(yaml_path, tables=sample_tables) flights = models["flights"] + assert flights.how == "left" # Test query with joined dimension (use dot notation) result = ( diff --git a/src/boring_semantic_layer/utils.py b/src/boring_semantic_layer/utils.py index 5c2672c3..08e2a038 100644 --- a/src/boring_semantic_layer/utils.py +++ b/src/boring_semantic_layer/utils.py @@ -515,32 +515,43 @@ def do_convert(): lambda_str = f"lambda t: {t_expr}" import ibis - from ibis import _ - try: - from ._xorq import api as xo, ibis as xorq_ibis - - eval_context = { - "ibis": ibis, - "_": _, - "xorq_ibis": xorq_ibis, - "xo": xo, - } - allowed_names = {"ibis", "_", "xorq_ibis", "xo", "t"} - except ImportError: - eval_context = { - "ibis": ibis, - "_": _, - } + def _build(flavor_ibis): + """Evaluate the lambda with ``ibis``/``_`` bound to one flavor.""" + eval_context = {"ibis": flavor_ibis, "_": flavor_ibis._} allowed_names = {"ibis", "_", "t"} + try: + from ._xorq import api as xo, ibis as xorq_ibis - result = safe_eval(lambda_str, context=eval_context, allowed_names=allowed_names) - if isinstance(result, Success): - return result.unwrap() - elif isinstance(result, Failure): - raise result.failure() - else: - raise ValueError(f"Unexpected result type: {type(result)}") + eval_context.update({"xorq_ibis": xorq_ibis, "xo": xo}) + allowed_names |= {"xorq_ibis", "xo"} + except ImportError: + pass + + result = safe_eval(lambda_str, context=eval_context, allowed_names=allowed_names) + if isinstance(result, Success): + return result.unwrap() + elif isinstance(result, Failure): + raise result.failure() + else: + raise ValueError(f"Unexpected result type: {type(result)}") + + # Eager evaluation validates the string up front; the returned wrapper + # re-binds ``ibis``/``_`` to the flavor (plain vs xorq-vendored) of the + # table it is called with, so eager constructors like ``ibis.literal`` + # compose with either flavor instead of silently mis-comparing. + fns = {id(ibis): _build(ibis)} + + def _flavored(t): + from .nested_compile import get_ibis_module + + flavor = get_ibis_module(t) + key = id(flavor) + if key not in fns: + fns[key] = _build(flavor) + return fns[key](t) + + return _flavored return do_convert() diff --git a/src/boring_semantic_layer/yaml.py b/src/boring_semantic_layer/yaml.py index 2d416302..935f8747 100644 --- a/src/boring_semantic_layer/yaml.py +++ b/src/boring_semantic_layer/yaml.py @@ -210,7 +210,7 @@ def _parse_joins( # Apply the join based on type join_type = join_config.get("type", "one") # Default to one-to-one - how = join_config.get("how") # Optional join method override + how = join_config.get("how") or "left" if join_type == "cross": # Cross join - no keys needed @@ -231,7 +231,7 @@ def make_join_condition(left_col, right_col): result_model = result_model.join_one( join_model, on=on_condition, - how=how if how else "inner", + how=how, ) elif join_type == "many": left_on = join_config.get("left_on") @@ -249,7 +249,7 @@ def make_join_condition(left_col, right_col): result_model = result_model.join_many( join_model, on=on_condition, - how=how if how else "left", + how=how, ) else: raise ValueError(f"Invalid join type '{join_type}'. Must be 'one', 'many', or 'cross'")