perf(query-orchestrator): reuse SQL tuples for full partitions

[ovr]

Description of Changes Made

Reuse the SQL tuple for structureVersionLoadSql and loadSql when a partition's full and load ranges match, avoiding duplicate SQL wrappers, parameter arrays, timestamp conversions, and options objects while preserving separate tuples for clipped partitions.
In an isolated 133,920-description benchmark (five runs per variant), median retained heap growth after GC fell from 248.7 to 189.7 MiB and expansion time from 1,322 to 1,008 ms (both about 24%).
Related to #11860; this reduces allocation and retention costs but does not fix the unbounded range cache.
Validation: oxlint and 96 standalone equivalence cases passed, along with nine partition-expansion Jest tests, including new UTC/New York cases asserting shared tuples for full partitions and distinct tuples and end parameters for clipped partitions, using transpilation without type checking because the default run was blocked by local cubestore-driver type errors.

Check List

• Tests have been run in packages where changes have been made if available
• Linter has been run for changed code
• Tests for the changes have been added if not covered yet — added clipped-partition cases and tuple identity assertions
• Docs have been added / updated if required — no public API change; explanatory code comment added

[claude]

Claude finished @ovr's task in 3m 10s —— View job

---

Approach is sound and safe — 1 medium (untested branch), 2 low. No correctness bug found.

Full review

• Read PR diff and changed file in full
• Read related issue Refresh worker heap grows until OOM when a partitioned pre-aggregation's build range moves with time #11860
• Check prior review threads (none exist)
• Trace downstream consumers of loadSql / structureVersionLoadSql for aliasing safety
• Check existing test coverage of both ternary branches
• Post findings

Verdict

The change is correct and the aliasing is safe. I traced every consumer of both fields and none mutates the tuple or its params array:

Consumer	Access
PreAggregationLoader.contentVersion (L377)	reads into version() → crypto.md5(JSON.stringify(...))
PreAggregationLoader.queryOptions (L457-458)	read-only cache-key material
refreshStoreInSourceStrategy (L548) / refreshWriteStrategy (L695)	destructures, passes params to the driver
QueryCache (L473)	.map(p => p.loadSql)
Drivers (BaseDriver, CubeStore, MySql, Databricks, Athena, Ksql, Crate, BigQuery)	pass params through; the only params.push calls in CubeStoreDriver are on locally-constructed arrays

Because version() is JSON.stringify-based, shared references serialize identically — no content-version or cache-key churn, so this won't trigger pre-aggregation rebuilds on upgrade. That was my main worry going in, and it's clean.

A nice incidental effect: replacePartitionSqlAndParams calls new Date() while computing renewalThreshold. Previously the two tuples got two independent readings that could land on different milliseconds; now the full-partition case takes one.

Findings

1. Medium — the new branch has no test coverage (inline)

The PR body claims existing partition-expansion tests cover the affected behaviour. They cover only the aliased side. mockPreAggregation sets partitionInvalidateKeyQueries: [], which makes the clipping guard at L193 false, and the mocked build range ends exactly on the last partition boundary. All seven partitionPreAggregations tests take loadSql === structureVersionLoadSql. Inverting the ternary would not fail the suite. The one behaviour this diff must preserve — a clipped partition keeping its own TO_PARTITION_RANGE param — is the thing nothing asserts.

2. Low — range[0] === loadRange[0] is structurally dead (inline)

loadRange = [...range] and only index 1 is ever reassigned, so the first comparison is always true.

3. Low — the comment states the wrong load-bearing fact (inline)

"Reuse the tuple to avoid duplicate allocations" narrates the diff, which the commit message already does. The fact worth pinning down is that the two fields are now often the same mutable tuple, so consumers must not mutate params — QueryWithParams doesn't encode that invariant. Suggested a restructure that names the intent via a local helper and drops the dead comparison.

On #11860

Correctly scoped as perf: and the body is honest that this isn't the fix. Worth keeping expectations calibrated for anyone reading this as a remediation: the ['partitions', JSON.stringify(buildRange)] entry in the schema-compiler QueryCache is still unbounded with no eviction, and CompilerCache's updateAgeOnGet: true still means the scheduler's per-run read keeps it alive forever. A ~24% constant-factor cut moves the reporter's OOM from ~11.5h to roughly ~15h; the heap still grows without bound. The remaining per-partition allocations in the same function (sql, invalidateKeyQueries, indexesSql, previewSql, and the ...this.preAggregation spread) are all keyed to distinct ranges, so there's no comparable sharing left to harvest here — the real fix has to be eviction or a narrower cache key.

Not verified

I could not run yarn lint or the Jest suite — node_modules is absent in this checkout and a full monorepo install wasn't warranted for a 9-line diff. The findings above are from reading the code and tracing consumers, not from a test run. The PR author reports oxlint and the seven partition-expansion tests passing locally.

· branch `profile-postgres-11860`

[claude]

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

---

Both prior findings addressed — no new issues. Approve-equivalent; nothing blocking.

Full review (re-review of `11e1ac3`)

• Re-read the diff against base 2762d76
• Verify the new test actually reaches the clipped branch
• Check the revised condition and comment against the prior round's findings
• Resolve my two stale threads

What changed since the last round

Prior finding	Status
Medium — clipped branch untested	Fixed by 11e1ac3
Low — range[0] === loadRange[0] structurally dead	Fixed — condition is now range[1] === loadRange[1]
Low — comment narrated the diff instead of the invariant	Fixed — now states the invariant (consumers must not mutate the shared tuple), 2 lines

Both of my threads are resolved.

The new test does reach the branch

Traced it rather than trusting the name:

• partitionInvalidateKeyQueries: [['SELECT NOW()', [], {}]] makes the guard at PreAggregationPartitionRangeLoader.ts:193 true (the default mockPreAggregation value of [] was what made it unreachable before).
• loadBuildRange is mocked to end at 2024-01-03T12:00:00.000, mid-partition, so buildRangeEnd < range[1] holds for the third partition only — loadRange[1] is reassigned and the ternary takes the non-aliased leg.
• Spying on loadBuildRange (rather than loadRangeQuery) is the right seam: partitionRanges() calls it at line 428, and compilerCacheFn defaults to a pass-through (line 84), so nothing caches around the assertion.
• Asserting results.slice(0, -1) aliases via toBe and the last one doesn't pins both legs — inverting the ternary now fails. The loadSql[1] not.toBe also catches a params array that is shared while the outer tuple is fresh.

The America/New_York row is a genuinely useful second case, not padding: it exercises localTimestampToUtc on both legs, so a bug that dropped the per-leg conversion (e.g. converting range twice) would surface as 17:00/04:59:59.999 mismatches rather than passing trivially in UTC where the local and UTC strings coincide.

Aliasing safety (unchanged from last round, re-confirmed)

loadSql and structureVersionLoadSql are now the same object for unclipped partitions. All consumers are read-only: contentVersion hashes through JSON.stringify, queryOptions uses them as cache-key material, refreshStoreInSourceStrategy/refreshWriteStrategy destructure and pass params to the driver, and the only params.push calls in CubeStoreDriver are on locally-constructed arrays. Shared references serialize identically, so no content-version churn and no pre-aggregation rebuild on upgrade. QueryWithParams still doesn't encode the immutability, which is exactly what the new comment is for.

On #11860 (unchanged)

Still correctly scoped as perf: and the body is honest that this isn't the fix. The ['partitions', JSON.stringify(buildRange)] entry in the schema-compiler QueryCache remains unbounded, and CompilerCache's updateAgeOnGet: true still keeps the scheduler's per-run read alive indefinitely. A ~24% constant-factor cut moves the reporter's OOM out but doesn't bound the heap; the real fix is eviction or a narrower cache key.

Not verified

node_modules is absent in this checkout, so I could not run Jest or oxlint — a full monorepo install wasn't warranted for a 36-line diff. The test analysis above is from reading the code paths, not a green run. Types check out by inspection: loadSql/structureVersionLoadSql are non-optional QueryWithParams (PreAggregations.ts:197-199), so the unguarded [1] indexing in the test compiles without a cast.

· branch `profile-postgres-11860`

[claude]

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

---

Only the comment changed since the last round — 1 low (comment wording). Nothing blocking.

Full review (re-review of `087a4a1`)

