Unify calc measures on ibis-native classifier (ADR 0001 phases 1+2)#262
Draft
hussainsultan wants to merge 18 commits into
Draft
Unify calc measures on ibis-native classifier (ADR 0001 phases 1+2)#262hussainsultan wants to merge 18 commits into
hussainsultan wants to merge 18 commits into
Conversation
First piece of ADR 0001 (phase 1): introduce calc_analyzer.analyze_calc_expr, which classifies a calc-measure ibis expression by walking the tree rather than pattern-matching curated-AST node types. Returns CalcExprAnalysis with pushable / references_AllOf / has_window / post_agg_only / depends_on. Distinguishes empty windows over reductions (the t.all totals pattern) from partitioned/ordered windows (real window functions). Pure additive — not yet wired into ops.py; the existing curated-AST classifier still runs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Second piece of ADR 0001 (phase 1): introduce calc_compiler with IbisCalcScope, evaluate_calc_lambda, classify_calc_lambda, and compile_calc_measure. Calc-measure lambdas evaluate against a dual-table scope (base table for inline aggs, virtual aggregated table for measure refs); the analyzer reads structural shape off the resulting ibis tree; compilation substitutes the virtual table for the real aggregated table via op.replace. Still additive: the curated-AST compile_grouped_with_all path remains intact. Next step is wiring the analyzer/compiler into _classify_measure and _build_aggregation_plan in ops.py. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Captures branch state, what's landed (analyzer + compiler modules), and the concrete 8-step plan to finish the hard cutover. Flags two design decisions worth re-checking before continuing: t.all semantics for non-sum measures (windowed sum vs. true totals re-aggregation) and calc-of-calc dependency ordering. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Rewrite the ADR from "drop SemanticMutateOp" to "unify calc measures and post-aggregation mutate on a single ibis-expression primitive," update the implementation handoff to reflect the landed phases 1+2 hard cutover, and add a refactor-review companion plus an ibis-vs-xorq guide for the plain-vs-vendored boundary. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add the ``Item`` (subscript) resolver to serialize_resolver and deserialize_resolver alongside the existing Attr/Call/BinaryOperator branches. Calc measures that look columns up via ``t["prefixed.name"]`` need this so the resolver tree round-trips through to_tagged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Extend the analyzer to skip Relation subtrees so base-table window expressions don't pollute calc-measure classification, and grow the compiler with the runtime pieces the cutover needs: - ``IbisCalcScope`` with column-first dispatch, suffix-resolution for prefixed measure names on joined models, and ``UnknownMeasureRefError`` on unknown lookups. - ``apply_calc_measures`` re-runs each calc lambda against the real aggregated table, ordering calcs topologically so calc-of-calc chains see prior results as columns. - ``lift_inline_reductions`` lifts inline base-table reductions into anonymous base measures so patterns like ``t.value.sum() / t.all(t.value.sum())`` compile through ``mutate`` on the post-aggregated table. - ``rename_measure_refs`` rebuilds calc expressions with prefixed field names for joined models (kept available for callers; runtime uses IbisCalcScope's suffix matching instead). The new functions are pure additions to the previously-committed analyzer/compiler scaffolding; they are unused by ops.py at this commit and become live in the cutover. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Implement ADR 0001 phases 1+2 as a hard cutover. Calc measures are now
ibis expressions classified by walking the resulting tree rather than
by AST node tag.
ops.py
- New ``CalcMeasure`` storage shape (frozen dataclass with expr,
description, requires_unnest, depends_on, metadata).
- ``_classify_measure`` runs the lambda against ``IbisCalcScope`` once,
walks the result with ``analyze_calc_expr``, and routes pushable
expressions to ``_make_base_measure`` or post-agg expressions to
``CalcMeasure``.
- ``_build_aggregation_plan`` / ``_compile_aggregation`` replace the
curated ``compile_grouped_with_all`` pipeline. Bare-name aggregate
lookups, transitive base-measure dependencies (with suffix matching
on prefixed names), and the inline-reduction lift all flow through
here.
- The pre-agg path's ``_apply_calc_specs`` and the deferred-join arm
both go through ``apply_calc_measures``; nothing in the planner
branches on AST node types.
- ``_update_measure_refs_in_calc`` is now a no-op stub since
``IbisCalcScope`` resolves prefixed names at query time.
- Curated AST helpers removed: ``_is_calculated_measure``,
``_matches_aggregation_pattern``, ``_find_matching_measure``,
``_resolve_aggregation_exprs``, ``_expand_calc_measure_refs``,
``_collect_measure_refs``, ``_create_measure_spec``.
measure_scope.py
- Drop the curated AST classes (``MeasureRef``, ``AllOf``, ``BinOp``,
``MethodCall``, ``AggregationExpr``, ``MeasureExpr``, ``_Node``,
``_PendingMethodCall``, ``DeferredColumn``) and ``validate_calc_ast``.
- ``MeasureScope`` / ``ColumnScope`` survive as thin pass-through
proxies for ``SemanticMutateOp`` (Phase 3 will drop that op).
compile_all.py
- Deleted. Nested-array helpers extracted to ``nested_compile.py``;
the curated calc-measure compiler (``compile_grouped_with_all``,
``infer_calc_dtype``, ``_compile_formula``, ``_collect_all_refs``)
is replaced by ``calc_compiler``.
nested_compile.py (new)
- ``join_tables``, ``build_session_table``, ``build_nested_level_table``,
``build_level_aggregations``, ``unnest_nested_arrays``, and
``get_ibis_module``.
serialization/extract.py
- ``serialize_calc_measures`` walks each ``CalcMeasure.expr`` via
``expr_to_structured`` and persists the resolver tree plus
description/requires_unnest/depends_on. No more AST-tag dispatch.
- ``deserialize_calc_measures`` rebuilds ``CalcMeasure(expr=Deferred,
depends_on=...)`` and lets the planner re-run the lambda at query
time.
expr.py / graph_utils.py / agents/backends/mcp.py / server/api.py
- Drop ``AggregationExpr`` import + wrap helper from
``SemanticGroupBy.aggregate`` (the analyzer covers it now).
- ``build_dependency_graph`` reads ``CalcMeasure.depends_on``.
- Two import sites switch from ``compile_all._get_ibis_module`` to
``nested_compile.get_ibis_module``.
tests
- ``test_method_call_serialization_roundtrip`` rewritten to assert the
behavioral round-trip via to_tagged/from_tagged.
- ``test_validate_calc_ast_*`` removed (function is gone).
- ``test_substring_measure_name_does_not_trigger_typo`` probes
``IbisCalcScope`` directly.
- ``test_all_of_multilayer_calc_measure`` updated to a sum-style chain
matching v1 ``t.all(...)`` semantics — the avg-style assertion is
documented as a v1 limitation per ADR design decision #1.
- ``test_all_with_aggregation_expr_post_ops`` reframed for the
inline-reduction lift.
Net diff approximately -727 lines across the touched modules. Test
suite: 956 passed, 1 failed (preexisting xorq-vendored-ibis read_parquet
issue, unrelated to this work).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The previous implementation emitted ``column.sum().over(window())`` for
``t.all(measure_ref)`` regardless of the underlying aggregation. That
matches the curated-AST behavior for sum-style measures (sum of per-group
sums = overall sum) but is wrong for ``mean``/``quantile``/etc.: a
windowed sum of per-group means is not the overall mean. Resolves the
v1 limitation flagged in the ADR 0001 handoff, design decision #1.
The fix mirrors the approach the deleted curated-AST compiler used:
build a no-group-by totals aggregation from the base table, apply
non-AllOf calc measures to it, and cross-join it into the per-group
result. ``t.all(measure_ref)`` now resolves to a Field reference on a
parallel totals virtual table that the compiler substitutes at
compile time.
Mechanism:
* ``IbisCalcScope`` carries a parallel ``totals_virtual_agg_tbl``
with the same schema as the existing virtual aggregated table.
``t.all("name")`` and ``t.all(t.measure_ref)`` both return
``Field(totals_vt, name)``. Raw column refs and inline reductions
keep their pre-existing shape until the lift runs.
* ``lift_inline_reductions`` extends both the regular and totals
virtual tables with the lifted anonymous reductions, then rewrites
bare reductions to ``Field(vt, anon)`` (per-group) and
window-wrapped reductions (the ``t.all(...)`` totals shape) to
``Field(totals_vt, anon)``. ``Sum`` no longer wraps the totals
field, so non-sum reductions (mean/quantile) get correct overall
values too.
* ``analyze_calc_expr`` takes a ``totals_vt_op`` parameter; any Field
referencing it sets ``references_AllOf=True`` and is added to
``depends_on``.
* ``compile_calc_measure`` now substitutes both ``virtual_agg_tbl``
and ``totals_virtual_agg_tbl`` Fields. Totals fields rewrite to
``Field(real_with_totals, "__bsl_totals__<name>")`` — the prefixed
column produced by cross-joining the real totals into the result.
* ``apply_calc_measures`` accepts an optional ``agg_specs`` and
builds the totals table lazily on first use; ``_compile_aggregation``
builds it eagerly when any classified calc references AllOf so the
base aggregation cost is paid once.
Tests:
* ``test_all_of_multilayer_calc_measure`` restored to its
pre-cutover ratios assertion (151/251, 351/251) — the windowed-sum
answer (1.0) was the wrong baseline.
* ``test_all_of_non_sum_measure_uses_totals_table`` covers the
``mean`` case directly: AA mean=150, UA mean=350, overall mean=250
(NOT 150+350=500).
* ``test_calc_compiler.py`` rewritten to exercise totals via
``apply_calc_measures``; the simple ``compile_calc_measures``
wrapper survives for non-totals cases.
Test suite: 957 passed (was 956), 1 preexisting failure (xorq
read_parquet, unrelated). The two new tests are net-additions.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…lear errors
Addresses code-review follow-ups on the totals-table fix:
* **Clear error when totals are unavailable.** Add
``TotalsNotAvailableError`` and raise it from ``apply_calc_measures``
and ``_compile_aggregation`` when a calc references ``t.all(...)``
but neither ``real_totals_tbl`` nor ``agg_specs`` lets us build one
(and on the nested-array path, where building one is not yet
supported). Previously these cases reached ibis with unsubstituted
``Field(totals_vt, ...)`` references and surfaced as opaque
``IntegrityError: Cannot add ... to projection``.
* **Memoize analyzer per calc name.** ``classify_calc_lambdas``
produces a ``{name → CalcExprAnalysis}`` dict that
``_topological_calc_order``, ``_build_totals_from_agg_specs``,
and the apply loop all reuse. Each calc lambda is now evaluated and
analyzed once per ``_compile_aggregation`` call instead of three or
four times.
* **Helpers + constant.** Module-level ``TOTALS_PREFIX`` constant
replaces the magic string that was hard-coded in three places.
``_to_op(x)`` collapses the ``x.op() if hasattr(...) and callable
(...) else x`` boilerplate that appeared ~10 times. ``_drop_totals_columns``
replaces two near-identical ``select([c for c in ... if not
c.startswith(...)])`` blocks.
* **Log swallowed exceptions.** ``logger.debug`` calls in the
classification fallback (``lifted_calc_specs[name] = None``), the
totals-aggregation evaluation, and the ``IbisCalcScope.all()``
reduction-detection branch so real bugs aren't invisible.
Tests:
* ``test_compile_substitutes_virtual_for_real`` rewritten to actually
call ``compile_calc_measure`` (the public entry point it claims to
test) and verify the resulting expression executes correctly,
rather than doing manual ``op.replace`` plumbing.
* ``test_apply_calc_measures_raises_when_totals_unavailable`` covers
the clear-error path.
* ``test_multiple_allof_calcs_share_one_totals_aggregation``
inspects the rendered SQL to lock down "one totals aggregation,
one CROSS JOIN" — guards against an O(n) regression where each
AllOf calc builds its own totals.
* ``test_apply_calc_measures_join_with_mean_totals`` covers the
pre-agg-on-joins code path with a non-sum measure: AA mean=150,
UA mean=350, overall mean=250 (NOT 150+350=500).
Test suite: 960 passed (up from 957), 1 preexisting failure (xorq
read_parquet, unrelated). No behavior changes for non-totals calcs.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…, assert totals substitutions
Addresses code-review follow-ups on the ibis-native calc-measure work:
* **Bare-name lambdas marked at the API site.** ``_detect_bare_name_lambda``
previously returned the first string default it found, which silently
misrouted any user lambda with a string default (``lambda t, c=col,
op=op: getattr(...)``) as a bare-name reference. The detector now reads
the ``_bsl_bare_ref`` sentinel attribute set by ``make_bare_ref_lambda``,
and ``SemanticGroupBy.aggregate`` constructs the trivial passthroughs
through that helper.
* **Re-eval branch re-lifts inline reductions.** When preprocessing lift
failed, the apply-time fallback called ``evaluate_calc_lambda`` +
``analyze_calc_expr`` but skipped ``lift_inline_reductions``, so a calc
with inline base reductions reached the compiler with bare
``Field(base, ...)`` references that ibis can't compile through
``mutate``. The fallback now re-runs the lift; if it produces lifted
aggregations the per-group result no longer accommodates, we raise with
the original preprocessing exception chained so the root cause is
visible.
* **Assert totals substitution completeness.** ``compile_calc_measure``
silently dropped ``Field(totals_vt, name)`` substitutions when neither
prefixed nor unprefixed columns existed on ``real_with_totals``;
surviving references later reached ibis as opaque
``IntegrityError: Cannot add ... to projection``. We now walk the
rewritten op and raise ``TotalsNotAvailableError`` listing the
unresolved names and the available columns.
Cleanups:
* Single-pass ``_scan_tree`` replaces three independent ``_walk``
traversals (``_has_window_under``, ``_has_reduction_under``,
``_has_totals_pattern``) — analyzer was doing O(K) full subtree walks
per encountered reduction.
* Collapse the two ``_topological_*`` helpers (calc_compiler.py and
ops.py) onto a shared ``topological_order_from_deps`` that takes a
generic ``name → {dep, …}`` map.
* Inline-reduction naming uses an integer counter instead of
``md5(repr(r))`` — repr is expensive on deep trees and the hash bought
nothing.
* ``IbisCalcScope.all`` now warns on the raw-column fallback so the
silent ``column.sum().over(window())`` shape doesn't hide non-sum
semantics from users.
* Drop the cleverness ``(lambda r=r: lambda t: r)()`` in favor of the
plain ``lambda t, r=r: r``.
Tests: 973 passed, 1 preexisting xorq read_parquet failure (unrelated).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…vers, extract empty-schema constant, drop dead helpers
Addresses code-review follow-ups on the ibis-native calc-measure work:
* **Latent hash bug fixed across all FrozenSlotted resolver branches.**
``deserialize_resolver`` constructs ``Attr``/``Item``/``Call``/
``BinaryOperator``/``UnaryOperator``/``Sequence``/``Mapping`` via
``object.__new__`` + ``__setattr__`` to bypass FrozenSlotted
validation, but never set ``__precomputed_hash__``. Hashing a
rebuilt resolver — e.g. when used as a dict key inside ``op.replace``
substitutions — raised ``AttributeError``. Add ``_finalize_frozen_slotted``
that mirrors xorq's hash formula (``hash((cls, tuple(field_values)))``,
with the inner ``tuple`` wrap that makes ``hash(r) == hash(fresh)``)
and call it from every branch.
* **Multi-source classifier fallthrough is no longer silent.**
``_classify_measure`` routes ``pushable=False AND post_agg_only=False``
(multiple source tables, no window/AllOf/measure-deps) to base
classification so the lambda runs verbatim at agg time. Add a
``logger.debug`` so the fallthrough is visible if a user's lambda
silently fails to reach the calc path.
* **``_make_agg_callable`` semantic explained.** The Phase 1+2 cutover
removed the implicit ``ColumnScope`` wrapping for raw callables —
intentional, since ``Measure.expr`` already self-wraps and lifted-
reduction stubs close over a pre-built ibis op. Expand the docstring
so the next reader doesn't put the wrapping back.
* **Drop dead ``_update_measure_refs_in_calc`` stub.** The function was
a no-op; ``IbisCalcScope`` does suffix-resolution at query time
instead. Also delete the ``is_calc_measures`` branch in
``_merge_fields_with_prefixing`` that called it. ~25 LOC removed.
Cleanups:
* ``_EMPTY_VT_SCHEMA`` module-level constant in ``calc_compiler.py``
replaces 4 inline ``{"__bsl_unused__": "int64"}`` literals across
``IbisCalcScope.__init__``, ``evaluate_calc_lambda``, and
``lift_inline_reductions``.
* Documented ``apply_calc_measures``'s asymmetric totals contract:
``agg_specs`` path re-applies non-AllOf calcs to the totals;
``real_totals_tbl`` path requires the caller to do so.
Tests added:
* ``test_apply_calc_measures_non_sum_totals`` parametrized over
``median``/``min``/``max`` — locks ``t.all(...)`` semantics for
the rest of the common non-sum reductions beyond the existing
``mean`` case.
* ``test_lift_inline_reductions_routes_window_to_totals`` directly
exercises the two-pass substitution: a single shared reduction
appearing both at top level and inside a window must route to
``Field(vt, anon)`` and ``Field(totals_vt, anon)`` respectively
(``op.replace`` would dedupe by equality otherwise).
* ``test_serialize_resolver_item_subscript_roundtrips_and_hashes``
pins the hash-after-deserialization behavior — would have caught
the FrozenSlotted bug above.
Test suite: 978 passed (up from 973), 1 preexisting xorq read_parquet
failure (unrelated). No behavior changes for existing calc semantics.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…3 readiness The ADR now captures both the work this branch landed (Phases 1+2 — unify calc measures on an ibis-native classifier) and the concrete deletion plan for Phase 3 (drop ``SemanticMutateOp``). Replaces the side-by-side handoff and refactor-review notes with one self-contained document. * **Implementation status** section splits realized work from outstanding work. Each landed module gets a one-line summary (``calc_analyzer``, ``calc_compiler``, ``ops``, ``measure_scope``, ``compile_all`` deletion, ``serialization/extract``, ``utils``). Each unrealized step lists concrete file:line targets. * **Phase 3 readiness checklist.** A 17-row table maps every ``SemanticMutateOp`` reference in the codebase to what replaces it — the planner branches in ``ops.py``, the chain operator in ``expr.py``, the to-ibis converter in ``convert.py``, the formatter, the chart introspection, and the serialization registrations. * **Composition gotchas table.** Eight chained-mutate compositions (after-filter, before-aggregate, after-join, …) with their ``with_measures``-equivalent and a pointer to where the equivalence comes from in the existing code. Calls out the one real gap: ``SemanticLimit.with_measures`` does not exist yet and must be added in Phase 3. The ``mutated_gb_keys`` heuristic (``ops.py:2553``) collapses without replacement once mutate- introduced columns become calc measures. * **Behavior changes for users** section commits to three pieces of surface area that the cutover surfaces or stabilizes: calc lambdas execute twice per query (no side effects in calcs); the ``IbisCalcScope`` dispatch order is the public contract (column- first, then known-measure suffix lookup, then ibis Table methods, then ``UnknownMeasureRefError``); nested-array measures combined with ``t.all(...)`` raise ``TotalsNotAvailableError`` rather than silently producing the wrong answer. * **No new method.** Per-query ad-hoc calcs use ``with_measures`` — the existing method on ``SemanticAggregate`` already routes through the analyzer post-cutover. The chained ``.mutate(**post)`` survives as a 3-line desugaring to ``self.with_measures(**post).aggregate( *current, *post.keys())`` so existing user code keeps working. * **No migration tool for old tags.** Tags containing ``SemanticMutateOp`` fail to deserialize with a clear error pointing at the ``with_measures`` equivalent. Users either re-tag from the current model or pin the prior BSL version. Building a tool that introspects arbitrary persisted lambdas would cost more than the user-side re-tag. * **Open questions** marked resolved/open inline. The only genuinely open question is sequencing within Phase 3 (which the recommendation fixes to a 4-step order); analyzer scope, ``t.all`` API, non-sum totals, inline reductions, calc-of-calc ordering, compat-shim duration, and chained-after-filter semantics are all settled. * **Removed:** ``0001-handoff-2026-05-08.md`` and ``refactor-review-2026-05-08.md``. Their durable content (code pointers, branch state, downstream cleanup ideas) is folded into the ADR or no longer relevant. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Out of scope for ADR 0001 — the plain-vs-vendored ibis boundary doc belongs in a separate documentation PR. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…al schema
The pre-process step in ``_compile_aggregation`` populated the virtual
aggregated table's schema with placeholder ``float64`` dtypes for every
known measure. When a user's calc lambda contained
``t.flight_count.cast('float64')``, ibis saw the column was already
``float64`` in the synthetic schema and silently elided the cast as a
no-op during ``evaluate_calc_lambda``. After ``op.replace`` substituted
``Field(virtual_agg) → Field(real_agg)``, the Cast was gone but the real
column was ``int64`` (e.g. from ``CountStar``), so
``int / int * 100`` truncated to 0 for any ratio less than 1.
Found end-to-end against the FAA flights demo where
``share_of_flights = t.flight_count.cast('float64') / t.all(t.flight_count) * 100``
returned 0.0 for every carrier instead of the expected per-carrier
percentages.
The fix probes ``agg_specs[n](base_tbl).type()`` once at preprocess
time to populate the virtual schema with the same dtypes the user's
calc will see at compile time. Falls back to ``float64`` (with a
debug log) when probing fails, so the existing fallback semantics are
preserved for measures the analyzer can't evaluate against the base
table.
The apply-time re-eval branch already used real dtypes derived from
``real_agg_tbl[col].type()`` — only the preprocess path was affected.
``_classify_measure`` still uses placeholder dtypes since classification
discards the resolved expression and only inspects structural shape.
Tests:
* New ``test_cast_to_float_survives_int_measure_substitution`` pins the
bug end-to-end: ``count.cast('float64') / count_total * 100`` returns
the right per-group percentages summing to 100%, not 0%.
Also documented as a known limitation in ``_join_totals``: when
per-group and totals aggregations share a parent relation (typical on
joined models built from xorq RemoteTables), some SQL backends
collapse the shared-ancestor cross-join and return zero rows.
``.view()`` is dissolved by downstream substitutions and ``.cache()``
breaks SQL ``.compile()``. Real fix likely needs to live in the SQL
compiler; tracked as a Phase 3 follow-up.
Test suite: 979 passed (up from 978), 1 preexisting unrelated failure.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…rried through aggregation Replaces the two-aggregates-plus-cross-join totals strategy with a single-pass windowed-totals approach: compute each AllOf-referenced measure's formula as a window function over the entire base, broadcast to every row, then carry through the per-group aggregation via ``arbitrary()``. The totals appear as ``__bsl_totals__<name>`` columns on the per-group result; calc-measure compilation rewrites ``Field(totals_vt, name) → Field(real_agg, "__bsl_totals__<name>")`` directly without a cross-join. Resolves the joined-model totals limitation: when ``real_agg_tbl`` and ``real_totals_tbl`` shared a parent relation (per-group ``Aggregate(X)`` and no-group-by ``Aggregate(X)`` with shared ``X``) — typical on joined models built from xorq RemoteTables — the SQL compiler collapsed the shared-ancestor cross-join and returned zero rows. The new path has no cross-join at all: there's a single aggregation, with totals carried as window-function columns. Compiles to standard SQL on every backend that supports window functions. Mechanism: * ``attach_windowed_totals(base, agg_specs, names)`` mutates the base with ``measure.over(window()) AS __bsl_totals__<name>`` for each base measure that's the (transitive) target of an ``AllOf``. * The per-group aggregation includes ``__bsl_totals__<name>`` via ``arbitrary()`` so the totals propagate as ordinary columns. * ``attach_calc_totals(real_agg, calc_specs, classifications)`` handles calc-of-calc-AllOf: for each calc transitively referenced by an ``AllOf``, evaluate the calc lambda against a ``_TotalsResolvingScope`` that maps ``t.measure`` to ``real_agg["__bsl_totals__measure"]``, producing ``__bsl_totals__<calc_name>`` derived from the base totals. * ``compile_calc_measure``'s substitution path is unchanged — it rewrites ``Field(totals_vt, ...)`` to point at the prefixed columns, which now live on ``real_agg_tbl`` directly instead of on a separate cross-joined table. Validation against the FAA flights demo (xorq RemoteTable + 5-table join chain): ``flights.share_of_flights`` correctly returns 15 per-carrier shares summing to 100%, where the previous cross-join implementation returned 0 rows on this code path. ``_join_totals`` survives as a deprecated shim for the ``apply_calc_measures(real_totals_tbl=...)`` entry point that takes a pre-built totals table; new code goes through the windowed-totals path. Tests: * ``test_multiple_allof_calcs_share_one_totals_per_measure`` updated from "expect one CROSS JOIN" to "expect zero cross joins, one windowed totals column per AllOf-referenced measure" — the property the test was guarding (don't compute the same totals twice) still holds, expressed via window functions instead of cross-joins. * All other existing calc-measure tests pass unchanged: 979 passed, 1 preexisting unrelated failure. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ion tests Adds four minimal reproducible tests that lock in the windowed-totals fix from the previous commit: * ``test_joined_model_totals_via_windowed_aggregation`` — minimal two-table join with ``t.all(measure)``. The bug it guards: the previous cross-join strategy returned zero rows on this shape because per-group and totals aggregations shared a parent relation. Asserts the result has the expected per-group rows and shares summing to exactly 100%. * ``test_joined_model_totals_does_not_emit_cross_join`` — pins the SQL shape: zero ``CROSS JOIN`` operations, at least one ``OVER`` window. A regression to cross-join would re-introduce the shared-ancestor collapse. * ``test_attach_windowed_totals_helper`` — direct unit test for the helper that adds ``measure.over(window())`` columns to the base. Verifies the columns appear, carry constant totals values across rows, and produce the expected ``arbitrary()`` specs. * ``test_attach_calc_totals_handles_calc_of_calc`` — pins the calc-of-calc-AllOf path (the case that surfaced ``test_all_of_multilayer_calc_measure``). Builds a chain ``avg = total_distance / total_flights`` then ``ratio = avg / t.all(avg)`` and verifies non-sum-style totals return the right answer (overall mean 250, not sum-of-per-group-means 500). Test suite: 983 passed (up from 979), 1 preexisting unrelated failure. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…y access works on re-group When re-aggregating a table that contains an array-of-struct column, post-aggregation measure lambdas like ``t.flights.carrier.nunique()`` ran against the raw ibis table and failed with ``'ArrayColumn' object has no attribute 'carrier'`` before the NestedAccessMarker pipeline could route them to _compile_aggregation_with_nested. Wrap raw post-agg callables in a Measure (which applies ColumnScope) so the existing nested-aggregation machinery activates. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements phases 1+2 of ADR 0001. Replaces the curated calc-measure AST (
MeasureRef/AllOf/BinOp/MethodCall/AggregationExpr) with an ibis-tree analyzer.Net diff: ~-727 lines in production code.
Highlights
xo.case, windows,xo.ifelse,.fillna().cast(), struct/array methods).t.all(measure)builds a real totals aggregation — fixes overall-mean/quantile/etc.t.all(...)lift to anonymous base measures.depends_onsurvives serialization.IbisCalcScope.compile_all.pydeleted; nested-array helpers moved tonested_compile.py.Phase 3 — not in this PR
SemanticMutateOpand.mutate(**post)still exist. Phase 3 (drop the operator, desugar.mutateto.with_measures(...).aggregate(...), delete the mutate-aware planner branches) is enumerated with file:line targets in the ADR's readiness checklist — separate branch.Test plan
read_parquet)mean/median/min/max), inline-reduction lift routing, multi-AllOf shared-totals SQL shape, FrozenSlotted hash round-trip,TotalsNotAvailableErrorclear-error pathOne behavior change to flag
Calc lambdas execute twice per query (classification + compilation). Pure expressions: no observable change. Lambdas with side effects (logging, counters): fire twice. Documented in ADR § "Behavior changes for users."
🤖 Generated with Claude Code