docs: show the multi-fact AOV measure owned by a cube, not only by a view - #11659
Conversation
A metric spanning two facts does not need a view to live in. A cube measure that references another cube's measure is derived rather than owned by its cube - the same property a view measure has - so AOV can sit on either fact cube, and every view including it gets it. The recipe said "neither cube can define AOV - neither can reference the other's measures", which is wrong; the views and view-reference pages said such a metric has no single cube to live in, which overstates it the same way. All three corrected, and the recipe gains a section comparing the two placements on reuse, coupling and query path. Test: the same ratio on `sales_line_item`, pinning that both placements plan identically (same legs, same stitch, same final division), that it is reachable through a view and reaches the other fact even when the query names only its own cube, and that `multi_stage` is required exactly as on the view. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RLzMjTkAfG9tjDmjVR3EL2
The schema-compiler tests pin the SQL shape; nothing ran the query. This adds a birdbox smoke test that asks a real Cube instance for AOV over DuckDB and checks the number, for both placements - the ratio owned by the view and the same ratio owned by the line-item cube, the latter both directly and through a view that includes it. The fixture is built so each way of getting it wrong lands somewhere different, and the assertion says which invariant broke: West sales 100, transactions 2 (T100 spans three lines) -> 50 East sales 60, transactions 1 (T201 is an EXCHANGE) -> 60 counting lines instead of transactions -> West 100/4 = 25 letting the join multiply the sum -> West 400/2 = 200 dropping the cube's measure filters -> East 60/2 = 30 Runs in the existing integration-smoke job via smoke.sh. Multi-fact is Tesseract-only, so the suite skips when CUBEJS_TESSERACT_SQL_PLANNER is explicitly 'false' - not merely unset, since Tesseract is the default. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RLzMjTkAfG9tjDmjVR3EL2
|
Claude finished @paveltiunov's task in 3m 40s —— View job Review: docs correction + multi-fact AOV coverage — no blockers; 1 medium (untested docs claim), 5 low/nits, posted inlineFull review
What's goodThe correction itself is right and well-argued: a cube measure referencing another cube's measure is derived in the same sense a view measure is, so the earlier "neither cube can define AOV" was too strong. The three overstatements are fixed consistently across recipe, guide and reference, and the new comparison table is the useful part — reuse vs coupling vs query path is the actual decision, not a capability limit. The fixture data is the standout. Each way of getting AOV wrong lands on a distinct number (25 for counting lines, 200 for a fan-out multiply, 30 for dropped filters), so a smoke failure names the broken invariant instead of just "expected 50". Worth keeping that property in mind for future edits to the fixture. Wiring checks out: Findings
One more note on the schema-compiler side: Not verifiedI could not execute either suite — this checkout has no |
Two review findings. The recipe says a cube-owned measure reaches the other fact whether or not the view naming it includes that fact, and nothing pinned it - every view in both models exposed sales_amount alongside the ratio. Adds `line_item_analysis`, a view carrying only the cube-owned ratio and a shared dimension, and a test that the ratio still divides the two aggregates through it. In the birdbox fixture every row was IN_STORE, so the channel filter was inert: dropping it changed no asserted number, while the header comment claimed both filters were load-bearing. Adds an ONLINE transaction to East, which leaves every expected value where it was and makes each filter move East's denominator on its own - checked by removing each in turn against DuckDB: 60 as written, 30 with either one gone. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RLzMjTkAfG9tjDmjVR3EL2
|
Claude finished @paveltiunov's task in 1m 6s —— View job Re-review of
|
| with both filters | drop transaction_type |
drop channel | |
|---|---|---|---|
| East denominator | T200 → 1 | T200, T201 → 2 | T200, T202 → 2 |
| East AOV | 60 | 30 | 30 |
West is untouched (2 → 50), ungrouped total is still 3 transactions over 160 dollars, and salesAmount comes from the other cube so East stays 60. Every expected value in the suite is unchanged, which is what makes this a clean strengthening rather than a re-baseline. The updated header comment now names the two drops separately instead of lumping them as "the cube's measure filters" — matching what the data actually distinguishes. Thread resolved.
One note: only the schema-compiler model gained the fact-free view; the birdbox RetailAnalysis still includes salesAmount alongside aovBasketFromCube. That's fine — the planner-level assertion is where the claim lives, and an end-to-end duplicate would cost a query for no extra invariant.
Still open (re-affirmed, no new comments posted)
All four were re-checked against the current file contents and still apply verbatim; the unresolved threads carry the detail.
| Where | What | |
|---|---|---|
| Low | smoke-multi-fact.test.ts:127 |
unguarded rawData()[0] turns an empty result into a TypeError |
| Nit | smoke-multi-fact.test.ts:34 |
skip guard === 'false' vs the server's asBool() |
| Nit | RetailAnalysis.js:6 |
join_path mixed with camelCase props |
| Nit | smoke.sh:23 |
DuckDB-backed suite placed outside the DuckDB group |
Nothing new surfaced in this delta. No blockers.
Not verified
Same limitation as the previous pass: no node_modules in this checkout and the native planner isn't built, so neither suite was executed. The fixture arithmetic above is worked out by hand from the SQL and the measure filters, not from a run — though the commit message reports each filter was removed in turn against DuckDB and produced 30 both ways, which matches.
…nits
Four more review findings.
The skip guard compared the raw env string to 'false', while the server parses
it with asBool, which also treats 0/no/False as off. Read it through
getEnv('nativeSqlPlanner') instead, so the skip and the birdbox instance can
never disagree. Checked all three: unset runs, 'false' skips, '0' now skips
too - it did not before.
The ungrouped assertion indexed [0] straight off the result, so an empty
result - a plausible way for multi-fact stitching to fail - would have thrown
a TypeError instead of reporting the number. Asserts the row count first.
The view fixture mixed snake_case join_path into a camelCase file; the
sibling JS fixtures use joinPath. Re-checked the model against DuckDB after
the rename, values unchanged.
Moved smoke:multi-fact inside the DuckDB group in smoke.sh - it is
DuckDB-backed, and the whole script shares one 30 minute budget.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RLzMjTkAfG9tjDmjVR3EL2
|
Claude finished @paveltiunov's task in 2m 2s —— View job Re-review of the two follow-up commits — all 6 prior findings addressed; 1 optional suggestion posted inlineFull review
Prior findings — status
New commentOne optional suggestion on One stale commentThe module header of Still not verifiedI could not execute either suite in this checkout (no |
The suite was gated on the ambient CUBEJS_TESSERACT_SQL_PLANNER, which is correct in the sense that birdbox spreads process.env into the server env, so on the use_tesseract_sql_planner: false leg the server really did run the legacy planner. But nothing this suite asserts is matrix-dependent, so that gate just cost the coverage on half the matrix. Pin the planner on in the birdbox env instead - options.env is spread last in birdbox.ts, so it overrides the leg's setting - and drop the getEnv / describe.skip branch, matching smoke-shared-calc-group.test.ts. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RLzMjTkAfG9tjDmjVR3EL2
|
Claude finished @paveltiunov's task in 1m 25s —— View job Re-review of
|
| Mode | Env assembly | Pin wins? |
|---|---|---|
| CLI / LOCAL | {...process.env, ...defaults, ...options.env} (src/birdbox.ts:561-582) |
yes — options.env is last |
| DOCKER | dc.withEnvironment(options.env) (src/birdbox.ts:328) |
yes — process.env never spread |
REQUIRED_ENV_VARS copy-back loop (birdbox.ts:735) |
would overwrite listed keys from process.env |
no risk — REQUIRED_ENV_VARS.duckdb is [] (src/REQUIRED_ENV_VARS.ts:60) |
Placement matters and is right: CUBEJS_TESSERACT_SQL_PLANNER sits after ...DEFAULT_CONFIG, and DEFAULT_CONFIG doesn't carry that key anyway (test/smoke-tests.ts:6-14), so there's no ordering trap either way. The header comment's parenthetical ("birdbox spreads process.env first, so the pin wins over whatever the CI leg exports") describes the CLI/LOCAL path accurately, which is the one the smoke script actually uses.
The trade this makes is worth naming: the legacy leg now runs the suite under Tesseract too, so it's duplicate coverage rather than legacy coverage. That's correct here — the legacy planner cannot build a single join tree over two unrelated facts, so there is no legacy behaviour to assert, and the comment says exactly that instead of leaving it implicit.
smoke.sh placement is also settled — the suite now sits inside the DuckDB group with a line explaining why. Startup cost is the same shape as smoke-duckdb.test.ts, which takes the identical getBirdbox('duckdb', …) path (Postgres container spun up and unused in both).
No new findings. Nothing in this delta touches the docs or the fixture arithmetic verified in the previous pass.
One stale comment, unchanged from last pass
multi-fact-derived-measure-in-view.test.ts:17-18 still reads "The ratio of the two is authored as a measure of the view rather than per consumer" — accurate when the file only covered the view placement, slightly behind now that the model carries both. Purely a comment; not worth a commit on its own, but worth folding in if you touch the file again.
Not verified
Same limitation as both prior passes: no node_modules in this checkout and the native planner isn't built, so neither suite was executed. The env-precedence conclusions above come from reading src/birdbox.ts, not from a run — CI on this push is the actual check that the suite now passes on the use_tesseract_sql_planner: false leg.
The header described the file from when it only covered the view-owned placement. The model carries both placements now, so say so. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RLzMjTkAfG9tjDmjVR3EL2
|
Claude finished @paveltiunov's task in 36s —— View job Re-review of
|
| Claim in the header | Where it's actually true |
|---|---|
| on the view | retail_analysis.aov_basket, multi_stage: true (line 171) |
| on the line-item cube itself | sales_line_item.aov_basket, multi_stage: true (line 108) |
| from where a view can re-expose it | retail_analysis aliases it as aov_basket_from_cube (line 153), and line_item_analysis includes it bare (line 187) |
The "re-expose" clause is doing real work — both re-exposure shapes exist in the model, and the aliased one is why the normalize() in the plans-identically test has to collapse retail_analysis__aov_basket and sales_line_item__aov_basket. The rest of the paragraph (the multi-fact rationale, the shared-cube join structure) is unchanged and still correct.
Nothing else in the file changed — no test bodies, no model YAML, so nothing verified in the earlier passes is disturbed.
Threads
All 7 of my prior threads are resolved; three of them (smoke-multi-fact.test.ts ×2, RetailAnalysis.js, smoke.sh) are also marked outdated by the rewrites. Nothing left open from my side, and no human threads to leave alone.
Not verified
Same as all three prior passes: no node_modules in this checkout and the native planner isn't built, so the suites weren't executed. Immaterial for this delta — it changes only comment text, which cannot affect a run.
|
CI note — The job ends with: Two independent reasons this isn't the PR's:
This is the third artifact-download failure of this exact shape across this PR's runs, all transient and all on runs that were superseded by a newer push. No fix to port — there's no code change that makes GitHub's artifact storage retry successfully. I'm deliberately not re-running this job: it's pinned to an outdated commit, so a green result there would prove nothing about Generated by Claude Code |
Codecov Report✅ All modified and coverable lines are covered by tests.
Additional details and impacted files@@ Coverage Diff @@
## master #11659 +/- ##
===========================================
- Coverage 80.10% 58.15% -21.96%
===========================================
Files 490 237 -253
Lines 103670 18851 -84819
Branches 3670 3825 +155
===========================================
- Hits 83048 10962 -72086
+ Misses 20070 7335 -12735
- Partials 552 554 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
CI note — Three tests failed in this job, all the same way: Not this PR's, for two reasons — and I'll name the part that is mine:
The mechanism, and my part in it. No fix to port: nothing in this repo makes a shared dataset safe against overlapping runs, and the real remedy (per-run dataset isolation) is well outside this PR. I'm not re-running the failed job — it's pinned to an outdated commit, so green there would say nothing about Generated by Claude Code |
|
Correction + update: In my previous comment I said the current head's BigQuery leg would run "with no competing run". That was wrong. Run What the new failure shows — same three tests, same error class, different tables:
Different partitions and different hash suffixes each time. A defect in the diff would fail deterministically on the same table; a shared dataset losing tables under concurrent runs would not. That is evidence for contention, but with the overlap still present in this run, it is not yet proof, and I'm not going to claim it is. Two things still to discriminate, neither of which I'm asserting the outcome of:
For the record, the diff still touches nothing that could plausibly cause this: I'll report both results here rather than leaving this hanging. Generated by Claude Code |
|
CI note — Different failure from the BigQuery ones, so recording it separately. 37 passed, 1 failed: Not this PR's — by construction. The failure is in Why it's timing-sensitive.
p3 and p4 are 25 ms apart while each query runs for 50 ms, so which one the queue picks up first depends on real scheduling on a loaded shared runner. p3 and p4 swapped their positions — the values aren't wrong, the interleaving is. The two failures are also mutually consistent ( No fix to port: the test needs its 25 ms assumption relaxed or an ordering-independent assertion, which is a change to a package outside this PR's scope. I'm not going to widen the diff to touch the query-orchestrator test suite. Run Generated by Claude Code |
|
Both runs finished on The result that matters:
|
|
Both re-runs passed. Both diagnoses confirmed.
That was the test I committed to: pass on re-run confirms the diagnosis, identical repeat failure would have meant digging further. Both passed, so both stand — no code change is warranted for either, and none was made. Summary of the four CI failures on this PR, none of them the diff's:
The contention was avoidable and partly mine. Four heads of this PR had driver runs alive at once because I pushed four times in ~15 minutes, three of them small review fixes. Batching them would have avoided most of these runs and most of this thread. Two things worth knowing beyond this PR, neither of which I'm acting on here:
I'm done investigating CI here and won't post further unless something changes. Approved by @keydunov and green — the merge is yours to make. Generated by Claude Code |
Check List
Description of Changes Made
Follow-up to #11656. That PR showed the multi-fact AOV ratio only as a measure of the view, on the premise that a metric spanning two facts has nowhere else to live. That premise is wrong, and this corrects it.
A cube measure may reference another cube's measure. Doing so makes it derived rather than owned by its cube — the same property a view measure has — so AOV can sit on either fact cube:
Both placements plan identically: the same per-fact subqueries, stitched the same way, divided in the same final stage.
multi_stageis required either way, and without it both fail with the sameCan't find join path. What differs is reuse and coupling — a cube-owned metric is defined once for every view that includes it, at the cost of one cube naming another.Corrections
Three statements from #11656 overstated the case and are fixed here:
views.mdx: "there is no single cube it could belong to"view.mdx: "which therefore has no single cube to live in"Docs
recipes/data-modeling/average-order-value.mdx— new "Where to put the measure" section with the cube-owned form and a table comparing the two placements on reuse, coupling and query path, plus guidance on picking one.docs/data-modeling/views.mdx,reference/data-modeling/view.mdx— claims corrected, both pointing at the recipe's comparison.Unit test —
multi-fact-derived-measure-in-view.test.ts, now 19 testsThe same ratio added to
sales_line_item, alongside the view-owned one so a single model covers both placements. New block pins that the two plan identically (normalising only the emitted column alias), that the cube-owned measure is reachable through a view, that it reaches the other fact even when the query names only its own cube, that it works on the shared date spine, and thatmulti_stageis required exactly as on the view.A second view,
line_item_analysis, carries onlysales_line_item.aov_basketand a shared dimension — no measure ofitem_location_salesanywhere in it — pinning the recipe's claim that a view can expose AOV without exposing its numerator.Every YAML sample added here was extracted from the page, compiled, and its SQL checked against the native planner — including grafting the documented
aov_basketblock verbatim onto the recipe's own step-1 model.End-to-end test —
smoke-multi-fact.test.ts(6 tests), DuckDB birdboxThe unit tests pin the generated SQL; this one pins the numbers that SQL produces, against a running server and a real database. Fixture (
birdbox-fixtures/multi-fact/schema/) is inlineSELECTs — no tables to load — and the data is chosen so each way of getting it wrong lands on a distinct value:T100spans three lines)T201EXCHANGE,T202ONLINE)Counting lines gives West 25; letting the join multiply the sum gives 200; dropping either of the cube's two measure filters gives East 30. So a failure says which invariant broke rather than just that a number moved.
Runs under the existing DuckDB group in
smoke.sh(one extra birdbox start, no extra container). The planner is pinned on in the birdbox env, so it covers both legs of theuse_tesseract_sql_plannermatrix rather than skipping one.🤖 Generated with Claude Code
https://claude.ai/code/session_01RLzMjTkAfG9tjDmjVR3EL2
Generated by Claude Code