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.51%. Comparing base (760a645) to head (2d39f7e).
⚠️ Report is 43 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 ⚠️

❗ There is a different number of reports uploaded between BASE (760a645) and HEAD (2d39f7e). Click for more details.

HEAD has 1 upload less than BASE

Flag	BASE (760a645)	HEAD (2d39f7e)
cubesql	1	0

Additional details and impacted files

@@             Coverage Diff             @@
##           master   #11277       +/-   ##
===========================================
- Coverage   79.43%   59.51%   -19.93%     
===========================================
  Files         473      216      -257     
  Lines       97382    17539    -79843     
  Branches     3539     3552       +13     
===========================================
- Hits        77354    10438    -66916     
+ Misses      19522     6593    -12929     
- Partials      506      508        +2     

Flag	Coverage Δ
cube-backend	59.51% <95.00%> (+0.07%)	⬆️
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 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]

Minor: the rollingWindow.leading === 'unbounded' case in the third if is unreachable — the preceding if (rollingWindow.leading && rollingWindow.leading !== 'unbounded') returns null for every truthy non-'unbounded' leading, and the only remaining truthy value is 'unbounded' itself. The two ifs collapse to one without behavior change:

if (rollingWindow.leading || rollingWindow.trailing === 'unbounded') {
  return null;
}

Not a bug, just dead code.

[igorlukanin]

Collapsed into the single check in 2d39f7e9f9.

[claude]

Low: cumulativeMeasuresTrailingInterval() walks the leaf-measure graph on every preAggregationDescriptionFor call. hasCumulativeMeasures() right above is memoized (hasCumulativeMeasuresValue) for the same reason — worth caching this too, especially for queries touching many rollupJoin/rollupLambda references.

[igorlukanin]

Memoized it alongside hasCumulativeMeasuresValue in 2d39f7e9f9.

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