fix(schema-compiler): only build pre-agg partitions needed for bounded rolling-window queries

[igorlukanin]

Issue

CORE-164 — a query with a narrow dateRange (e.g. yesterday) against a week-partitioned pre-aggregation builds all partitions instead of only the one(s) needed to serve the timeframe.

Root cause

preAggregationDescriptionFor gated computing matchedTimeDimensionDateRange on !this.hasCumulativeMeasures(). So whenever the query has a cumulative (rolling window) measure, matchedTimeDimensionDateRange was left undefined, and the orchestrator's PreAggregationPartitionRangeLoader.partitionRanges() → intersectDateRanges(buildRange, undefined) falls back to the whole build range — every partition.

That blanket skip is only correct for unbounded trailing windows (which genuinely need all history). For a bounded window (trailing: 7 day) only [queryStart − window, queryEnd] is needed.

Evidence (local repro)

Week-partitioned pre-agg, dateRange = a single day, ~4.5 years of build range:

Query	matchedTimeDimensionDateRange	Partitions built
additive measure (with/without granularity, incl. via a view)	set to the requested day	1 ✅
rollingWindow.trailing: 7 day	undefined	235 ❌
rollingWindow.trailing: unbounded	undefined (correct)	235 (correct)

A correct 7-day-trailing build for a single day needs 2 weekly partitions, not 235.

Fix

cumulativeMeasuresTrailingInterval() classifies the query's cumulative leaf measures:

• no cumulative measures → regular narrowing (unchanged);
• all bounded trailing, no leading → narrow, and shift the matched range start back by the largest trailing window so the lookback partitions are still built;
• any unbounded trailing / any leading / non-fixed window → don't narrow, keep building the whole range (previous behaviour preserved).

The usage-info merge path (Tesseract) now takes the union of the per-usage range and the base matched range, so neither a time_shift usage's earlier partitions nor a rolling window's trailing lookback is dropped.

Tesseract has no partition-narrowing logic of its own — it routes pre-agg description assembly through this same JS method — so the fix covers both the default (Tesseract) and legacy planners.

Tests

New core-164-rolling-window-partitions.test.ts:

• bounded trailing window → matchedTimeDimensionDateRange set to the requested range expanded back by the window (was undefined before — red without the fix);
• unbounded trailing window → still undefined (whole range), guarding the correct case.

Existing pre-aggregations, pre-agg-time-dim-match, pre-agg-by-filter-match suites: no new failures (the pre-existing failures in this checkout are native-binary/viewGroups drift, unrelated and identical on master).

[claude]

Claude finished @igorlukanin's task in 1m 13s —— View job

---

Code review

Details

• Read PR diff and both changed files
• Analyze correctness of narrowing logic and unionDateRanges
• Check for edge cases and comment on issues

Overall the fix is well-scoped and the reasoning in the PR description matches the code: cumulativeMeasuresTrailingInterval() correctly refuses to narrow when a leading window is present, when trailing is unbounded, or when the window type isn't fixed. The test coverage picks the right two guarding cases (bounded vs unbounded), and returning the trailing interval as a value (rather than a boolean) keeps the caller cleanly responsible for the actual shift.

A few observations worth considering — none blocking:

1. unionDateRanges relies on lexicographic string comparison (PreAggregations.ts:235-237). This is only correct when both a and b are already normalized ISO strings with identical precision/timezone. desc.matchedTimeDimensionDateRange comes from boundaryDateRangeFormatted()/formattedDateRange() (format YYYY-MM-DDTHH:mm:ss.SSS[…]) and mergedDateRange comes from usageInfo.usages[*].dateRange. If the two paths ever disagree on precision or produce a Z suffix on one side, '2024-06-10T00:00:00.000' < '2024-06-10T00:00:00.000Z' would silently pick the wrong endpoint. Worth either normalizing via moment(...).format(format) first, or adding a brief comment asserting the invariant.

2. Degenerate rolling window with no trailing and no leading — cumulativeMeasuresTrailingInterval falls through and returns maxTrailing as undefined, which the caller interprets as "no cumulative measures → narrow normally, no shift". That likely isn't reachable (schema validation should require one), but if it ever is, the narrowing would silently drop data. Cheap defense: initialize maxTrailing to a zero-length ParsedInterval so canMatchTimeDimension still narrows but the range is unshifted, or return null when the window has neither bound (safer default).

