Add aggregate flag to /sql/measures/v3/ for raw-row output#2222
Open
jsogarro wants to merge 1 commit into
Open
Add aggregate flag to /sql/measures/v3/ for raw-row output#2222jsogarro wants to merge 1 commit into
jsogarro wants to merge 1 commit into
Conversation
Adds aggregate: bool = True query parameter to /sql/measures/v3/. When False, the endpoint emits a flat SELECT with no GROUP BY, projects raw column references using v2-style amenable aliases (<parent_node>_DOT_<column>), and bypasses pre-aggregation table matching. metric_formulas[].combiner is still populated so callers can re-aggregate the raw output downstream. Default True preserves all existing behavior. This is the v3 equivalent of v2's preaggregate=False mode. It exists for callers that apply their own aggregation downstream over the raw row-level output, where v3's mandatory server-side aggregation would double-count. Implementation: - New BuildContext.aggregate field, threaded through setup_build_context and build_measures_sql. - Matcher gate at measures.py:1846 adds 'ctx.aggregate and' to the existing condition. use_materialized is intentionally NOT reused because it is dual-purpose: it also controls parent-transform materialization via should_use_materialized_table. - Single early-exit projection branch in build_grain_group_sql handles all aggregability levels uniformly when ctx.aggregate is False. - skip_aggregation=True is OR'd with the existing condition in build_select_ast, reusing the working no-GROUP-BY path. Tests: 5 new tests covering binary indicator, additive numeric, multi-metric combiner preservation, default backward compat, and route-level integration. All 126 measures_sql_test.py tests pass (121 existing + 5 new), all 544 build_v3 tests pass, pre-commit clean.
✅ Deploy Preview for thriving-cassata-78ae72 canceled.
|
Collaborator
|
@jsogarro thanks for adding this -- it might be good to merge this anyway as a fallback, even if it's not used immediately. I'll go through it more closely later today |
Author
Sounds good. Thank you! |
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.
PR: Add
aggregateflag to/sql/measures/v3/for raw-row outputTarget:
DataJunction/dj, base branchmainBranch:
feature/jogarro/measures-v3-preaggregate-falseType: Draft
Summary
Adds an
aggregate: bool = Truequery parameter to/sql/measures/v3/. The default is fully backward compatible. Whenaggregate=false, the endpoint:SELECTwith noGROUP BY<parent_node>_DOT_<column>) instead of synthetic<col>_sum_<hash>namesmetric_formulas[].combiner(pointing at the raw alias) so the caller can aggregate the raw output downstreamThis is the v3 equivalent of v2's
preaggregate=Falsemode. It exists for callers that apply their own aggregation downstream over the raw row-level output, where v3's server-side aggregation would double-count.Why
A downstream consumer of
/sql/measures/v2(withpreaggregate=false) hit a 7-8x metric over-count when migrating to/sql/measures/v3. The root cause: the consumer's downstream analysis layer applies its own aggregation over raw indicator values (e.g.,SUM(is_retained_end) / COUNT(accounts)whereis_retained_end ∈ {0, 1}), and v3's mandatory server-side aggregation stacked under that outer aggregation produced inflated sums. The metricSUM(is_retained_end)displayed as0.9048under v2 +preaggregate=falseand6.4107under v3 — same query, same data, ~640% inflation purely from the doubled aggregation.The consumer cannot reshape v3's output back to the v2 shape because v3's synthetic column naming and server-side
GROUP BYdestroy the per-row signal before it ever leaves DJ. They need v3 to support a mode that does not pre-aggregate.Changes
Behavior:
aggregate: bool = Query(True)on/sql/measures/v3/. Default preserves all existing behavior byte-for-byte.aggregate: bool = Truefield onBuildContext. Threaded throughsetup_build_contextandbuild_measures_sql.ctx.aggregate=False:measures.py:1846is gated and never fires (raw rows cannot be served from a pre-agg).build_grain_group_sqlprojectsmake_column_ref(component.expression)instead ofbuild_component_expression(component)(noSUM/COUNTwrapping).amenable_name(parent_node.name + "." + component.expression), producing v2-style names likev3_DOT_page_views_enriched_DOT_is_product_view.skip_aggregation=Trueis passed tobuild_select_ast, which already has a working no-GROUP BYpath for non-decomposable metrics.metric_formulas[].combineris populated with<aggregation>(<v2-style alias>)(e.g.,SUM(v3_DOT_page_views_enriched_DOT_is_product_view)).Out of scope (deferred to follow-up PRs):
/sql/measures/v3/combined— semantics ofaggregate=falseon the combined endpoint are not obvious (combined output requires alignment that aggregation provides)./sql/preaggregationsroute — unchanged.datajunction-clients/python/) — unchanged.Design decisions
aggregate, notpreaggregateoruse_preagg_tablespreaggregatewould collide with v2's identically-named param (different semantics on each endpoint).use_preagg_tablesalready exists on/combinedwith a narrower meaning (table sourcing only, not GROUP BY emission).aggregatenames the actual behavior: when False, no server-side aggregation. Default-True preserves backward compat.BuildContext.aggregatefield instead of reusinguse_materializeduse_materializedis dual-purpose: it controls both pre-agg table matching (preagg_matcher.py:72-73) AND parent-transform materialization (materialization.py:112-182,cte.py:2012,2053,measures.py:1463). Flippinguse_materialized=Falseto bypass the matcher would also force every parent transform to be recomputed via CTE instead of read from its materialized table — large perf regression. New field isolates the concern.make_column_ref(component.expression)pattern the existing NONE-aggregability path uses atmeasures.py:1902-1911. Trade-off: aggregability-specific paths are short-circuited entirely in raw mode, which is the correct semantics for the target use case (callers want raw rows regardless of aggregability level).amenable_name(parent_node.name + SEPARATOR + component.expression)naming.py:38-50. Output is<parent_node>_DOT_<column>(e.g.,v3_DOT_page_views_enriched_DOT_is_product_view), matching the v2 column naming that the v2-pinned consumer already consumes. Minimizes downstream migration work.measures.py:1846). A method onBuildContextwould add indirection without reducing duplication. The inline guard with a comment explaining whyuse_materializedis intentionally left alone is clearer.Test plan
5 new test cases in
tests/construction/build_v3/measures_sql_test.py:TestMeasuresSQLAggregateFalse::test_binary_indicator_no_group_bySUM(is_product_view)on a 0/1 indicator withaggregate=False: SQL projects rawt1.is_product_viewwith v2-style alias, noGROUP BY,combinerisSUM(<raw_alias>). Regression test for the production over-count.TestMeasuresSQLAggregateFalse::test_additive_numeric_metricSUM(line_total)onaggregate=False: same flat-SELECT / raw-column shape.TestMeasuresSQLAggregateFalse::test_multi_metric_preserves_each_combinerv3.total_revenue,v3.max_unit_price,v3.min_unit_price): each metric's combiner is preserved and references its raw v2-style column alias.TestMeasuresSQLAggregateFalse::test_default_aggregate_true_unchangedaggregate=Trueproduce identical responses withGROUP BYpresent and synthetic hash aliases.TestMeasuresSQLAggregateFalseEndpoint::test_route_aggregate_false_full_response_shapegrain_groups,metric_formulas,dialect,requested_dimensions); per-component fields reflect raw-row semantics.Verified test results:
No existing test was modified; no regression observed across the v3 construction or v3 cube tests.
Backward compatibility
True.setup_build_context/build_measures_sqluses keyword arguments withTruedefaults, so any existing caller of these helpers compiles unchanged.measures.py:1846adds oneand ctx.aggregateterm to the existing condition; whenaggregate=True(default), the condition is identical to before.not ctx.aggregate— whenaggregate=True, control falls through to the existing aggregability-specific branches unchanged.Rollback
Revert the merge commit. No data migration, no config change, no feature flag — the new parameter is the only behavior toggle and defaults to current behavior.
Notes for reviewers
semantic_entityfield on component columns still references the synthetic<col>_sum_<hash>name whenaggregate=False(e.g.,v3.product_view_count:is_product_view_sum_eb3a4b41). This is intentional —semantic_entityis for DJ's internal identity matching and is independent of the projected column name. Happy to revisit if reviewers prefer it to track the v2-style alias.grainandcomponentsarrays are not produced underaggregate=False; the response keepsgrainpopulated with the requested dimensions andcomponentspopulated with the (renamed) raw-column entries. The plan we wrote internally initially considered emptying both fields, but the test output showed that preserving them gives the caller more useful metadata without breaking any existing consumer assumption./sql/measures/v3/./sql/measures/v3/combinedis not in scope; semantics of "raw rows across multiple aligned grain groups" are not obvious and deserve a separate design discussion.Manual Test Plan
Prerequisites
DataJunction/dj(or the relevant fork) and check outfeature/jogarro/measures-v3-preaggregate-false.cd datajunction-server && uv sync --group test --extra transpilation.Test Steps
Step 1: Run the new test class
Expected: 5 passed, 0 failed.
Step 2: Run the full measures_sql test file (regression check)
Expected: 126 passed (121 existing + 5 new), 0 failed.
Step 3: Run the entire build_v3 test directory
Expected: 544 passed, 1 xfailed (pre-existing).
Step 4: Pre-commit / lint
cd .. uv run pre-commit run --files \ datajunction-server/datajunction_server/api/sql.py \ datajunction-server/datajunction_server/construction/build_v3/builder.py \ datajunction-server/datajunction_server/construction/build_v3/measures.py \ datajunction-server/datajunction_server/construction/build_v3/types.py \ datajunction-server/tests/construction/build_v3/measures_sql_test.pyExpected: All hooks pass.
Verification Queries
After standing up the patched server locally, call:
Edge Cases
aggregate=falsewith multiple metrics from the same parent fact (verified bytest_multi_metric_preserves_each_combiner).aggregate=falsecombined withuse_materialized=true— matcher bypass takes precedence (verified: matcher gate isctx.aggregate and ctx.use_materialized and ctx.available_preaggs).aggregate=true(default) is identical to pre-PR behavior (verified bytest_default_aggregate_true_unchanged).aggregate=falsewith no dimensions (global raw projection) — verified bytest_multi_metric_preserves_each_combiner(calls withdimensions=[]).Cleanup
No external resources allocated. Test container is torn down by the pytest fixture.