Skip to content

feat(schema-compiler): compute a default measure drill-member set behind an env flag - #11488

Open
igorlukanin wants to merge 3 commits into
masterfrom
igor/core-740-auto-drill-down-core-computes-a-default-measure-drill-set-at
Open

feat(schema-compiler): compute a default measure drill-member set behind an env flag#11488
igorlukanin wants to merge 3 commits into
masterfrom
igor/core-740-auto-drill-down-core-computes-a-default-measure-drill-set-at

Conversation

@igorlukanin

@igorlukanin igorlukanin commented Aug 5, 2026

Copy link
Copy Markdown
Member

Summary

  • A measure with no drill_members gets no drill-down affordance at all today, and nothing supplies one — drill_members is 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.
  • Behind CUBEJS_AUTO_DRILL_MEMBERS (default false). The default set is the cube's own dimensions — primary key first, then remaining public dimensions in definition order — capped by CUBEJS_AUTO_DRILL_MEMBERS_LIMIT (default 10). synthetic link helpers and sub-query dimensions are excluded.
  • The hook is the || [] "nothing declared" branch in CubeToMetaTransformer.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's dimensions keys are its own member names, so a default computed from them names members the view actually exposes.
  • Declared config always wins, keyed on declaration presence rather than evaluated lengthdrill_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.
  • The primary key is included regardless of visibility. Primary keys are hidden by default, and hand-written drill_members name 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 declare drill_members, so it can never observe the empty case.

Metadata only — nothing in BaseQuery or the Rust planner reads drill_members, and drill execution stays client-driven. The same metaConfig feeds 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_LIMIT is 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 > Check and 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

  • 28 unit tests in 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 legacy drillMemberReferences key, the bare-string declaration shape, and a malformed limit while the flag is off
  • views.test.ts snapshots pass with --ci and zero updates, confirming flag-off byte-identity
  • tsc --noEmit and eslint clean on the changed files
  • Full schema-compiler unit suite green locally: 36 suites / 706 tests / 110 snapshots, zero snapshot updates
  • CI must pass

@igorlukanin
igorlukanin requested review from a team and keydunov as code owners August 5, 2026 19:16
@mintlify

mintlify Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Preview deployment for your docs. Learn more about Mintlify Previews.

Project Status Preview Updated (UTC)
cubed3 🟢 Ready View Preview Aug 5, 2026, 7:17 PM

💡 Tip: Enable Workflows to automatically generate PRs for you.

@github-actions github-actions Bot added the javascript Pull requests that update Javascript code label Aug 5, 2026
@claude

claude Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

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
  • Gather context / read diff
  • Review env.ts + CubeEvaluator.ts
  • Review CubeToMetaTransformer.ts
  • Review tests
  • Review docs
  • Check existing review threads (none), post findings

What's good

The hook site is well chosen. measureConfig()'s || [] branch is genuinely the single path every cube and view flows through, keying on declaration presence rather than evaluated length is the correct call (it preserves drill_members: [] as an opt-out and stops a view from silently acquiring defaults when a declared set resolves to nothing), and per-view resolution really does fall out for free because a view's dimensions keys are its own member names. Checking the flag before parsing the limit is the right ordering. The rejection of CubeSymbols.generateIncludeMembers() as an alternative site is correct — line 1085 there is gated on resolvedMember.drillMembers being present, so it can never see the empty case. Comments explain why rather than what, which is what this file needed.

Findings

# Severity Where Issue
1 Medium-high CubeToMetaTransformer.ts:481 Non-public primary key becomes an unresolvable drill member in production meta
2 Medium CubeToMetaTransformer.ts:515 Single array instance shared by every undeclared measure on the cube
3 Low CubeToMetaTransformer.ts:521 Bare-string quirk makes drillMembersGrouped iterate characters (pre-existing)
4 Low auto-drill-members.test.ts synthetic exclusion is claimed as covered but has no test
5 Low environment-variables.mdx:48 Out of alphabetical order; limit-parse failure mode undocumented

1 is the one worth deciding before merge. Gateway.filterVisibleItemsInMeta() strips non-visible dimensions from /meta outside dev mode, so orders.id is named by drillMembers while being absent from the cube's dimensions; Meta.resolveMember() then returns { title, error: "Path not found" }. The drill query itself still executes — ResultSet.drillDown() reads the unfiltered load-response annotation — so this is a picker-rendering problem, not a query failure. But it hits the first entry of every auto-generated set, and it changes from "models that opted in by hand" to "the default everywhere."

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:

  • isSubQuery() resolves aliasMember exactly one hop. That's sufficient for cube-in-view, which is what the tests cover; if a view can ever include another view, a sub-query dimension two hops down would slip through. Worth a comment noting the single-hop assumption either way.
  • defaultDrillMembers() doesn't consider isCubeVisible. Harmless today (a non-public cube's measures are non-public too, so nothing surfaces), but it's the kind of implicit dependency that's cheap to state in the comment.

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 dimensionByPath per aliased dimension, both negligible against the surrounding transform.

Verification

