Conversation
github-actions
Bot
added
the
javascript
|
Claude finished @ovr's task in 5m 33s —— View job The fix addresses #11860's leak correctly. 1 medium, 1 low — both on the shape of the change, not the mechanism. Full review
The fix does cover the reported symptomIssue #11860 is unbounded growth of Two things I checked that are fine:
Findings
Worth calling out as a bonus: the old key Not verified
Review threadsThe listing returned 0 threads of my own on this PR; none resolved, none re-affirmed. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| return this.compilerCacheFn(['partitions', JSON.stringify(buildRange)], () => partitionRanges.map(range => this.partitionPreAggregationDescription(range, buildRange))); | ||
| const buildRange = await this.effectiveDateRange(); | ||
| const { preAggregationId, tableName, dataSource, timezone, partitionGranularity, timestampFormat, timestampPrecision } = this.preAggregation; | ||
| const identity = { preAggregationId, tableName, dataSource, timezone, partitionGranularity, timestampFormat, timestampPrecision }; |
There was a problem hiding this comment.
The identity is a hand-picked seven-field subset, but the cached value depends on all of this.preAggregation.
partitionPreAggregationDescription (line 206) spreads ...this.preAggregation into every description and additionally branches on partitionInvalidateKeyQueries/invalidateKeyQueries (line 212), updateWindowSeconds (line 221), loadSql, sql, indexesSql and previewSql. None of those are in the key. The scope that key lives in is not per-pre-aggregation either: PreAggregations.expandPartitionsInPreAggregations hands the same queryBody.compilerCacheFn to a loader for every entry of preAggregationDescriptionList (PreAggregations.ts:701), all running concurrently under Promise.all. Two descriptions in that list that agree on the seven fields but differ in, say, updateWindowSeconds or invalidateKeyQueries would serve each other's plans, with the second loader getting the first's sealAt and clipping.
I could not construct that collision from the current code — distinct entries in a list carry distinct preAggregationIds — so this is about the shape rather than a live failure: the subset is correct only as long as nobody adds a field that partitionPreAggregationDescription reads, and nothing makes that coupling visible at either site. Two options that do not carry the maintenance debt:
- Key on what actually identifies the instance —
preAggregationId+tableName+timezone— and drop the four fields that are already derived from it, with a line saying the rest ofpreAggregationis assumed constant for a given id within one compiler cache. Shorter key, same guarantee, and the assumption is written down. - Or key on a hash of the whole
preAggregationminusmatchedTimeDimensionDateRange, which is exact and stays exact, at the cost of one hash per call.
Worth noting the old key had no identity component at all (['partitions', JSON.stringify(buildRange)]), so any two pre-aggregations in one list sharing a build range collided outright — this diff fixes that as a side effect, which is a good catch.
|
Claude finished @ovr's task in 6m 50s —— View job The fix matches the leak described in #11860 and I found nothing that breaks it — 1 low (inline), 1 prior thread re-affirmed, 1 resolved. Review details
VerdictThe root cause in #11860 is correctly identified and the fix is the minimal one: replacing the range-keyed What I verifiedNo regression from removing the
The cached arrays are not mutated downstream. This is the risk the change introduces — on master a moving build range produced a fresh array every refresh, which masked any in-place mutation; now the same array survives across refreshes. Every consumer of
Timezone isolation holds for the real caller. CI ordering is fine. Findings
Re-affirmed 1 prior thread that still applies: the hand-picked seven-field Resolved 1 of my own stale threads: the Not runNo tests, |
| bounds.mockResolvedValue(['2024-01-01T00:00:00.000', '2024-01-03T00:00:00.000']); | ||
| await expect(loader.partitionPreAggregations()).rejects.toThrow('requested to build 3 partitions'); | ||
| bounds.mockResolvedValue(rangeB); | ||
| const describe = jest.spyOn(loader as any, 'partitionPreAggregationDescription').mockImplementationOnce(() => { throw new Error('expansion failed'); }); |
There was a problem hiding this comment.
const describe = jest.spyOn(...) shadows Jest's global describe for the rest of the test body.
Nothing in the current body calls describe, so it passes — but the binding is live from line 1588 to the end of the arrow function, and anyone adding a nested block or moving a describe(...) into this scope gets TypeError: describe is not a function (or worse, a silent spy invocation) rather than a test. spy, describeSpy or expansion costs nothing here.
The related point: partitionPreAggregationDescription is private, which is why the call needs as any. CLAUDE.md asks for a widened subclass instead, and that works here once the method is protected:
class TestLoader extends PreAggregationPartitionRangeLoader {
public partitionPreAggregationDescription(range: QueryDateRange, buildRange: QueryDateRange) {
return super.partitionPreAggregationDescription(range, buildRange);
}
public partitionRanges(ignoreMatchedDateRange?: boolean) { return super.partitionRanges(ignoreMatchedDateRange); }
}That also covers (loader as any).partitionRanges(...) at lines 1630, 1647 and 1648, and keeps both call sites type-checked against future signature changes — the partitionRanges signature moved in this very diff.
Codecov Report✅ All modified and coverable lines are covered by tests.
Additional details and impacted files@@ Coverage Diff @@
## master #11896 +/- ##
===========================================
- Coverage 84.88% 61.77% -23.12%
===========================================
Files 261 247 -14
Lines 87542 19861 -67681
Branches 0 4051 +4051
===========================================
- Hits 74314 12270 -62044
+ Misses 13228 7023 -6205
- Partials 0 568 +568
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:
|
|
Claude finished @ovr's task in 7m 40s —— View job Approach is sound and it does address #11860 — 1 medium, 3 low posted inline; 2 prior threads re-affirmed; tests not executed (no dependency install available). Full reviewTodo
VerdictThe mechanism is right. Retaining one plan per pre-aggregation identity, keyed on the reference identity of the Things I checked and found fine:
Findings
CommentsAll three added comments earn their place — Review threadsThe listing returned 2 unresolved threads with me as the first author. Resolved 0 — both concerns are still live against the current tree:
Not verifiedI could not install dependencies in this environment ( |
Reviewing PR #11896
|
7ecf753 to
747fe89
Compare
Reviewing PR #11896
|
747fe89 to
5818834
Compare
|
Claude finished @ovr's task in 8m 24s —— View job Approve with notes — 0 blocking, 1 low (comment hygiene); 2 prior threads re-affirmedFull review
VerdictThe fix matches the symptom in #11860. The new key is strictly more specific than the old one in every dimension, so it introduces no collision the previous code did not already have, and hit rate is a subset of before (previous range only, rather than any previous range) — nothing that was correct before becomes stale now. Ordering is right: Notes (non-blocking)Dropping the Steady-state floor, not a leak. The retained slot keeps its Posted inline: Prior threadsThe listing returned 6 threads authored by
Not verified
|
| entries.set(JSON.stringify(key), entry); | ||
| return entry; | ||
| }); | ||
| // Range loading is mocked; planning uses the real loader and compiler cache. |
There was a problem hiding this comment.
Comment restates the three lines under it — jest.spyOn(loader, 'loadBuildRange') two lines down is "range loading is mocked", and new PreAggregationPartitionRangeLoader(...) with the CompilerCache-backed compilerCacheFn is "the real loader and compiler cache". A reader loses nothing by deleting it.
| // Range loading is mocked; planning uses the real loader and compiler cache. | |
| const loaders = ['UTC', 'Europe/Paris'].map(timezone => new PreAggregationPartitionRangeLoader( |
There was a problem hiding this comment.
Correction: the suggestion block above is malformed — it replaces only line 43, so applying it duplicates line 44. Just delete line 43.
This branch has not been deployed
1 participant
Check List
Issue Reference this PR resolves
Refs #11860
Description of Changes Made
Moving build ranges retained a partition plan and time-series grid for every refresh; retain only the latest plan per pre-aggregation identity within each base-query compiler cache and remove the separate time-series cache.
Replacement preserves arrays used by in-flight requests and the previous plan on failure, while cache hits still enforce
maxPartitions.Regression tests cover range changes, identity isolation and bounded retention with the real
CompilerCache, addingschema-compileras a dev dependency.Validation: 224 orchestrator tests, 139 time/timezone tests, 160 expansion comparisons, TypeScript, oxlint and
git diff --check; live PostgreSQL/CubeStore checks included 108 successful REST loads and 72 result comparisons, and the 40-refresh profile retained 10 plans with heap at 71.10 MiB versus 72.72 MiB after warmup.