Fix preagg registration identity#2369
Draft
shangyian wants to merge 7 commits into
Draft
Conversation
find_matching_preagg matched a metric's measure to a pre-agg measure purely by compute_expression_hash(component.expression), which hashes only the inner expression and ignores the Phase-1 aggregation. So SUM(x) and MAX(x) collided on the same hash: a MAX/MIN metric would bind to a SUM-built pre-agg column and compute MAX(sum) instead of MAX(row) -- silently wrong (and overstated) results. Make the measure identity (expression hash, Phase-1 aggregation) on both sides of the match. PreAggMeasure already stores `aggregation`, so this is a match-time check only -- no re-registration. A SUM partial can no longer serve a MAX/MIN metric; such metrics fall back to the base fact (like non-additive metrics do). Additive SUM/COUNT metrics are unaffected. Regression test: v3.total_unit_price (SUM(unit_price)) vs v3.max_unit_price (MAX(unit_price)) sharing the inner expression -- the SUM metric still uses the agg, the MAX metric falls back to the base fact. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The metrics layer applies filters on projected (output) dimensions at the outer query but deliberately skips "filter-only" dimensions -- ones present in a grain group but rolled away -- expecting each grain group's CTE to have applied them. The source-built path does; build_grain_group_from_preagg did not, so a filter on a dimension that is in the pre-agg grain but not in the requested output was silently dropped: no WHERE was emitted and the query summed across all values (e.g. filtering country='US' still returned every country -- an over-count). Inject those filter-only predicates into the pre-agg scan, mapping each filtered dimension to its physical (possibly remapped) column and filtering before the GROUP BY roll-up, mirroring the source path. Filters on projected dimensions are untouched (the outer query still applies them, no double-apply). A filter on a column absent from the pre-agg grain already forces a fallback to source. Regression test: filter on a rolled-up, column-mapped dimension is emitted as `WHERE <physical_col> = ...` inside the CTE, in both measures and metrics SQL. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
✅ Deploy Preview for thriving-cassata-78ae72 canceled.
|
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.
Summary
Test Plan
make checkpassesmake testshows 100% unit test coverageDeployment Plan