docs: rewrite the custom calendar recipe onto calendar cubes - #11750
docs: rewrite the custom calendar recipe onto calendar cubes#11750igorlukanin wants to merge 6 commits into
Conversation
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Automations to automatically generate PRs for you. |
|
Claude finished @igorlukanin's task in 2m 39s —— View job Approve with 3 non-blocking comments — 0 high, 2 medium, 1 lowFull review
VerificationI checked the three claims in the PR description against the source rather than taking them at face value:
Also verified: the What's goodThe rewrite is a clear improvement. Deriving the model from the fixture the Rust planner tests cover is the right call, and it shows — the Findings
Details are in the three inline comments. None block merge; #2 is pre-existing but the PR is already editing those lines and adding the callout that makes it read as wrong. Minor, not worth an inline commentThe Pre-aggregations section is prose-only. Given the section's point is "declare the overridden granularity explicitly", a four-line |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Claude finished @igorlukanin's task in 1m 37s —— View job Approve — incremental push is clean; 3 prior threads still open (2 medium, 1 low)Full review of the new commit
What changed since the last reviewOnly 1. Verified: the file parses, the entry appears exactly once (no duplicate under another group), 2. -an inherited value is not passed to the query engine. Without it, the extending cube is
-not treated as a calendar cube and its granularity overrides are ignored.
+inherited cube-level parameters are not always passed to the query engine. Setting it
+again keeps the extending cube a calendar cube.Still accurate. The mechanism is unchanged — The hedge does cost the reader the why: "not always" leaves them unable to predict which other cube-level parameters behave this way, where the previous sentence told them exactly what breaks. Both versions get them to the right code, so this is a judgement call and yours to make — not raising it as a finding. Prior threadsAll three are still unresolved and still apply — the new commit touches none of the relevant lines. Re-affirmed rather than re-posted, so no duplicate inline comments:
None block merge. The second is the one I'd most want closed in this PR, since the page now carries a callout that makes its own example read as wrong. |
|
Claude finished @igorlukanin's task in 2m 39s —— View job Approve — all 3 prior threads addressed and resolved; 1 new low noteFull review of `29f9ee4`
Prior threads — all closed
All three resolved. New claims, verified
The new The reworded
So the earlier, stronger version of this warning was wrong in the specific it named, and the new one is both more precise and more useful — it names the symptom a reader would actually see. The New finding
Every other snippet on the page traces to a fixture the Rust planner covers — that is the PR's stated method and it holds up. The Pre-aggregations section is the exception: there is no test under Minor, not worth an inline commentThe second |
|
Actioned the three review threads, and the middle one grew: the calendar cubes concept page had two examples that could not run at all. The granularity example declares two primary keys, which compiles silently and then fails every query against the cube; the time-shifts example was missing I compiled every model snippet on both pages and generated SQL for each — all 10 blocks pass now, and the recipe reproduces its own documented 3 / 5 / 4 result table. One thing I left alone: in the time-shifts example the shifted CTE groups by the shifted column, so |
|
Claude finished @igorlukanin's task in 3m 24s —— View job Request changes on the new commit — 1 medium (the pre-aggregation warning may be inverted); prior 3 threads all resolvedFull review of `0ef1eee`
What changedOnly This is a direct response to the low-severity note I left last round — that the Pre-aggregations section was the one part of the page not backed by a planner test. Turning an unverified positive claim into an unverified negative one doesn't settle it, though, and the negative version is the more costly one to get wrong: it tells readers to give up pre-aggregations for their main reporting grain. Why I think it may be invertedTraced both halves of the path; the override survives both as far as I can tell:
The case that does break silently is the one the removed paragraph covered: a query matched to a rollup at a different grain via I could not run the compiler to settle this — this checkout has no Prior threadsAll three from earlier rounds are now resolved by the author, and the fixes hold up in the current tree. Nothing re-raised. Minor
|
|
Good catch, and it was worse than "unverified". I built the section's own model with a rollup declaring CREATE TABLE stb_pre_aggregations.orders_orders_by_retail_month AS
SELECT date_trunc('month', ("retail_calendar".date_value::timestamptz AT TIME ZONE 'UTC')) "retail_calendar__date_month", ...
The control that makes it conclusive: on the same model without the pre-aggregation, Tesseract does emit That falsifies both claims the section made — the "declare it explicitly" advice does not protect the reader, so I replaced the section with a warning not to pre-aggregate these groupings at all. Also corrected an earlier ticket of mine that asserted the declared-granularity case worked; it doesn't, and that makes the defect wider than filed. |
|
Claude finished @igorlukanin's task in 1m 56s —— View job Approve — the correction is right; 2 low notes, all prior threads resolvedFull review of `441c1a6`
The correctionOnly Your re-run settles it, and it matches the trace I posted last round rather than contradicting it — the earlier The hierarchy finding is a good addition and worth keeping in the warning: your One thing the warning leaves implicit that might be worth a clause: the reader can do everything right and still get hit, because a pre-existing Prior threadsAll four now resolved. I resolved my own thread on the previous pre-aggregation wording, since Findings
Both are inline. Neither blocks merge. · |
|
Claude finished @igorlukanin's task in 1m 18s —— View job Approve — all threads now resolved; no new findings, 1 whitespace nitFull review of `d58a409`
Prior threads — both closed
Both resolved. The new JavaScript snippet, checked
The anchor the recipe links to, Nit
|
2 participants
The custom calendar recipe was written before calendar cubes existed: it modelled a 4-5-4 retail calendar with a join plus proxy dimensions, and its
retail_weekwas atype: numberdimension rather than a granularity. That is the workaround, not the feature. This rewrites the page onto calendar cubes and keeps the URL.The model is derived from the fixture the Rust planner tests cover, so every snippet reflects tested behaviour: a
calendar: truecube whoseprimary_keydimension overrides themonth,week, andyeargranularities with pre-calculated columns, plus prior-month and prior-year time shifts. The page also states why a 4-5-4 month needssql:rather thanintervaland links to the naming rule instead of restating it.Both the YAML and the JavaScript version of the full model were compiled against the schema compiler before publishing. That turned up three things worth calling out:
granularitiesform was wrong in the existing docs. It must be an object keyed by granularity name; the array form does not compile. The calendar cubes page had it as an array in two places, so those are fixed here too.sql: '{CUBE}.col'in JavaScript is a plain string. Only a template literal is transpiled to receiveCUBE, so the single-quoted form on the calendar cubes page silently produced a literal instead of a column reference. Also fixed.extendsdoes not carrycalendar: trueto the query engine. The child reads it through the prototype chain, but it is not an own property, so it is not serialised and the extending cube is not treated as a calendar cube — its granularity overrides are quietly ignored. The recipe now showscalendar: truerepeated on the extending cube, with a warning explaining why.The measure snippets are now shown on the cube with facts. Previously they were bare fragments, and a reader who put them on the calendar cube would have counted dates instead of orders.
Also updated the recipe index card, which still described the proxy-dimension approach.
Verified with
mint broken-links --check-anchors(clean; the one reported break is pre-existing and unrelated) and by rendering both changed pages locally.