3. cumulativeMeasuresTrailingInterval() re-walks the leaf-measure graph on every preAggregationDescriptionFor call (once per pre-agg). hasCumulativeMeasures() right above it is memoized in hasCumulativeMeasuresValue for the same reason — worth caching this too, especially for queries touching many rollupJoin/rollupLambda references.

4. Comparing intervals by moment.duration(parsed).asSeconds() (PreAggregations.ts:354) is fine in practice but uses moment's approximate month/year length. If someone mixes 1 month and 30 day windows on the same query, the "largest" pick is essentially arbitrary between them — acceptable for lookback shift, just something to be aware of.

5. Unbounded-check flow (PreAggregations.ts:346-351) — the leading === 'unbounded' branch is unreachable because the preceding if (rollingWindow.leading && rollingWindow.leading !== 'unbounded') already short-circuits every non-unbounded truthy leading. The three ifs could collapse to two without changing behaviour.

6. Test coverage is tight and exactly targets the regression, which is great. One additional case that would harden it: a measure with rollingWindow.trailing: 1 month, offset: end (bounded but with offset) — worth confirming offset doesn't change what needs to be present in partitions. Also, an integration-style assertion that the actual number of built partitions matches the expected (2, per the PR table) would guard against future regressions at the orchestrator boundary; but that's out of scope for a unit test and probably belongs in cubejs-testing.

Nothing here changes the correctness verdict — the fix looks right. LGTM modulo the union-comparison note.