Couldn't run the suite — node_modules is absent in this checkout and installing the monorepo's dependencies wasn't in scope for the review. The PR's own test-plan claims (22 tests, views.test.ts snapshots clean, tsc --noEmit, eslint) are unverified here beyond reading; the synthetic claim is the one I could check statically, and it doesn't hold (finding 4). CI is the gate.

• branch `igor/core-740-auto-drill-down-core-computes-a-default-measure-drill-set-at`

Comment on lines +481 to +482
if (eligible && extendedDimDef.primaryKey) {
primaryKeys.push(`${cubeName}.${dimensionName}`);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Fix this →

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread docs-mintlify/reference/configuration/environment-variables.mdx
@codecov

codecov Bot commented Aug 5, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 81.48148% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 59.14%. Comparing base (f7ff67a) to head (30bb1c6).

Files with missing lines Patch % Lines
...ema-compiler/src/compiler/CubeToMetaTransformer.ts 88.00% 1 Missing and 2 partials ⚠️
packages/cubejs-backend-shared/src/env.ts 0.00% 2 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (f7ff67a) and HEAD (30bb1c6). Click for more details.

HEAD has 1 upload less than BASE
Flag BASE (f7ff67a) HEAD (30bb1c6)
cubesql 1 0
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     
Flag Coverage Δ
cube-backend 59.14% <81.48%> (+0.03%) ⬆️
cubesql ?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@claude

claude Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Claude finished @igorlukanin's task in 2m 25s —— View job


Re-review of ce923d6: 3 of 4 prior findings addressed, 1 open by design decision, 1 new nit. 1 inline comment posted.

Full review
  • Read incremental diff (ce923d6)
  • Verify the .slice() fix
  • Verify the new tests aren't vacuous
  • Verify the docs reordering
  • Re-check prior review threads, dedupe

Status of prior findings

# Prior finding Status
2 Shared array instance across measures FixedautoDrillMembers.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 Fixedcity 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 FixedCUBEJS_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 matches generateSyntheticLinkDimensions()'s `${dimName}___link_${linkName}_url` (CubeSymbols.ts:672) exactly, and the YAML links: [{name, label, url}] shape is the same one test/unit/links.test.ts exercises, 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 caseCubeSymbols.ts:839-840 reads typeof 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-PKshipments covers both branches of the primaryKeys/rest split plus the cap counting PK parts; events covers the empty-primaryKeys path. 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 at CubeToMetaTransformer.ts:240 even when every measure on it declares drill_members. One wasted pass over the dimensions per such cube; negligible, and hoisting it would cost more clarity than it buys.
  • asBoolStrict() means CUBEJS_AUTO_DRILL_MEMBERS=1 throws at compile time rather than reading as true. That's the established pattern for every other boolean in env.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.

• branch `igor/core-740-auto-drill-down-core-computes-a-default-measure-drill-set-at`

Comment thread packages/cubejs-schema-compiler/test/unit/auto-drill-members.test.ts Outdated
@claude

claude Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Claude finished @igorlukanin's task in 57s —— View job


Re-review of 30bb1c6: test-only refactor, clean. No new findings; 1 prior thread open by the author's decision.

Full review
  • Read incremental diff (ce923d6..30bb1c6)
  • Verify the helper fold preserves save/restore semantics
  • Re-check prior review threads, resolve stale ones

What changed

Only test/unit/auto-drill-members.test.ts. The three duplicated env save/set/restore blocks (withEnv, withModel, withLegacyEnv) are folded into a single withCompiled(prepare, env, fn) at line 106, with the three former helpers reduced to one-line partial applications. This is the shape I suggested in the (now resolved, outdated) nit thread.

Verification

Four things could have broken in a fold like this; none did:

  • Every call site still awaits. All 26 withEnv/withModel/withLegacyEnv calls are awaited. The helpers dropped their own async keyword but return withCompiled(...)'s promise, so the awaited value is unchanged.
  • withModel's callback is now awaited. The old copy called fn(metaTransformer) bare; withCompiled does await fn(...). Its callbacks are all synchronous, so this is a no-op today and strictly safer if one ever returns a promise.
  • Restore ordering is intact. The finally block still iterates Object.keys(env) and distinguishes undefined (delete) from a prior value (reassign) — the part that actually prevents cross-test leakage — and it now exists once instead of three times.
  • Compile-after-env is preserved. The prepare() thunk is invoked inside the try, after the env writes, which is the property the comment at line 116 documents (meta is computed during compile() and cached on the instance). Passing a thunk rather than a prepared instance is what keeps that ordering enforceable.

Two smaller notes, both fine:

  • The prepare parameter is typed () => { compiler: any; metaTransformer: any }; both prepareYamlCompiler and prepareJsCompiler return supersets of that, so they're structurally assignable.
  • The three new one-liners exceed 120 chars, but max-len is 0 in packages/cubejs-linter/index.js:37, so eslint won't flag them.

Prior findings

# 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.

• branch `igor/core-740-auto-drill-down-core-computes-a-default-measure-drill-set-at`

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

javascript Pull requests that update Javascript code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant