feat(schema-compiler): compute a default measure drill-member set behind an env flag - #11488
Conversation
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
|
Claude finished @igorlukanin's task in 5m 19s —— View job Review: 1 medium-high, 1 medium, 3 low — solid design, one production-meta interaction to resolve. 4 inline comments posted.Full review
What's goodThe hook site is well chosen. Findings
1 is the one worth deciding before merge. A related design note, not filed inline: putting the primary key first means the default drill query groups at row grain, which no pre-aggregation will match. That's arguably what drill-down means, but it does make the default shape the most expensive one, and any consumer that truncates the list keeps the PK and drops the useful attributes. Two smaller notes:
Nothing security-relevant beyond finding 1 — no new user input reaches SQL, and the feature only reorders names already present in the compiled model. No performance concern in the compiler itself: one extra pass over each cube's dimensions plus a VerificationCouldn't run the suite — |
| if (eligible && extendedDimDef.primaryKey) { | ||
| primaryKeys.push(`${cubeName}.${dimensionName}`); |
There was a problem hiding this comment.
Including a non-public primary key produces a drill member that clients can't resolve in production.
The reasoning ("visibility governs the member picker, not what a drill query may reference") holds for the query, but not for the meta document the picker is built from. Gateway.filterVisibleItemsInMeta() strips non-visible dimensions from the /meta response outside dev mode / playground auth:
// packages/cubejs-api-gateway/src/gateway.ts:645
dimensions: cube.config.dimensions?.filter(visibilityFilter),So in production, orders.count.drillMembers[0] === 'orders.id' while orders.id is absent from the cube's dimensions. Meta.resolveMember() in cubejs-client-core then returns a not-found stub for it:
// packages/cubejs-client-core/src/Meta.ts:196
return { title: memberName, error: `Path not found '${memberName}'` };The drill query still runs (ResultSet.drillDown() reads drillMembers off the load-response annotation, which is unfiltered), but any UI that renders the drill member list from /meta gets a raw path plus an error object as the first entry of every auto-generated set. Today that only happens for models whose authors explicitly named a hidden PK; with this flag it becomes the default for every cube in every model.
Options: gate the PK on this.isVisible(extendedDimDef, false) too, put it last instead of first, or keep it and document the interaction with production meta filtering explicitly. Either way it's worth a test asserting the shape a production client actually sees.
There was a problem hiding this comment.
Checked it and the reasoning holds: filterVisibleItemsInMeta does strip the hidden PK from /v1/meta, and resolveMember returns the not-found stub for it, so a picker built from meta shows a broken first entry for every automatic set. The drill itself still works, since drillDown() reads the unfiltered load-response annotation.
Holding it rather than fixing it, though: the ticket is on Igor > Check and the verification artifact is a meta before/after that demonstrates the primary-key-first ordering by name. Every fix here (gate the PK on visibility, or move it last) changes exactly what he is about to verify, so it needs his call first. Written up in the planning doc with the three options and the evidence; it will land as a follow-up rather than get resolved silently under him.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #11488 +/- ##
===========================================
- Coverage 79.46% 59.14% -20.32%
===========================================
Files 480 223 -257
Lines 98810 17917 -80893
Branches 3636 3645 +9
===========================================
- Hits 78515 10597 -67918
+ Misses 19774 6798 -12976
- Partials 521 522 +1
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:
|
…the link/PK branches
|
Claude finished @igorlukanin's task in 2m 25s —— View job Re-review of
|
| # | Prior finding | Status |
|---|---|---|
| 2 | Shared array instance across measures | Fixed — autoDrillMembers.slice() at CubeToMetaTransformer.ts:520, plus a regression test that reverses one measure's list and asserts the other is unmoved |
| 4 | synthetic exclusion untested |
Fixed — city now declares a link, and both the cube-level and view-level cases assert the URL helper is excluded |
| 5 | Docs ordering + limit failure mode | Fixed — CUBEJS_AUTO_DRILL_MEMBERS* moved ahead of CUBEJS_AUTO_RUN_MODE, and "a value that isn't a number fails data model compilation" is now stated |
| 1 | Non-public primary key unresolvable in production meta | Still open — code unchanged; the doc comment at CubeToMetaTransformer.ts:450-453 now states the rationale explicitly, which was one of the three options I offered. Thread left unresolved for a human call; not re-posting. |
Verification of the new tests
I checked the three new assertions that could have been vacuous, and none are:
orders.city___link_city_page_url— the name matchesgenerateSyntheticLinkDimensions()'s`${dimName}___link_${linkName}_url`(CubeSymbols.ts:672) exactly, and the YAMLlinks: [{name, label, url}]shape is the same onetest/unit/links.test.tsexercises, so the synthetic dimension really is minted. The adjacent exact-equality test (['orders.id', 'orders.status', 'orders.city', 'orders.created_at']) would fail if it weren't being filtered.- The view link case —
CubeSymbols.ts:839-840readstypeof include === 'object' ? include.name : include, so the aliased{name: city, alias: renamed_city}include does trigger auto-inclusion of the helper into the view. The filter test is reached. - Compound PK / no-PK —
shipmentscovers both branches of theprimaryKeys/restsplit plus the cap counting PK parts;eventscovers the empty-primaryKeyspath. Both were genuinely uncovered before.
The mutation test (count.drillMembers.reverse()) is safe across tests because withEnv builds a fresh compiler per invocation.
New observations
Only one, filed inline: three near-identical env save/restore helpers (withEnv:103, withModel:267, withLegacyEnv:411) — the restore block is what prevents env leakage between tests, and it's now duplicated three times.
Two non-blocking asides, not filed:
defaultDrillMembers()runs for every cube atCubeToMetaTransformer.ts:240even when every measure on it declaresdrill_members. One wasted pass over the dimensions per such cube; negligible, and hoisting it would cost more clarity than it buys.asBoolStrict()meansCUBEJS_AUTO_DRILL_MEMBERS=1throws at compile time rather than reading as true. That's the established pattern for every other boolean inenv.ts, so it's consistent — worth knowing, not worth changing.
The subQuery?: boolean addition to DimensionDefinition is a correct type fix; it was already being read off dimension definitions elsewhere without being declared.
Verification limits
node_modules is absent in this checkout, so I could not run the suite, tsc, or eslint — everything above is static analysis against the source it calls into. The PR's own test-plan claims remain CI's to confirm.
|
Claude finished @igorlukanin's task in 57s —— View job Re-review of
|
| # | Prior finding | Status |
|---|---|---|
| 2 | Shared array instance across measures | Fixed in ce923d6, thread resolved |
| 3 | synthetic exclusion untested |
Fixed in ce923d6, thread resolved |
| 5 | Docs ordering + limit failure mode | Fixed in ce923d6, thread resolved |
| — | Duplicated env helpers | Fixed in 30bb1c6, thread already resolved and outdated |
| 1 | Non-public primary key unresolvable in production meta | Open — held deliberately: the verification artifact for the ticket is a meta before/after that demonstrates PK-first ordering by name, and every candidate fix changes exactly what's being verified. Documented in the code comment at CubeToMetaTransformer.ts:450-453 and slated as a follow-up. Not re-posting; the unresolved thread is the record. |
No threads needed resolving this round — the only one my re-review would have closed was already resolved by the author.
Verification limits
node_modules is absent in this checkout, so I could not run the suite, tsc, or eslint — the above is static analysis against the source and the repo's lint config. CI remains the gate on the PR's own test-plan claims.
Summary
drill_membersgets no drill-down affordance at all today, and nothing supplies one —drill_membersis measure-only, declared-only, and computed nowhere. This adds an opt-in default set computed at data-model compilation time, so the gap closes for every model rather than only for generated ones.CUBEJS_AUTO_DRILL_MEMBERS(defaultfalse). The default set is the cube's own dimensions — primary key first, then remaining public dimensions in definition order — capped byCUBEJS_AUTO_DRILL_MEMBERS_LIMIT(default10).syntheticlink helpers and sub-query dimensions are excluded.|| []"nothing declared" branch inCubeToMetaTransformer.measureConfig(), which is on the single path every cube and view flows through. It also gives declared-wins precedence for free, and per-view resolution falls out: a view'sdimensionskeys are its own member names, so a default computed from them names members the view actually exposes.drill_members: []stays a per-measure opt-out, and a view that includes none of a declared set keeps its (empty) declared result instead of picking up defaults.drill_membersname them routinely; visibility governs the member picker, not what a drill query may reference.CubeSymbols.generateIncludeMembers()was the other candidate site and was rejected: it runs only for views, and only for measures that already declaredrill_members, so it can never observe the empty case.Metadata only — nothing in
BaseQueryor the Rust planner readsdrill_members, and drill execution stays client-driven. The samemetaConfigfeeds load-response annotations, so every wired surface picks this up at once.Backward compatibility
With the flag off, emitted meta is byte-identical to today. The declared branch keeps its previous expression verbatim, quirks included — a reference written without an array literal still evaluates to a bare string, which this field has always passed through. Normalizing that would have changed meta for models that never enabled the flag, so it's deliberately left alone.
CUBEJS_AUTO_DRILL_MEMBERS_LIMITis parsed only after the flag is checked, so a malformed value cannot fail compilation for a deployment that never opted in.Open review thread
One finding is deliberately left open rather than fixed: the primary key leads the automatic set, but
Gateway.filterVisibleItemsInMeta()strips a hidden PK from/v1/meta, so a member picker built from meta resolves it to a not-found stub. The drill query itself is unaffected —ResultSet.drillDown()reads the unfiltered load-response annotation.The ticket is on
Igor > Checkand the verification artifact demonstrates the primary-key-first ordering by name, so every available fix (gate the PK on visibility, or move it last) would change exactly what is being verified. Options and evidence are in the planning doc; it lands as a follow-up rather than being resolved silently.Test plan
test/unit/auto-drill-members.test.ts: flag off/on, declared-wins including the[]opt-out, the cap (and a zero limit), primary-key-first ordering, compound and absent primary keys, non-public /synthetic/ sub-query exclusion on both a cube and through a view, per-measure isolation of the computed array, view resolution through an aliased include, the legacydrillMemberReferenceskey, the bare-string declaration shape, and a malformed limit while the flag is offviews.test.tssnapshots pass with--ciand zero updates, confirming flag-off byte-identitytsc --noEmitand eslint clean on the changed filesschema-compilerunit suite green locally: 36 suites / 706 tests / 110 snapshots, zero snapshot updates