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 59.35% 59.38% +0.03%
==========================================
Files 223 223
Lines 17989 18015 +26
Branches 3641 3650 +9
==========================================
+ Hits 10677 10699 +22
- Misses 6793 6796 +3
- Partials 519 520 +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:
|
|
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.
30bb1c6 to
7bd5be4
Compare
|
Claude finished @igorlukanin's task in 2m 11s —— View job Re-review of
|
| Before | Now | Message |
|---|---|---|
e05d8c2-equivalent |
e05d8c2 |
feat: compute a default measure drill-member set behind an env flag |
ce923d6 |
366f320 |
fix: copy the computed drill set per measure, cover the link/PK branches |
30bb1c6 |
7bd5be4 |
test: fold the three env save/restore helpers into one |
git diff origin/master...HEAD is still 615/6 across the same 6 files, and re-reading the whole diff line by line turns up nothing that differs from what the previous two rounds covered: .slice() is at CubeToMetaTransformer.ts:520, the synthetic / compound-PK / no-PK tests are present, the docs sections sit above CUBEJS_AUTO_RUN_MODE with the limit-parse failure documented, and withCompiled is the single env save/restore block at test line 106 with withEnv / withModel / withLegacyEnv as one-line partial applications. This is a rebase, not a code change.
(The previously reviewed 30bb1c6 is no longer reachable in this checkout and I couldn't fetch it, so the equivalence above is established by re-reading the current diff in full against the two prior reviews rather than by a commit-to-commit diff.)
Spot-checks on the current state
defaultDrillMembers()is invoked once per cube at line 240 and threaded intomeasureConfig()as a parameter — views take the sametransform()path, so per-view resolution still holds.isVisible(extendedDimDef, true)at line 483 is the non-PK default; the PK is admitted by the preceding branch. That matches the emitted-dimension rule at line 305 (this.isVisible(extendedDimDef, !extendedDimDef.primaryKey)) for every case except theisCubeVisible === falsecube, where the cube's measures are non-public too so nothing surfaces. Unchanged, and still only an implicit dependency rather than a defect.drillMembersGrouped(line 525) classifies the computed set throughisMeasure/isDimension, so the auto set lands entirely underdimensions— asserted by thepopulates drillMembersGroupedtest.- The
measure filterstest assertsbig_ordersgets the same computed set ascount; the filtered-measure path is genuinely orthogonal to drill-member derivation, so that's a correct expectation rather than a vacuous one.
Prior findings
| # | Prior finding | Status |
|---|---|---|
| 2 | Shared array instance across measures | Fixed (366f320), thread resolved |
| 3 | synthetic exclusion untested |
Fixed (366f320), thread resolved |
| 5 | Docs ordering + limit failure mode | Fixed (366f320), thread resolved |
| — | Duplicated env helpers | Fixed (7bd5be4), thread resolved and outdated |
| 1 | Non-public primary key unresolvable in production meta | Open — still applicable at CubeToMetaTransformer.ts:482, held deliberately: the ticket's verification artifact demonstrates PK-first ordering by name, and every candidate fix changes exactly that. Re-affirmed, not re-posted; the unresolved thread is the record. |
No threads needed resolving this round — the only unresolved claude thread is the one still applicable, and no human threads were touched.
Verification limits
node_modules is absent in this checkout (no node_modules/jest), so I could not run the suite, tsc, or eslint. Everything above is static analysis against the source. The PR's own test-plan claims — 28 unit tests, views.test.ts snapshots clean, full schema-compiler suite green — remain CI's to confirm, and the rebase means they should be re-confirmed against the new base rather than carried over.
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