fix: soundness round4#288
Draft
hussainsultan wants to merge 12 commits into
Draft
Conversation
Filter strings, dict predicates, and agent query strings previously built literals with whichever ibis was importable (xorq-vendored when installed), mis-composing against tables of the other flavor: equality silently yielded a constant predicate, ordering raised. Pick the flavor from the actual table at resolve time, memoize per module, and reject expressions that resolve to a Python bool (the cross-flavor identity-comparison footgun). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Apply pre-agg filters before grain transforms: range and lambda filters
compared grain-truncated buckets instead of raw values, silently
dropping in-range rows (week grain corrupted both directions).
- Date-only time_range ends now cover the whole end day (exclusive bound
at next midnight); explicit time endpoints keep <= semantics.
- smallest_time_grain long form ("TIME_GRAIN_DAY") was double-prefixed
in _validate_time_grain, silently disabling grain validation.
- in/not-in dict filters reject bare-string values instead of matching
individual characters ('not in "gold"' returned the gold rows).
- Only complete ISO date/datetime strings are coerced to typed literals;
partial strings like "2024" no longer inherit today's month/day.
- limit=0 means LIMIT 0, not "no limit"; bool limits rejected.
- order_by directions validated; 'descending'/'ascending' accepted,
anything else raises instead of silently sorting ascending.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- with_measures now evicts a same-named measure of the other kind: base-first lookup silently served the old definition when a redefinition was classified as calc. Self-referential redefinitions raise instead of resolving to the old measure. - get_ibis_module unwraps the resolver proxies passed to filter and dimension callables, so the bool-guard's remediation (build literals with the table's flavor) actually works inside a filter lambda; the error message now shows the constant-predicate escape hatch. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ures A dimension sharing its name with a raw column (amount=lambda t: t.amount*2) was materialized over the column before filter resolution, so lambda filters applied the expression twice while dict/string filters applied it once, and any filter's presence made measures silently aggregate the mutated column. - Filter enrichment no longer materializes dimensions over existing columns (overwrite_existing=False): the resolver applies the dimension expression exactly once against raw values, and measures keep reading the raw column. All three filter spellings now agree. - Dict/string filter callables resolve against the dimension-aware resolver instead of unwrapping it through _ensure_xorq_table. - group_by on a shadowing dimension combined with a measure that reads the shadowed column is inherently ambiguous and now raises with a rename suggestion; identity dimensions and measures over other columns are unaffected. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The final column selection silently filtered requested group keys and measures to whatever the pre-agg result happened to contain. On cross-joined models (no dimension bridge) this returned a result that ignored the requested grouping entirely — one unlabeled grand-total row in place of per-group rows. Missing requested columns now raise with an explanation and restructuring guidance; de-fanned cross-join grand totals are unaffected. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Pre-aggregation built many-side measures on the RAW unjoined table unless cross-table filter routing happened to force a join-key bridge. Orphan rows (NULL FK, or a key matching no left-side row) were silently counted in grand totals and many-side-only group-bys while mixed group-bys excluded them, so sum-over-groups stopped matching the ungrouped grand total on the same model. The key-bridge inner join against the joined table's distinct join keys now applies unconditionally to every join_many table with requested measures, not only when needs_bridge is set. When no shared join-key column is available (or in the chasm fallback with no root-side key to bridge through) the query raises instead of silently counting rows the LEFT JOIN can never produce. Flights star-schema grand-total expectations updated to LEFT JOIN ground truth (14 SCE-origin flights match no airport row); verified against raw SQL. Regression suite: test_soundness_round4_join_many.py, including a NULL-dimension-value case guarding the round-2 C1 fix. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…tions
lift_inline_reductions rewrote EVERY WindowFunction whose func was a
lifted base-column reduction to Field(totals_vt, anon) — the t.all()
grand-totals shape — without checking whether the window was empty.
A measure like
t.amount.sum() / t.amount.sum().over(window(group_by=t.region))
silently returned shares of the GRAND total instead of per-region
shares (the emitted SQL had no PARTITION BY).
Route each windowed base reduction by shape instead:
- empty window: grand totals, unchanged (ADR 0001 t.all semantics) —
except when the window is the whole calc expression and the reduction
reads only group-key columns, where base-row totals and output-row
windows are both plausible readings; that ambiguous shape now raises.
- non-empty window, decomposable reduction (sum/count/min/max/any/all)
with every window key a group key of the aggregation: re-aggregate
the lifted per-group value over the output rows inside the same
window, partition/order keys remapped to output columns (count
re-aggregates with sum). Mirrors the already-correct measure-
reference window path.
- anything else (non-decomposable reduction, non-group-key window key):
raise WindowedBaseReductionError pointing at the measure-reference
form. _compile_aggregation re-raises it past the apply-time fallback
so it cannot be silently re-evaluated at different grain.
No silent wrong answers remain for this family: every non-empty-window
case either computes the ground-truth value or raises loudly.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
nest= lambdas received a scope over raw columns only: the canonical
measure-name form (t.group_by("sku").aggregate("total_qty")) raised
column-not-found, and inner aggregations whose result names collided
with raw columns executed but were silently replaced by one raw struct
per source row.
Nest lambdas now receive the aggregation's semantic source table, so
measure names, dimension names, and inline lambdas/Deferreds resolve
exactly like a top-level aggregate:
- t.group_by(...).aggregate(...) becomes a NestAggSpec: the inner query
is re-grouped at (outer keys + inner keys) grain over its own source
chain (inner filters included) and compiled as its own semantic
aggregation; SemanticAggregateOp._to_untagged_with_nest collects its
rows into one array-of-structs per outer group and attaches them with
a null-safe left join on the outer keys (NULL dimension groups keep
their rows; groups the inner filter emptied keep a NULL array).
- Bare t.group_by(...) keeps the pinned per-row struct collection
(duplicates included); group_by now also flattens list arguments so
the documented list form works on semantic tables.
- Unsupported nested shapes (order_by/limit, transformed bare group_by,
raw ibis returns) raise NotImplementedError naming the shape — silent
raw-row structs are gone.
Adds test_soundness_round4_nest.py pinning canonical, aliased-inline,
raw-name-collision, multi-key, filtered-outer, inner-filter, no-outer-
key, and bare forms against pandas ground truth, plus the loud errors.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The rebase reconciled our NestAggSpec pipeline with main's parallel nest fix (1e7fa98), adopting its filter/order_by/limit support after the inner aggregate. The regression test that pinned order_by-raises is now a positive ordered-array assertion. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
hussainsultan
force-pushed
the
fix/soundness-round4
branch
from
July 18, 2026 14:03
fa2fe3a to
e277287
Compare
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.
No description provided.