fix: fan-out-safe t.all() totals and loud, ownership-based filter pus…#283
Merged
Conversation
…hdown Fixes three silent-wrong-answer defects in the join_many pre-aggregation path, found during a soundness evaluation of v0.3.15: 1. t.all(...) totals were built by re-running agg specs on the fanned-out joined table, inflating denominators (percent_of_total summed to 0.5 with 2 line items per order). The pre-agg path now captures each (filtered) source table plus its original measure expressions and passes a lazy fan-out-safe totals builder through _apply_calc_specs → apply_calc_measures(totals_base_builder=...): each table aggregates at zero grain — where fan-out cannot occur — and the one-row results are cross-joined. Correct for sum/count, mean, and count-distinct totals. 2. Filters were silently dropped or mis-pushed. Root cause: _Resolver .__getitem__ ignored the dimension map, so t["orders.status"] raised and the entire filtered-join build collapsed to tbl=None inside a broad except, discarding the filter. Bracket access on _Resolver / _AggResolver now falls back to declared dims/measures (materialized columns still win, preserving post-agg stale-dimension semantics), and pre-agg filter pushdown is ownership-based: each filter resolves against every source table's dims (bare and table-prefixed); it is pushed to a raw table only when exactly one table owns it, ambiguous filters apply through the filtered joined table's key bridge, and a filter handled by neither path raises ValueError instead of being silently ignored. The deferred-join path's filter application and _find_deferrable_joins' reference detection get the same treatment. 3. find_join_in_tree treated the wrapper SemanticTableOp created by SemanticJoin.with_measures()/with_dimensions() as a leaf, so a .filter() between the wrapper and .aggregate() silently skipped pre-aggregation and returned fanned-out sums. The wrapper's _source_join is now followed. Regression coverage in test_preagg_filters_and_totals.py: totals under uneven 1:N fan (sum and mean), totals with filters, prefixed / bare- ambiguous / single-owner filter pushdown, unresolvable-filter error, and prefixed filters on the join_one path. Co-Authored-By: Claude Fable 5 <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
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.
…hdown
Fixes three silent-wrong-answer defects in the join_many pre-aggregation path, found during a soundness evaluation of v0.3.15:
t.all(...) totals were built by re-running agg specs on the fanned-out joined table, inflating denominators (percent_of_total summed to 0.5 with 2 line items per order). The pre-agg path now captures each (filtered) source table plus its original measure expressions and passes a lazy fan-out-safe totals builder through _apply_calc_specs → apply_calc_measures(totals_base_builder=...): each table aggregates at zero grain — where fan-out cannot occur — and the one-row results are cross-joined. Correct for sum/count, mean, and count-distinct totals.
Filters were silently dropped or mis-pushed. Root cause: _Resolver .getitem ignored the dimension map, so t["orders.status"] raised and the entire filtered-join build collapsed to tbl=None inside a broad except, discarding the filter. Bracket access on _Resolver / _AggResolver now falls back to declared dims/measures (materialized columns still win, preserving post-agg stale-dimension semantics), and pre-agg filter pushdown is ownership-based: each filter resolves against every source table's dims (bare and table-prefixed); it is pushed to a raw table only when exactly one table owns it, ambiguous filters apply through the filtered joined table's key bridge, and a filter handled by neither path raises ValueError instead of being silently ignored. The deferred-join path's filter application and _find_deferrable_joins' reference detection get the same treatment.
find_join_in_tree treated the wrapper SemanticTableOp created by SemanticJoin.with_measures()/with_dimensions() as a leaf, so a .filter() between the wrapper and .aggregate() silently skipped pre-aggregation and returned fanned-out sums. The wrapper's _source_join is now followed.
Regression coverage in test_preagg_filters_and_totals.py: totals under uneven 1:N fan (sum and mean), totals with filters, prefixed / bare- ambiguous / single-owner filter pushdown, unresolvable-filter error, and prefixed filters on the join_one path.