[pull] master from cube-js:master - #692
Merged
Merged
Conversation
…view (#11659) * docs: show the AOV measure owned by a cube, not only by a view 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 * test(testing): cover the multi-fact AOV ratio end to end 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 * test: pin the view-without-the-other-fact claim; make both filters bite 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 * test(testing): read the planner flag the way the server does; review 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 * test(cubejs-testing): run the multi-fact smoke suite on both CI legs 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 * test(schema-compiler): correct stale module header on the AOV spec 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 --------- Co-authored-by: Claude <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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
See Commits and Changes for more details.
Created by
pull[bot] (v2.0.0-alpha.4)
Can you help keep this open source service alive? 💖 Please sponsor : )