fix: soundness round#284
Merged
Merged
Conversation
SemanticJoin/SemanticFilter/SemanticAggregate.join_one defaulted to "inner" while SemanticModel.join_one, ops.SemanticTableOp.join_one and api.join_one default to "left". With a right table missing keys, a semantically no-op .filter() before .join_one() silently flipped the join type and changed totals (350 -> 300). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
_resolve_selector returned [] on any exception, and [] means "index all dimensions and columns" — so a typo'd selector silently profiled the whole table. String selectors now validate against merged dimensions + physical columns (so prefixed names like "flights.carrier" index just that field instead of falling back to everything), and callable/ibis selectors propagate their errors. Plain lambdas returning a list of columns are now resolved by calling them, which the previous select() path never supported. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Two defects in the deferred join_one path: 1. Deferral attached dim labels to entity-grain rows without re-aggregating, so group_by on a coarser right-table attribute alone (e.g. customers.region) returned one row per entity with duplicate dim values (east 220 / west 80 / east 50 instead of east 270 / west 80). Deferral now requires the requested group keys to pin the entity grain; coarser groupings fall back to the pre-agg path, which re-groups correctly. 2. A derived dim referencing another derived dim failed to resolve on the raw dim table and 'except Exception: pass' silently dropped the requested dimension, returning unlabeled rows at a hidden grain. Dependencies are now materialized via _mutate_dimensions_with_dependencies, and remaining failures raise. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ation MEAN decomposition built sum(arg)/count(arg) from mean_op.arg and never consulted mean_op.where, so a conditional mean silently became an unconditional mean on every joined grain (flat models were unaffected). Both decomposed legs now carry the where clause. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The pre-agg engine recombines per-table aggregates with plain == equi-joins on the group keys. A NULL group key — a real NULL dimension value, or one minted by the left join for parents with no children — never satisfies ==, so every table except the join spine lost its value for the NULL group, and because the spine follows the first listed measure, the same query returned different numbers depending on measure order in .aggregate(). join_tables (nested_compile), _rejoin_one and _left_join_bridge now use identical_to (IS NOT DISTINCT FROM). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
_reagg_op_for_expr defaulted to "sum" for any unrecognized reduction, so median/stddev/variance and compound base measures (sum()/count() ratios) were silently summed per join key whenever a cross-table group-by forced re-aggregation (median 180 vs 90; stddev/var -> NULL; aov doubled). The classifier now returns None for non-decomposable expressions, and those measures are aggregated at the exact requested grain through a (group keys -> join keys) bridge built from the joined table — each raw row participates once per group it maps to, matching the decomposable path's join-participation semantics. Unbridgeable cases raise instead of degrading. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
merged_dimensions only carries prefixed names, so a filter written with a bare derived-dim name (t.size where orders declares size) failed to resolve on the joined table, was pushed only to its owning table, and the dim bridge was then built from the UNFILTERED join: sibling tables' measures ignored the filter and filtered-out groups came back as ghost rows with NaN measures. The joined-table resolver now exposes bare aliases for prefixed dims when the suffix is unambiguous and not a physical column; ambiguous suffixes still hit the loud ownership check. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A predicate mixing tables — (t["orders.status"]=="open") & (t.qty>=2) — has no single owner, so the many side was restricted only through a join-KEY bridge: every item of any qualifying order survived and items measures were silently inflated (the same query as two chained .filter() calls gave different numbers). Top-level AND chains are now split into legs; each leg's source tables are determined by field provenance (walking projection values down to base relations), and single-table legs are inlined to base columns and re-applied to the owning raw table at row grain. Legs that genuinely span tables (cross-table OR) raise for many-side tables with requested measures instead of silently inflating them. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
SemanticGroupBy inherited SemanticTable.filter, which wrapped the group-by op in a SemanticFilterOp; aggregate() only recovers keys from its direct source, so the requested grouping was silently discarded and one unlabeled global row came back. Filtering between group_by and aggregate is pre-aggregation row filtering, so SemanticGroupBy.filter now commutes it below the grouping and re-groups with the same keys. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Two defects: 1. A derived dimension used as a group key was materialized on the raw table under its SHORT name, so the pre-agg grain never matched the prefixed group-by keys, the dim bridge found no common columns, and the key column silently vanished from the output (unlabeled rows); with a many-side measure added it crashed instead. Derived group-key dims are now materialized under the requested prefixed name so the pre-agg grain matches the group-by exactly. 2. Dim lambdas reference sibling dims by bare name (t.region_band), but merged dimension maps key them by prefixed name on joins, so dependency resolution failed and derived-of-derived dims either raised or (pre-fix) knocked the whole join build into the chasm fallback. _mutate_dimensions_with_dependencies now aliases unambiguous suffixes, and the per-table grain loop resolves dependencies before giving up. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The zero-fact-rows shortcut (#224) probed filters with a tracking proxy over the raw dimension table, so the qualified spelling BSL's own error messages recommend (t["customers.region"]) failed the probe, silently disabled the shortcut, and dimension members with no fact rows vanished from the result. The probe and the filter application now go through the table-scoped resolver (bare + prefixed dims), and the validation set accepts both spellings. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
to_tagged/from_tagged of a join_many(...).with_measures()/ with_dimensions() model dropped _source_join — extract_op_tree only walked source/left/right — so the reconstructed model executed on the lowered fanned-out join: sums doubled (220 -> 420), means shifted, and t.all() denominators went back to the fanned-out total, silently undoing the pre-agg fixes after any round trip. The wrapper extractor now serializes the join tree under source_join, and reconstruction rebuilds the SemanticModel around the reconstructed join with _source_join restored. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… swapping to_tagged tags the LOWERED expression; for queries lowered through the pre-agg path, _split_join_expr recovers partial-aggregate/key-bridge tables instead of the raw join leaves. Most shapes crashed with confusing AttributeErrors, but when shapes aligned the wrong table landed in a leaf slot and the query silently returned wrong rows (item_count 4 instead of 2). Reconstructed leaves are now validated: each declared dimension/measure must resolve against its recovered table (missing-column failures only), with a clear error pointing at model-level serialization. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Three sinks in the totals machinery could substitute a per-group value for the grand total when any guarded step failed (every share becomes 1.0 with no error): - expression rewrite: the unprefixed else-col_name fallback masked the unresolved-reference guard, which was dead code for exactly this case; only prefixed totals columns are substituted now - attach_windowed_totals: agg-spec evaluation / windowing failures skipped the totals column with a debug log; they now raise TotalsNotAvailableError - calc-of-calc totals: same continue-on-failure, same fix No natural trigger exists on this branch (fault injection confirmed the sink), but any future .over() regression or spec-eval failure would have landed here silently. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
23 tests pinning every confirmed defect from the July 2026 round-2 soundness evaluation against pandas-derived ground truth: A1/A2 (non-decomposable re-agg), A3 (conditional mean), B1/B2 (filter routing), B3 (grouping through filter), B4 (derived group keys), C1 (NULL group keys, measure-order independence), D1 (join_one defaults), E1/E2 (serialization round-trips), F1 (deferred-join grain), F2 (dimension-only shortcut), F3 (index selector). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The dimensional-indexing example indexed s.cols("carrier",
"airports__state"), but the join-prefixed field is "airports.state"
(dot, matching the Malloy equivalent). "airports__state" matched no
field. This was masked until the round-2 F3 fix made index() raise on a
no-match selector instead of silently indexing every field, so the
example (run in CI via `make examples`) now fails loudly.
Co-Authored-By: Claude Opus 4.8 (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
2 participants
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.