• Diff 087a4a1 against the previously reviewed 11e1ac3
• Re-read partitionPreAggregationDescription in full, not just the hunk
• Re-check the clipped-branch test still reaches the branch
• Check prior review threads (both already resolved)
• Post findings

What changed

087a4a1 touches nothing but the comment. Logic and tests are byte-identical to the commit I reviewed at 11:26:

-    // Full partitions use the same SQL and parameters for loading and structure versioning.
-    // Reuse the tuple to avoid duplicate allocations; a clipped load range needs its own parameters.
+    // Reuse the SQL tuple for unclipped partitions to reduce computation and memory allocations.

Two lines → one. The condition is still range[1] === loadRange[1], the test still covers both legs.

Findings

1. Low — the new wording restates the code instead of the invariant (inline)

range[1] === loadRange[1] ? structureVersionLoadSql : … already reads as "reuse the tuple for unclipped partitions", and the commit subject already records the motive. The fact the code can't state is the consequence: the two fields are the same object on the common path, and QueryWithParams doesn't encode immutability, so a future params.push(...) in a driver would corrupt the structure-version input. That's the sentence worth keeping. Non-blocking.

Re-confirmed from the prior round

• Aliasing is safe. Every consumer is read-only: contentVersion hashes via JSON.stringify, queryOptions uses the tuples as cache-key material, refreshStoreInSourceStrategy/refreshWriteStrategy destructure and pass params to the driver, QueryCache maps over loadSql. The only params.push calls in CubeStoreDriver are on locally-constructed arrays. Shared references serialize identically → no content-version churn, no pre-aggregation rebuild on upgrade.
• The clipped-branch test genuinely reaches the branch. partitionInvalidateKeyQueries: [['SELECT NOW()', [], {}]] makes the guard at PreAggregationPartitionRangeLoader.ts:193 true (the default [] in mockPreAggregation was what made it unreachable), and the mocked loadBuildRange ending at 2024-01-03T12:00:00.000 makes buildRangeEnd < range[1] hold for the third partition only. toBe on slice(0, -1) plus not.toBe on the last pins both legs — inverting the ternary fails. The America/New_York row is load-bearing, not padding: it exercises localTimestampToUtc on both legs, which UTC alone can't distinguish.
• Dead range[0] === loadRange[0] comparison was dropped in 11e1ac3; still gone.

On #11860 (unchanged)

Correctly scoped as perf:, and the body is honest that this isn't the fix. The ['partitions', JSON.stringify(buildRange)] entry in the schema-compiler QueryCache is still unbounded, and CompilerCache's updateAgeOnGet: true still keeps the scheduler's per-run read alive indefinitely. A ~24% constant-factor cut pushes the reporter's OOM out but doesn't bound the heap — the real fix is eviction or a narrower cache key.

Not verified

node_modules is absent in this checkout (root and package both), so I could not run Jest or oxlint — a full monorepo install wasn't warranted for a one-line comment change. The analysis above is from reading the code, not a green run. Note that 087a4a1 cannot affect test outcomes: the only changed bytes are inside a // comment.

· branch `profile-postgres-11860`

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 60.25%. Comparing base (2762d76) to head (fda87b3).
⚠️ Report is 2 commits behind head on master.

❗ There is a different number of reports uploaded between BASE (2762d76) and HEAD (fda87b3). Click for more details.

HEAD has 1 upload less than BASE

Flag	BASE (2762d76)	HEAD (fda87b3)
cubesql	1	0

Additional details and impacted files

@@             Coverage Diff             @@
##           master   #11866       +/-   ##
===========================================
- Coverage   80.35%   60.25%   -20.10%     
===========================================
  Files         500      239      -261     
  Lines      106555    19180    -87375     
  Branches     3881     3882        +1     
===========================================
- Hits        85618    11557    -74061     
+ Misses      20387     7074    -13313     
+ Partials      550      549        -1     

Flag	Coverage Δ
cube-backend	60.25% <100.00%> (+0.01%)	⬆️
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 @ovr's task in 1m 7s —— View job

---

fda87b3 removes two redundant assertions without weakening the test — no findings. Nothing blocking.

Full review (re-review of `fda87b3`)