• [`igor/core-164-should-only-the-pre-aggregation-partitions-that-is-needed-to`](https://github.com/cube-js/cube/tree/igor/core-164-should-only-the-pre-aggregation-partitions-that-is-needed-to)

[codecov]

Codecov Report

❌ Patch coverage is 95.00000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 59.03%. Comparing base (26c4801) to head (6ad6f71).
⚠️ Report is 7 commits behind head on master.

Files with missing lines	Patch %	Lines
...ejs-schema-compiler/src/adapter/PreAggregations.ts	95.00%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #11277      +/-   ##
==========================================
+ Coverage   59.00%   59.03%   +0.03%     
==========================================
  Files         223      223              
  Lines       17891    17925      +34     
  Branches     3636     3643       +7     
==========================================
+ Hits        10556    10582      +26     
- Misses       6817     6824       +7     
- Partials      518      519       +1     

Flag	Coverage Δ
cube-backend	59.03% <95.00%> (+0.03%)	⬆️

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 finished @igorlukanin's task in 1m 23s —— View job

---

Code review (updated for commit c2c53bb)

Verdict: LGTM with one concern worth checking — 1 medium, 3 low

• Re-read both changed files after the follow-up commit
• Trace the Tesseract usage-info merge path
• Re-check prior review threads for staleness

Medium — Tesseract path may silently drop the trailing lookback expansion

Commit c2c53bb dropped unionDateRanges (good — mixed-precision string compare was fragile) but the replacement at PreAggregations.ts:206 now unconditionally overwrites desc.matchedTimeDimensionDateRange with mergedDateRange:

...(mergedDateRange && desc.matchedTimeDimensionDateRange ? { matchedTimeDimensionDateRange: mergedDateRange } : {}),

desc.matchedTimeDimensionDateRange at this point already includes the trailing-window shift from expandRangeByTrailingWindow (the whole point of the fix). mergedDateRange comes from usageInfo.usages[*].dateRange which reflects the raw query date range, not the rolling-window lookback. So for a Tesseract query with a bounded rolling-window measure served via a rollup with usageInfo populated, the trailing lookback expansion introduced by this PR is thrown away at line 206 — reintroducing the original bug on the Tesseract path.

If usages[*].dateRange is guaranteed to be ≥ base range on both sides in every real code path, this is fine — but that's not obvious from the code and it's the exact invariant the removed union was defending. Worth either:

• Keeping the union but normalizing both endpoints to a single format first (e.g. moment(str).format(format) with this.query.timestampPrecision() — same format used in expandRangeByTrailingWindow), or
• Asserting the invariant with a comment plus a test (bounded rolling-window measure exercised through the Tesseract usage-info path).

The new core-164-rolling-window-partitions.test.ts covers the non-usage-info path only, so this regression wouldn't be caught.

Low — cumulativeMeasuresTrailingInterval() recomputes per pre-agg

Walks leaf measures on every preAggregationDescriptionFor call. hasCumulativeMeasures() right above it is memoized (hasCumulativeMeasuresValue) for the same reason. Worth caching, especially for queries touching many rollupJoin/rollupLambda references.

Low — parseSqlInterval compared via moment.duration(...).asSeconds()

PreAggregations.ts:354 — 1 month vs 30 day picks the "largest" by moment's approximate month length. Acceptable for a lookback shift (over-approximating is safe), just worth a one-liner comment noting the approximation.

Low — dead branch in the unbounded check

PreAggregations.ts:346-351: the leading === 'unbounded' case in the third if is unreachable because the second if already returns null for every truthy non-'unbounded' leading. Not a bug, but the three ifs collapse to two without behavior change.

---

Positive

• The null / ParsedInterval / undefined tri-state return from cumulativeMeasuresTrailingInterval cleanly encodes the three cases and keeps preAggregationDescriptionFor readable.
• Refusing to narrow when any leaf measure has an unbounded window / leading / non-fixed type is the conservative, correct default.
• Test file directly exercises the regression (bounded vs unbounded) — will stay red on a regression.

• [`igor/core-164-should-only-the-pre-aggregation-partitions-that-is-needed-to`](https://github.com/cube-js/cube/tree/igor/core-164-should-only-the-pre-aggregation-partitions-that-is-needed-to)

[claude]

Claude finished @igorlukanin's task in 1m 52s —— View job

---

Code review (updated for commit 20cfbce)

Verdict: LGTM once the Tesseract path is addressed — 1 medium, 2 low. Inline comments posted.

Details

• Re-read both changed files after c2c53bb + 20cfbce
• Trace preAggregationDescriptionsForUsageInfos merge path and Tesseract extract_date_range
• Post inline comments on surviving concerns

Medium — Tesseract usage-info merge discards the trailing-window expansion (PreAggregations.ts:206)

Commit c2c53bb correctly dropped the fragile lexicographic-string union, but the replacement now unconditionally overwrites desc.matchedTimeDimensionDateRange with mergedDateRange when both exist. desc.matchedTimeDimensionDateRange already includes the trailing-window shift added by this PR; mergedDateRange comes from usageInfo.usages[*].dateRange, which on the Tesseract side (extract_date_range in rust/.../pre_aggregation/optimizer.rs:436) is the raw filter date range with only a time-shift applied — no rolling-window lookback. So on the Tesseract usage-info path the CORE-164 fix is silently reverted. The new test file doesn't exercise usageInfo, so this wouldn't be caught. Details + suggested fixes in the inline comment.

Low — cumulativeMeasuresTrailingInterval() recomputes per pre-agg call — worth memoizing like hasCumulativeMeasuresValue.

Low — dead branch at PreAggregations.ts:346-351 (leading === 'unbounded' is unreachable).

Positive

• Dropping unionDateRanges was the right call — normalized-comparison is a much cleaner approach.
• The null / ParsedInterval / undefined tri-state cleanly encodes the three cases and keeps the caller readable.
• The precision test added in 20cfbce guards a real footgun (BigQuery microsecond precision) — nice catch.

• igor/core-164-should-only-the-pre-aggregation-partitions-that-is-needed-to •

[igorlukanin]

Follow-up: my first cut regressed the bigquery-export-bucket-gcs driver test Tesseract: SQL API: Timeshift measure from cube:

Date range expected to be in YYYY-MM-DDTHH:mm:ss.SSS format but 2019-12-01T00:00:00.000,2020-12-31T23:59:59.999999 found

I had added a unionDateRanges step in the Tesseract usage-info path (preAggregationDescriptionsForUsageInfos) to preserve a rolling window's trailing lookback through the time_shift merge, but it string-combined my millisecond-precision range with Tesseract's microsecond-precision (.999999) per-usage range. Only BigQuery (timestamp precision 6) exposes it.

Reverted that union — the usage-info path is now identical to master, and the fix is isolated to the base-path trailing-window expansion. Added a BigqueryQuery unit test asserting the expanded range keeps the query's timestamp precision so it can't silently regress.

[claude]

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

---

Code review (updated for commit 2d39f7e)

Verdict: LGTM. Both prior threads addressed; one narrow limitation to flag on the Tesseract path.

Details

• Re-read both changed files after 2d39f7e
• Verify prior review threads are resolved by the follow-up commits
• Trace the Tesseract usage-info merge path again against the deferred fix

Addressed since last review

• Dead leading === 'unbounded' branch collapsed into if (rollingWindow.leading || rollingWindow.trailing === 'unbounded') at PreAggregations.ts:346.
• cumulativeMeasuresTrailingInterval() memoized via cumulativeMeasuresTrailingIntervalValue + …Computed flags, mirroring hasCumulativeMeasuresValue right next to it. Correctly caches the tri-state (undefined / null / ParsedInterval) — a plain != null cache guard would have been wrong; using a separate …Computed boolean is the right shape.
• BigQuery microsecond-precision regression fixed by dropping the fragile unionDateRanges and adding 20cfbce's precision-preserving assertion via this.query.timestampPrecision() in expandRangeByTrailingWindow.

Remaining, informational

• Low — Tesseract usage-info path is not covered by this fix. PreAggregations.ts:210 still overwrites desc.matchedTimeDimensionDateRange with mergedDateRange, so on a query that goes through preAggregationDescriptionsForUsageInfos (Tesseract) and has a bounded rolling window, the base-path trailing expansion is thrown away — same behaviour as master, i.e. every partition still builds on that path. The author's own follow-up note calls this out ("the fix is isolated to the base-path trailing-window expansion") and the alternative (a normalized-precision union) caused a real BigQuery regression, so deferring is defensible. Worth a follow-up CORE ticket + a // TODO at line 210 so the next reader isn't left inferring why the union is absent.
• Low — moment.duration(...).asSeconds() at PreAggregations.ts:344 picks "largest" trailing using moment's approximate month/year lengths, so 1 month vs 30 day is essentially arbitrary. Safe for a lookback shift (over-shifting only builds a redundant partition, never drops one) — just an implicit assumption worth a one-liner in the comment.

Positive

• The tri-state null / ParsedInterval / undefined return from cumulativeMeasuresTrailingInterval cleanly encodes the three cases and keeps preAggregationDescriptionFor readable.
• Test file directly asserts both the bounded (narrowed + shifted) and unbounded (untouched) cases — future regressions will be red.
• The 20cfbce precision test guards a real footgun that already burned this PR once — nice add.

Re-affirmed 0 prior threads (both resolved by the follow-up commit).

• igor/core-164-should-only-the-pre-aggregation-partitions-that-is-needed-to •

[igorlukanin]

On the Tesseract concern (PreAggregations.ts:206): that overwrite is identical to master — the fix here is deliberately isolated to the legacy-planner base path, and these two paths are mutually exclusive. preAggregationDescriptionsForUsageInfos only runs when preAggregationUsageInfos is populated, which happens exclusively from the native result (BaseQuery.js:1035), i.e. Tesseract; expandRangeByTrailingWindow only runs on the legacy path.

And Tesseract never feeds a rolling-window measure through that path with the un-expanded range. Cumulative/multi-stage measures are rejected by the direct pre-agg matcher (measure_matcher.rs, is_cumulative() → false) and matched only at the leaf-CTE level, where the date-range filter has already been widened by the trailing/leading window in make_rolling_base_state / replace_date_range_for_rolling_window. So extract_date_range reads the already-expanded range and mergedDateRange carries it — the CORE-164 behavior holds on Tesseract too.

Fair point that extract_date_range has no trailing logic of its own and leans on that upstream invariant, but that's existing Tesseract structure rather than something this PR should reach across languages to defend.

[claude]

Code review

• Read PR commits and diff
• Read changed files in full
• Check prior review threads for staleness
• Post review

View job run

[igorlukanin]

Both Lows are now spelled out in the code in 188b4c9fbb — no TODO on the usage-range overwrite, since the Tesseract path already carries the widened range rather than losing it, and the approximate month/year comparison only ever over-shifts the lookback.

[claude]

Claude finished @igorlukanin's task in 3m 45s —— View job

---

Code review (updated for commit 188b4c9)

Verdict: LGTM — 0 blocking, 2 low (inline), 2 observations. Both prior threads confirmed resolved; the two doc commits address the earlier Lows.

Details

• Read PR commits and full diff against origin/master
• Read both changed files, plus BaseMeasure, BaseTimeDimension, BaseFilter, shared/time.ts
• Check prior review threads for staleness (gh list-review-threads)
• Post inline comments and summary

Addressed since the last review

• 188b4c9 documents both remaining Lows in the code: the usage-range overwrite at PreAggregations.ts:205-209 now explains why it isn't a union (Tesseract-only path, mutually exclusive with expandRangeByTrailingWindow, and the union mixed timestamp precisions), and PreAggregations.ts:364-366 notes that approximate month/year lengths only ever over-shift the lookback. That's the right resolution — a comment is more durable than a TODO for an invariant that holds.
• Both open claude threads (dead leading === 'unbounded' branch, memoization) are isResolved: true and genuinely fixed in 3b2da34. Nothing to resolve or re-affirm.

Low — dead code (inline at PreAggregations.ts:320)

preAggregationDescriptionFor was the only caller of the instance hasCumulativeMeasures(). After the switch to cumulativeMeasuresTrailingInterval(), that method (line 299), the hasCumulativeMeasuresValue field (line 120), and public static hasCumulativeMeasures (line 1403 — called only from line 301) have no callers left anywhere in packages/*/src, packages/*/test, or rust/. computeCumulativeMeasuresTrailingInterval() also duplicates the static's leaf-measure walk, and the new comments at lines 320/331 reference the now-dead method. Suggested collapse in the inline comment.

Low — host-timezone parse in expandRangeByTrailingWindow (inline at PreAggregations.ts:390)

moment(range[0], format) parses offset-less query-timezone wall clock in the host's local timezone. Normally harmless (moment's subtract(n, 'day') preserves clock time across DST), but if the computed start lands on a nonexistent/ambiguous local wall time it's normalized forward, shifting the lookback later and making output host-dependent. moment.utc(...) removes the dependency with no change to the arithmetic.

Observation — the change is slightly broader than the PR description

canMatchTimeDimension now also gates the filters block at line 462, not just the time-dimension match. On master, a cumulative query with an inDateRange filter (and no time-dimension dateRange) did get matchedTimeDimensionDateRange from filters[0].formattedDateRange() — narrowed with no trailing lookback, i.e. the same class of bug this PR fixes, but silently dropping data instead of over-building. This PR makes that path correct: bounded windows get the expansion, unbounded ones stop narrowing at all. Worth a line in the description, since for unbounded-trailing + inDateRange it trades narrower builds for correctness rather than being a pure win on partition count.

Testing

Coverage of the two headline cases plus the BigQuery precision guard is good and directly red-on-regression. Two gaps, both cheap:

• The maxTrailingSeconds comparison at lines 367-369 is untested — a query with two cumulative measures (trailing: 7 day and trailing: 1 month) asserting the range shifts by the larger window would cover it. This is likely the branch Codecov flags as partial.
• The filter-based path (inDateRange filter, no time-dimension dateRange) reaches expandRangeByTrailingWindow via a different formatter and isn't exercised.

I could not run the suite in this environment — npx jest isn't in the allowed tool list, so the assertions above are from reading only. Worth confirming test/unit/core-164-rolling-window-partitions.test.ts locally if CI doesn't cover it.

Positive

• Refusing to narrow on any unbounded trailing / any leading / non-fixed window is the correct conservative default, and offset: 'start' | 'end' both stay inside [start − trailing, end] (checked against rollingWindowDateJoinCondition, BaseQuery.js:1147), so the shift is a safe superset in every reachable case.
• The degenerate rollingWindow: {} case the earlier review worried about is fine on inspection: with no trailing and no leading the SQL window is the range itself (or empty), so returning undefined and narrowing unshifted is correct, not lossy.
• The tri-state undefined / ParsedInterval / null return keeps preAggregationDescriptionFor readable, and the separate …Computed flag is the right memoization shape for it.

• igor/core-164-should-only-the-pre-aggregation-partitions-that-is-needed-to •

[claude]

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

---

Code review (updated for commit 03287e2)

Verdict: LGTM — 0 blocking, 2 low (test gaps + residual duplication). Both previous inline threads are fixed and already resolved.

Details

• Read PR commits and full diff against origin/master
• Read both changed files, plus shared/time.ts, CubeValidator interval rules, rollingWindowDateJoinCondition
• Check prior review threads for staleness (gh list-review-threads — all 4 already isResolved: true, nothing to resolve or re-affirm)
• Post review

Verified fixes in 03287e2

• expandRangeByTrailingWindow now parses with moment.utc(range[0], format) (PreAggregations.ts:382), with a comment explaining why. That removes the host-TZ/DST dependency; subtractInterval clones and subtracts per unit, so the arithmetic is unchanged. The note about no test is right — packages/cubejs-schema-compiler/package.json:25 pins TZ=UTC for the unit run, so a spec can't reach the host-TZ path.
• The dead private hasCumulativeMeasures() and hasCumulativeMeasuresValue are gone; the remaining public static hasCumulativeMeasures (line 1395) is a reasonable keep as exported surface, and the "Mirror the static hasCumulativeMeasures()" comment at line 321 still reads correctly now that the static is the only one left.

Extra checks that came out clean

• Interval parsing is safe. CubeValidator.ts:72-83 constrains trailing/leading to /^(-?\d+) (minute|hour|day|week|month|quarter|year)s?$/ or 'unbounded', so parseSqlInterval can't hit its parts[i + 1].toLowerCase() crash path on a validated schema.
• Negative trailing (-1 day, allowed by that regexp) is still a superset. subtractInterval with {day: -1} moves the start forward, and rollingWindowDateJoinCondition (BaseQuery.js:1147) computes the SQL bound the same way, so the matched range never starts later than the window needs.
• offset is safe both ways. With the default offset: 'end' and no isFromStartToEnd, the per-interval trailing bound is dateTo − trailing, which is later than the rangeStart − trailing this PR shifts to — a superset in every reachable case.
• Sub-ms precision. On BigQuery (SSSSSS) moment truncates fractional input to milliseconds, so a .123456 start round-trips as .123000 — earlier, i.e. still a superset. Only the start is reformatted; range[1] is passed through verbatim, which is what keeps the earlier .999999 regression from coming back.

Low — test gaps (carried over, both cheap)

• The maxTrailingSeconds comparison at lines 356-360 is still unexercised. A query with two cumulative measures (trailing: 7 day + trailing: 1 month) asserting the shift uses the larger window would cover it — likely the branch Codecov reports as partial.
• The filter-only path (inDateRange filter, no time-dimension dateRange) reaches expandRangeByTrailingWindow through filters?.[0]?.formattedDateRange() and isn't tested. It's also the path where the canMatchTimeDimension gate at line 451 changes master behaviour (previously narrowed with no lookback for unbounded windows — a data-dropping bug this PR incidentally fixes), so a test there guards the more interesting half of the change.

Low — residual duplication

computeCumulativeMeasuresTrailingInterval() re-implements the leaf-measure walk of the static hasCumulativeMeasures(). Not worth churn now given the tri-state return, but the two will drift if leaf collection changes; the comment at line 321 is the only thing tying them together.

Not run

I couldn't execute the suite here — npx jest/node --eval aren't in the allowed tool list, so everything above is from reading. Worth a local yarn unit -t 'CORE-164' if CI doesn't cover the new file.

Positive

• The tri-state undefined / ParsedInterval / null contract plus the separate …Computed memo flag is the right shape, and the doc comments added in 188b4c9/03287e2 now carry the two non-obvious invariants (Tesseract usage-range overwrite, approximate month/year comparison) in the code rather than in the PR thread.
• Refusing to narrow on any unbounded trailing / any leading / non-fixed window remains the correct conservative default.

• igor/core-164-should-only-the-pre-aggregation-partitions-that-is-needed-to •

[igorlukanin]

Both test gaps covered in e4b424c64f.

The maxTrailingSeconds pick now has a spec with a 7 day and a 1 month measure on one pre-agg, asserting the start shifts back by the month. Confirmed it's a real test: it goes red if the comparison picks the first window instead of the largest.

The filter-only path turned out to be unreachable rather than untested, so I pinned that instead of asserting a lookback that never happens: with the range expressed only as an inDateRange filter, the partitioned rollup doesn't match at all — for a plain count as much as for a rolling window, so it's not something this change introduced. Same limitation #11400 pinned as a skipped FIXME. The spec now asserts the empty description for both measures, so if rollup matching ever starts covering that shape the untested lookback surfaces as a failure instead of shipping silently.

[claude]

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

---

Code review (updated for commit e4b424c)

Verdict: LGTM — 0 blocking, 2 low (test strength only). Both test gaps from the last round are addressed; the filter-only claim checks out against the matcher.

Details

• Read PR commits and the full diff (c3edaf0..HEAD)
• Review e4b424c's two new specs against the source
• Verify the "filter-only path is unreachable" claim in transformQueryToCanUseForm
• Check prior review threads for staleness (gh list-review-threads — all 4 already isResolved: true, nothing to resolve or re-affirm)
• Post review

Verified in e4b424c

Largest-window pick. rolling_mixed_pa over rolling_7d + rolling_1m with a single-day range asserts a start of 2024-05-10, i.e. the month wins over the 7 days. That does exercise maxTrailingSeconds (PreAggregations.ts:356-360) — a first-wins implementation gives 2024-06-03 and goes red, matching the author's out-of-band check. One residual gap noted inline.

Filter-only pin. The claim that this shape can't reach filters?.[0]?.formattedDateRange() holds up in the matcher, and for the reason the comment gives — not for anything rolling-window specific:

• allFiltersWithinSelectedDimensions (line 657) tests filter members against dimensionsList, which holds the query's plain dimensions. rent.date is a time dimension, so it isn't in that list → false.
• collectFilterDimensionsWithSingleValueEqual is seeded from the same dimensionsList.concat(segmentsList) map, so filterDimensionsSingleValueEqual comes back null.
• Both arms of the rollup-match condition at lines 825-828 therefore fail, for count as much as for rolling_7d.

Pinning the non-match instead of asserting a lookback that never fires is the right call — a spec asserting behaviour on an unreachable path would have been worse than no spec. preAggregationsDescription() always returns an array (line 138), so toEqual([]) is a sound assertion rather than an accidental undefined match.

Low — test strength (inline at test line 126)

The largest-window spec doesn't separate "picks the largest" from "picks the last", since rolling_1m is already last in measure order; and desc.find(...) fails as a TypeError rather than a match failure if the mixed rollup stops being chosen. Suggestion inline.

Low — shared-schema coupling

Adding rolling_mixed_pa to the module-level SCHEMA means rolling_7d is now covered by two pre-aggregations, so the first spec's expect(desc[0].preAggregationId).toEqual('rent.rolling_7d_pa') is now also an implicit assertion about pre-agg selection order, not just about the matched range. It's pinned rather than silent, which is fine, but a future change to rollup ranking would surface as a confusing failure in the bounded-window spec. Not worth restructuring now.

Low — loop without a label

The filter-only spec loops over ['rent.count', 'rent.rolling_7d'] inside one it, so a failure doesn't say which measure regressed. it.each would give that for free.

Not run

I couldn't execute the suite — this checkout has no installed node_modules for cubejs-schema-compiler (no node_modules/.bin/jest, no dist), and yarn install isn't in the allowed tool list. Everything above is from reading the source and the matcher. Worth confirming CI covers test/unit/core-164-rolling-window-partitions.test.ts.

Positive

• The source side is unchanged since 03287e2 and still reads well: the tri-state undefined / ParsedInterval / null contract, the separate …Computed memo flag, moment.utc parsing, and the two invariant comments (PreAggregations.ts:203-207 on the Tesseract usage-range overwrite, 356-358 on the approximate month/year compare) carry the non-obvious reasoning in the code rather than in this thread.
• Negative trailing (-1 day, permitted by the CubeValidator regexp) sorts below the maxTrailingSeconds = -1 sentinel, leaving maxTrailing unset and the range unshifted — which is correct, since a negative trailing moves the window start later than the range start. Worth knowing that's load-bearing, not accidental.
• Refusing to narrow on any unbounded trailing / any leading / non-fixed window remains the correct conservative default.

• [`igor/core-164-should-only-the-pre-aggregation-partitions-that-is-needed-to`](https://github.com/cube-js/cube/tree/igor/core-164-should-only-the-pre-aggregation-partitions-that-is-needed-to)

[claude]

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

---

Code review (updated for commit 6ad6f71)

Verdict: LGTM — 0 blocking, 1 low (test ergonomics). The last inline thread is addressed correctly; source is unchanged since 03287e2.

Details

• Read PR commits and the full diff (c3edaf0..HEAD)
• Review 6ad6f71's test changes against the source
• Verify the both-orders assertion actually discriminates largest-wins from last-wins
• Check prior review threads for staleness (gh list-review-threads — all 5 already isResolved: true, nothing to resolve or re-affirm)
• Post review

Verified in 6ad6f71

Both measure orders. The it.each over ['7d first', …] / ['1m first', …] does close the gap. computeCumulativeMeasuresTrailingInterval walks leaf measures in query-measure order, so with rolling_1m first a last-wins implementation (maxTrailing = parsed with no comparison) yields the 7-day window → 2024-06-03, red against the expected 2024-05-10. Both orders expect the same shifted start, which is the right invariant to pin — the pick must not depend on position. Matches the author's out-of-band check.

desc.find(...) removed. expect(ids).toContain('rent.rolling_mixed_pa') now fails as a match assertion rather than a TypeError when the mixed rollup stops being chosen, and toContain over an exact list is the right call here — rolling_7d_pa legitimately also matches in the 7d first case, so toHaveLength(1) (my earlier suggestion) would have been wrong for this spec. Good correction.

_label is unused but matches argsIgnorePattern: '^_.*' in packages/cubejs-linter/index.js:52, so no lint noise.

Low — the index lookup reads awkwardly

desc[ids.indexOf('rent.rolling_mixed_pa')] (test line 138) recomputes a lookup the toContain just proved. desc.find(...) after the toContain guard would be equivalent and clearer — the guard already removes the TypeError concern that motivated dropping find. Purely cosmetic.

Carried over, unchanged

• The filter-only spec (line 150) still loops over ['rent.count', 'rent.rolling_7d'] inside one it, so a failure doesn't say which measure regressed. it.each would give that for free — same pattern now used two specs above.
• Adding rolling_mixed_pa to the shared module-level SCHEMA means the first spec's expect(desc[0].preAggregationId).toEqual('rent.rolling_7d_pa') is implicitly an assertion about rollup selection order. Pinned rather than silent, so acceptable.

Not run

I couldn't execute the suite — this checkout has no installed node_modules for cubejs-schema-compiler and yarn install isn't in the allowed tool list. Everything above is from reading the source and tests. Worth confirming CI covers test/unit/core-164-rolling-window-partitions.test.ts.

Positive

• The source side is unchanged since 03287e2 and still reads well: the tri-state undefined / ParsedInterval / null contract, the separate …Computed memo flag, moment.utc parsing, and the two invariant comments (PreAggregations.ts:203-207 on the Tesseract usage-range overwrite, 356-358 on the approximate month/year compare) keep the non-obvious reasoning in the code rather than in this thread.
• Every low from earlier rounds has now been either fixed or explicitly pinned with a reason in the code — the filter-only non-match spec in particular is a better outcome than the lookback assertion I originally asked for, since that path is unreachable.
• Refusing to narrow on any unbounded trailing / any leading / non-fixed window remains the correct conservative default.

• igor/core-164-should-only-the-pre-aggregation-partitions-that-is-needed-to •