feat(api-gateway): granularities config and /v1/granularities endpoint - #10918
feat(api-gateway): granularities config and /v1/granularities endpoint#10918igorlukanin wants to merge 22 commits into
Conversation
02bf1d7 to
b7ac787
Compare
b7ac787 to
898dec8
Compare
898dec8 to
19ef48a
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #10918 +/- ##
==========================================
- Coverage 84.06% 79.58% -4.49%
==========================================
Files 257 486 +229
Lines 81582 100027 +18445
Branches 0 3755 +3755
==========================================
+ Hits 68581 79605 +11024
- Misses 13001 19863 +6862
- Partials 0 559 +559
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:
|
b438640 to
d0745d7
Compare
|
Claude finished @igorlukanin's task in 4m 28s —— View job Code Review — 1 high, 2 mediumReview details, checklist, and findings
Overall this is a clean, well-scoped change. The two-shape validation, the lazy per-dim resolver in the load path, and the "block is internal; strip before responding" pattern in Findings
Minor / nits (not filed inline)
|
|
Claude finished @igorlukanin's task in 1m 59s —— View job Re-review after 5d91cf7 / d0745d7 — 1 medium remainingDetails
FindingsMedium — Suggested fix in function transformDimension(dimension: any, cubeDefinitions: any) {
const { cubeName, memberName } = getMemberPath(dimension.name);
const { granularitiesBlock, ...rest } = dimension;
return {
...rest,
sql: stringifyMemberSql(cubeDefinitions[cubeName]?.dimensions?.[memberName]?.sql),
case: handleDimensionCaseCondition(cubeDefinitions[cubeName]?.dimensions?.[memberName]?.case),
};
}Separately (and out of scope for this PR if you'd rather punt it): Notes on the new architecture (d0745d7)
|
|
Claude finished @igorlukanin's task in 54s —— View job Re-review after db2126e — no substantive changeDetails
db2126e is a pure lint fix in Still openMedium — Fix as previously suggested: function transformDimension(dimension: any, cubeDefinitions: any) {
const { cubeName, memberName } = getMemberPath(dimension.name);
const { granularitiesBlock, ...rest } = dimension;
return {
...rest,
sql: stringifyMemberSql(cubeDefinitions[cubeName]?.dimensions?.[memberName]?.sql),
case: handleDimensionCaseCondition(cubeDefinitions[cubeName]?.dimensions?.[memberName]?.case),
};
}• • Branch |
|
Claude finished @igorlukanin's task in 1m 0s —— View job Re-review after da17307 — 1 medium still openDetails
da17307 makes Still openMedium — I attempted to file this inline, but function transformDimension(dimension: any, cubeDefinitions: any) {
const { cubeName, memberName } = getMemberPath(dimension.name);
const { granularitiesBlock, ...rest } = dimension;
return {
...rest,
sql: stringifyMemberSql(cubeDefinitions[cubeName]?.dimensions?.[memberName]?.sql),
case: handleDimensionCaseCondition(cubeDefinitions[cubeName]?.dimensions?.[memberName]?.case),
};
}Separately (out of scope if you'd rather punt): Notes
• • Branch |
|
Claude finished @igorlukanin's task in 3m 41s —— View job Re-review after 314311d — no blocking findings314311d is a substantial refactor: Details, checklist, prior findings
Prior open finding — resolved by this commit
New code — notesVariant cache correctness.
Hash canonical form.
Bug fix in pre-agg job path. Minor / nits (not filing inline)
Rate-limiting on
|
|
Reworked where effective granularities are resolved: moved from per-request gateway enrichment into the compiler layer.
|
|
Claude finished @igorlukanin's task in 40s —— View job Re-review after 56b8b82 — no blocking findings56b8b82 is a pure tuning change: Details
Sizing noteThe new bound (64) sits under CubeSQL's LRU-100 compiler cache, which the comment now calls out explicitly — reasonable ceiling given each entry is roughly a full enriched meta copy (~2 MB on a 3k-cube model per the commit rationale ⇒ ~128 MB worst-case for this cache alone). If a real deployment's Still worth confirming (unchanged from prior notes)
Nothing here needs a code change. • • Branch • |
904ee5f to
a7a7604
Compare
|
Claude finished @igorlukanin's task in 41s —— View job Re-review after
|
| # | Severity | File | Concern |
|---|---|---|---|
| 1 | High | packages/cubejs-schema-compiler/src/compiler/CubeSymbols.ts:673 |
Global-custom merge runs before CubeValidator, so a config-level custom that resolveFromList accepts ({ name: 'retail_period', interval: '5 days' }, or a name failing identifierRegex) is validated through the stricter per-dimension CustomGranularityEntrySchema and fails compilation on every plain time dimension. Repro confirmed by you; the choice between config-time validation and moving the merge past validation is recorded in the planning doc. |
| 2 | Medium | packages/cubejs-schema-compiler/src/compiler/CubeToMetaTransformer.ts:346 |
A time dim declaring no granularities has no granularitiesBlock, so the deprecated granularities meta field falls through to the merged map and now advertises global customs — a wire-shape change on the deprecated field, and those dims lose the shared defaultSet fast path. Confirmed. |
| 3 | Medium | packages/cubejs-schema-compiler/src/compiler/CubeSymbols.ts:1666 |
The unconditional global fallback in resolveGranularity undoes the per-dimension includes/excludes filtering the merge performs — an excluded global still resolves in SQL. Product call on display-only vs. enforced; test to be pinned once settled. |
Nothing new in this push. The prior review stands verbatim for a7a7604, and the three threads above are left unresolved since the underlying decisions are still pending.
a7a7604 to
da0d7cb
Compare
|
Claude finished @igorlukanin's task in 35s —— View job Re-review after
|
| # | Severity | File | Concern |
|---|---|---|---|
| 1 | High | packages/cubejs-schema-compiler/src/compiler/CubeSymbols.ts:673 |
Global-custom merge runs before CubeValidator, so a config-level custom that resolveFromList accepts ({ name: 'retail_period', interval: '5 days' }, or a name failing identifierRegex) is validated through the stricter per-dimension CustomGranularityEntrySchema and fails compilation on every plain time dimension. Repro confirmed by you; the choice between config-time validation and moving the merge past validation is recorded in the planning doc. |
| 2 | Medium | packages/cubejs-schema-compiler/src/compiler/CubeToMetaTransformer.ts:346 |
A time dim declaring no granularities has no granularitiesBlock, so the deprecated granularities meta field falls through to the merged map and now advertises global customs — a wire-shape change on the deprecated field, and those dims lose the shared defaultSet fast path. Confirmed. |
| 3 | Medium | packages/cubejs-schema-compiler/src/compiler/CubeSymbols.ts:1663 |
The unconditional global fallback in resolveGranularity undoes the per-dimension includes/excludes filtering the merge performs — an excluded global still resolves in SQL. Product call on display-only vs. enforced; test to be pinned once settled. |
Nothing new in this push. The prior review stands verbatim for da0d7cb, and the three threads above are left unresolved since the underlying decisions are still pending.
…larities, resolve on demand
…rd to granularities.custom
… snapshots and stale assertion
…ler with a bounded per-config variant cache
…binatorial calendar configs
…via CUBEJS_MAX_GRANULARITY_VARIANTS
…, keep fixed bound of 64, tighten comments
…; harden resolution (proto keys, built-in interval, view block, validator, config validation, hash)
…and SQL paths; drop empty-interval env customs; non-array function return uses default catalog
…aritiesFor + GRANULARITY_STRING_FIELDS constant
…ig-hash on the compiled model
…ut a local block (share one global-custom map by reference)
…, attach onto base cubes at read time
…fs map; memoize proxy bound methods; dedup definitions cache; hash only output-affecting built-in fields; fix custom-named/proto-name bugs; skip compilerId when unneeded
…ile time, bake into compiled model (CUB-2567)
… CompilerApi, tighten comments (CUB-2567 review)
…ls instead of a bespoke CompilerApi compile flow
da0d7cb to
dba5209
Compare
Summary
Engine-side foundation for configurable, deployment-wide granularities:
granularitiesshape on time dimensions. Accepts a dict{ includes?, excludes?, custom? }alongside the legacy flat array.includes/excludes(string list or'*') select from the enabled set;customdefines per-dimension granularities. Mutually exclusive except forincludes: '*' + excludes: [...].CUBEJS_GRANULARITIES(comma-separated list), per-custom env varsCUBEJS_GRANULARITIES_<NAME>_INTERVAL|TITLE|OFFSET|ORIGIN, orconfig.granularities(list or context-dependent function). File config fully replaces env vars.GET /v1/granularities. New read-only endpoint returning the resolved set (built-ins + customs) for the request's security context./v1/metaenrichment. Each time-dimension granularity now carriestype('built-in'/'custom'),title,format(d3-time-format), andinterval. Per-dimensionincludes/excludesare resolved against the deployment config so the response reflects the effective set./v1/load+/v1/cubesqlannotations. Sametype/formatenrichment flows throughprepareAnnotationto query responses.Granularity/GranularityAnnotationand the Rust transport struct in@cubejs-backend/nativenow carrytypeandformat.Out of scope (separate follow-ups):
allow_no_granularityparameter end-to-endTest plan
granularities-shape.test.ts) and global config precedence (granularities-config.test.ts).cube-validator.test.tsfor the new dict shape: includes/excludes mutual-exclusion, built-in shadowing, legacy form acceptance.prepare-annotation.test.tsfor the newtype/formatfields on the granularity annotation./v1/granularitiesreturns 8 built-ins + customs sourced fromCUBEJS_GRANULARITIES./v1/metafor a time dim withgranularities: { includes: ['year', 'quarter'], custom: [...] }returns exactly those built-ins plus the local custom; a dim with the legacy flat array gets all enabled built-ins plus its locals./v1/loadwithgranularity: 'week'returns{ type: 'built-in', title: 'Week', interval: '1 week', format: '%b %-d, %Y' }.