• Diff fda87b3 against the previously reviewed 087a4a1
• Re-read the test in full and re-derive whether the ternary is still pinned
• Re-read partitionPreAggregationDescription in full
• Check prior review threads (all three already resolved)
• Post findings

What changed

fda87b3 touches the test only. Source is byte-identical to 087a4a1.

       const lastPartition = results[2];
-      expect(lastPartition.loadSql).not.toBe(lastPartition.structureVersionLoadSql);
-      expect(lastPartition.loadSql[1]).not.toBe(lastPartition.structureVersionLoadSql[1]);
       expect(lastPartition.loadSql[1]).toEqual([start, loadEnd]);
       expect(lastPartition.structureVersionLoadSql[1]).toEqual([start, structureEnd]);

The removals are genuinely redundant, not a loss of coverage

I re-derived this rather than assuming. Both dropped assertions are implied by the two toEquals that remain, because loadEnd !== structureEnd in both test.each rows (12:00:00.000 vs 23:59:59.999 in UTC; 17:00:00.000 vs 2024-01-04T04:59:59.999 in New York):

• loadSql[1] and structureVersionLoadSql[1] are asserted to hold different contents, so they cannot be the same array — the not.toBe on [1] was unreachable-as-a-failure.
• If the outer tuples were the same object, [1] would resolve to one array and the two toEquals would contradict each other — so the not.toBe on the tuple is likewise implied.

The inversion check still holds: flip the ternary at PreAggregationPartitionRangeLoader.ts:202 and loadSql[1] becomes [start, structureEnd], failing line 1000. Compare the wrong index and the clipped leg stops being taken, same failure. The aliased leg stays pinned by the toBe loop over results.slice(0, -1) at line 995-997. Both legs remain mutation-detecting.

Worth noting what the removal does give up, and why it doesn't matter: the assertions were the only ones stating object identity on the clipped side. A hypothetical refactor that produced a fresh outer tuple while reusing the params array would no longer be caught by identity — but it also can't arise, since replacePartitionSqlAndParams builds a new params array per call and the clipped call passes a different range. The surviving value assertions are the stronger check anyway: they pin what the params are, not just that two references differ.

Re-confirmed from prior rounds

• Aliasing is safe. Every consumer of loadSql / structureVersionLoadSql is read-only: contentVersion hashes via JSON.stringify, queryOptions uses the tuples as cache-key material, refreshStoreInSourceStrategy / refreshWriteStrategy destructure and pass params to the driver, QueryCache maps over loadSql. The only params.push calls in CubeStoreDriver are on locally-constructed arrays. Shared references serialize identically → no content-version churn, no pre-aggregation rebuild on upgrade.
• The clipped branch is genuinely reached. partitionInvalidateKeyQueries: [['SELECT NOW()', [], {}]] makes the guard at line 193 true (the default [] in mockPreAggregation was what made it unreachable), and the mocked loadBuildRange ending at 2024-01-03T12:00:00.000 makes buildRangeEnd < range[1] hold for the third partition only.
• The America/New_York row is load-bearing. It exercises localTimestampToUtc on both legs — 05:00/17:00/04:59:59.999 are conversions UTC alone can't distinguish, so a bug converting range twice would surface there.
• Dead range[0] === loadRange[0] comparison dropped in 11e1ac3; still gone.

I have no new inline comments. All three of my prior threads are already resolved and none is re-raised — including the line-201 comment-wording nit, which the author resolved without changing the wording; that's their call and it's non-blocking either way.

On #11860 (unchanged)

Correctly scoped as perf:, and the body is honest that this isn't the fix. The ['partitions', JSON.stringify(buildRange)] entry in the schema-compiler QueryCache is still unbounded, and CompilerCache's updateAgeOnGet: true still keeps the scheduler's per-run read alive indefinitely. A ~24% constant-factor cut pushes the reporter's OOM out but doesn't bound the heap — the real fix is eviction or a narrower cache key.

Not verified

node_modules is absent in this checkout (root and package both), so I could not run Jest or oxlint — a full monorepo install wasn't warranted for a two-line test deletion. The analysis above is from reading the code, not a green run. Types are unaffected: the removed lines used the same unguarded [1] indexing as the lines that remain.

· branch `profile-postgres-11860`