Conversation
✅ Deploy Preview for thriving-cassata-78ae72 canceled.
|
Fixes DataJunction#2489. A non-decomposable metric (a percentile, MAX_BY, ...) drags every metric sharing its parent down to a raw-grain grain group, so the CTE projects raw columns instead of applying each component's accumulate function. The final SELECT still applied the components' merge functions, which is only correct where accumulate and merge coincide: AVG(x) -> SUM(x) / SUM(x), silently 1.0 COUNT(x) -> SUM(x), adds up the values COUNT(*) -> SUM(alias.*), invalid SQL APPROX_COUNT_DISTINCT(x) -> sketch union over a raw column SUM/MIN/MAX -> unaffected Route those metrics down the branch that already serves genuinely non-decomposable metrics: emit the metric's own expression over the raw columns. The invariant is now carried explicitly as GrainGroupSQL.components_accumulated instead of being inferred from a component_aliases miss, which used to fall back to a raw column name and produce wrong SQL rather than failing. Also project each raw column once. Two components can read the same column (AVG needs SUM and COUNT), and a component can read a column the grain group already projects, both of which made references to it ambiguous. COUNT(*)'s "*" component is skipped entirely -- it reads no column, and projecting it re-projected every column of the parent. Adds a regression test per accumulate != merge shape, each paired with a control asserting the pre-aggregated form when the metric is requested alone, plus SUM alongside a percentile to pin the shape that was always correct.
approx_percentile(col, percentage) dispatched to the right infer_type overload and then died calling it: the accuracy parameter was annotated `ct.NumberType | None` but had no default, so a two-argument call raised `TypeError: infer_type() missing 1 required positional argument`. Creating a metric like `SELECT APPROX_PERCENTILE(price, 0.9) FROM ...` surfaced that as "Unknown TypeError on column ...". Give accuracy the `= None` default the other optional parameters in this module use. Also widen the scalar overload's percentage from FloatType to NumberType, matching PERCENTILE, so a DOUBLE percentage is accepted wherever a FLOAT one is.
Derived metrics are built by inlining their base metrics' expressions (build_intermediate_metric_expr), so the fix for DataJunction#2489 already carries them: v3.revenue_per_customer beside a percentile now divides by APPROX_COUNT_DISTINCT(customer_id) instead of unioning HLL sketches that were never built. Add the tests that pin this, and say so where the inlining happens -- it is no longer only about COUNT DISTINCT from _agg CTEs, it is what keeps the merge functions out of a raw-grain group. Tests cover a derived metric whose components are self-merging (v3.avg_order_value: SUM over COUNT DISTINCT, correct before and after -- its expression is byte-identical, only the grain group CTE lost the duplicate order_id projection) and one whose components are not (v3.revenue_per_customer), each alone and beside a percentile. The percentile fixture is now APPROX_PERCENTILE, the function that motivated the issue, and a PERCENTILE metric stays as a second non-decomposable aggregation so the behavior is visibly not tied to one function. Also assert an APPROX_PERCENTILE metric with an explicit accuracy argument can be created and builds SQL.
find_upstream_node_names ends its recursive CTE with SELECT DISTINCT and no ORDER BY, so parent_map's per-child list order is whatever the plan produced. For a derived metric that order decides which base metric resolves first, and with it the order their components are projected in the grain group CTE -- so the same request produced different SQL in CI than locally. Reversing every parent list reproduces CI's output exactly; with this change the SQL is byte-identical either way. Order the CTE, and sort the base metrics by name in get_base_metrics_for_derived so component order is a property of the metrics rather than of the graph walk that reached them. Sorting grain_group.components by name would also work, but it reorders thirteen existing expectations and replaces requested-metric order with alphabetical-by-hash in the measures response; this way nothing else moves. find_join_paths_batch had the same exposure with more at stake: row order set the frontier order and broke ties for a (source, dimension, role) key, where the first path found is the one kept. Two same-depth routes to one dimension sharing a role path would therefore join along whichever route the query happened to return first. Order those queries too, so the lowest dimension link id wins consistently. The new tests perturb parent_map rather than pinning a string: they build the SQL twice, once with every parent list reversed, and assert the two are identical.
shangyian
force-pushed
the
fix/single-level-accumulate
branch
from
September 3, 2026 22:51
068631b to
3b294fb
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.
Summary
Test Plan
make checkpassesmake testshows 100% unit test coverageDeployment Plan