fix(schema-compiler,query-orchestrator): bound the rollupLambda source query - #11708
fix(schema-compiler,query-orchestrator): bound the rollupLambda source query#11708ovr wants to merge 1 commit into
Conversation
github-actions
Bot
added
the
javascript
…e query
A rollupLambda pre-aggregation with unionWithSourceData issued a live source
query on every request. It was lower-bounded by FROM_PARTITION_RANGE and had no
upper bound, so it aggregated and CSV-downloaded the entire tail of the source
table even when the requested range sat entirely inside already-built
partitions - every row of which the outer query then discarded. In the
reporter's repro a query returning 696 cost ~330-390 ms at granularity day and
~7.0 s at granularity minute, streaming 1,271,521 rows into a Cube Store inline
table per request.
Three defects, not the two named in the issue:
- buildLambdaQuery() built the sub-query from references.timeDimensions, which
carry only {dimension, granularity}, so the requested range never reached it.
- loadPreAggregations() downloaded the lambda table whenever a lambdaQuery was
present, without checking whether the requested range even reached past the
last partition's buildRangeEnd.
- Under Tesseract - the default planner - the source query carried no LIMIT at
all. query_properties_compiler.rs reads rowLimit as parse::<usize>().ok(), so
the __MAX_SOURCE_ROW_LIMIT sentinel resolved to None and the maxSourceRowLimit
safety valve had silently stopped protecting anything. The row limit is now
resolved to a number before the options cross the bridge, and echoed back on
LambdaQuery so the orchestrator checks the count against the limit that was
actually rendered.
Generated source SQL for the reporter's model, dateRange 2024-02-01..2024-02-29:
| planner | before | after |
| --- | --- | --- |
| legacy | `WHERE ts > $1 GROUP BY 1 LIMIT $2` | `WHERE ts > $1 AND (ts >= $2 AND ts <= $3) GROUP BY 1 LIMIT 200000` |
| native | `WHERE ts > $1 GROUP BY 1` | `WHERE ts > $1 AND (ts >= $2 AND ts <= $3) GROUP BY 1 LIMIT 200000` |
The bound is not a new computation: the matchedTimeDimension/filters block moves
out of preAggregationDescriptionFor() into
PreAggregations.matchedTimeDimensionDateRangeFor(), so buildLambdaQuery and the
orchestrator skip both decide from the range that already selects partitions.
lambdaSourceDateRange() wraps it to mirror the multi-usage mergeUsageDateRanges
overwrite, and bounds nothing when a usage range is unknown. The inDateRange
filter stays separate from the afterDate one on purpose: inDateRange's lower
bound is inclusive and would double-count rows sitting exactly at buildRangeEnd.
Requests carrying no date range are untouched and still expose all fresh data,
which the smoke suite asserts.
Fixes #11682
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
5cc95c8 to
0f4b3bf
Compare
|
Claude finished @ovr's task in 4m 25s —— View job Review complete — solid, well-targeted fix; 1 medium regression risk, 1 medium correctness gap, 3 low (4 inline comments posted)Full review
OverallThe diagnosis is convincing and the shape of the fix is right: reusing the same range that already drives partition selection to bound the source half of the union is exactly the invariant you want, and hoisting it into Nothing security-relevant: the new bounds go through the normal filter/param path, no string interpolation of user input. The Findings
Smaller notes (no inline comment)
DocsNo docs change needed — this is behavior-preserving from a user-facing API standpoint. If finding #1 is resolved by keeping the env-only resolution, |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| // Tesseract renders LIMIT as an inline number and can't carry the MAX_SOURCE_ROW_LIMIT | ||
| // placeholder (it parses to None, emitting no LIMIT at all), so resolve it here. | ||
| const resolvedRowLimit = this.options.maxSourceRowLimit ?? getEnv('maxSourceRowLimit'); | ||
| const maxSourceRowLimit = typeof resolvedRowLimit === 'number' && resolvedRowLimit > 0 | ||
| ? resolvedRowLimit |
There was a problem hiding this comment.
Baking the limit here silently drops support for the preAggregationsOptions.maxSourceRowLimit config option.
Two things line up badly:
this.options.maxSourceRowLimitis not a schema-compiler query option — nothing in the repo ever puts it there (grep maxSourceRowLimitonly findspreAggregationsOptionsinOptsHandler.ts:634,LambdaOptionsin the orchestrator, and the env getter). So this always falls through togetEnv('maxSourceRowLimit')(default200000).- The runtime value that used to win is the orchestrator's
this.options.maxSourceRowLimit, whichOptsHandlerfills frompreAggregationsOptions.maxSourceRowLimitor env. That is what theMAX_SOURCE_ROW_LIMITplaceholder was substituted with indownloadLambdaTable.
So for a user with preAggregationsOptions: { maxSourceRowLimit: 5000 } in cube.js and no CUBEJS_MAX_SOURCE_ROW_LIMIT, after this change the source SQL gets LIMIT 200000, and because PreAggregationPartitionRangeLoader:425 prefers this.lambdaQuery.maxSourceRowLimit ?? this.options.maxSourceRowLimit, the guard also checks against 200000 — the configured 5000 becomes completely inert (previously it was honored on the legacy planner).
Either plumb preAggregationsOptions.maxSourceRowLimit into the compiler query options (so this.options.maxSourceRowLimit is real), or keep the placeholder when it hasn't been resolved from an authoritative source and only bake the number on the native path.
| private lambdaSourceDataCovered(buildRangeEnd?: string): boolean { | ||
| const matchedRangeEnd = this.preAggregation.matchedTimeDimensionDateRange?.[1]; | ||
| if (!matchedRangeEnd || !buildRangeEnd) { | ||
| return false; | ||
| } | ||
| // buildRangeEnd comes back from the DB and may carry a `Z` suffix. | ||
| return reformatInIsoLocal(matchedRangeEnd) <= reformatInIsoLocal(buildRangeEnd); | ||
| } |
There was a problem hiding this comment.
The skip decision and the SQL bound can disagree in the multi-usage case, and the disagreement is in the unsafe direction.
lambdaSourceDateRange() bounds the SQL by the union of every usage range for the rollupLambda, but this check reads this.preAggregation.matchedTimeDimensionDateRange, which comes from one description. With several usage infos, preAggregationDescriptionsForUsageInfos() produces one description per usage (each with its own merged range) and then preAggregationsDescription() collapses them with R.uniqBy(desc => desc.tableName) (schema-compiler/src/adapter/PreAggregations.ts:145) — the first one wins.
So if usage A ends inside the built range and usage B extends past it, the surviving description carries A's range, lambdaSourceDataCovered() returns true, and the fresh tail that B's SQL bound was explicitly widened to fetch is never downloaded. Before this change the download always ran.
Given the PR already notes that multi-usage descriptions don't add the lambda table to their per-usage unions, this may be masked today — but the two computations should be fed from the same value rather than relying on that. Consider having the schema compiler stamp the effective source range onto LambdaQuery (next to maxSourceRowLimit) and comparing against that here, so the skip can never be wider than the bound actually rendered into the SQL.
| const partitionInvalidateKeyQueries = queryForSqlEvaluation.partitionInvalidateKeyQueries?.(cube, preAggregation); | ||
| if (!preAggregation.partitionGranularity) { | ||
| return undefined; | ||
| } |
There was a problem hiding this comment.
The PR description says "so nothing is recomputed", but extracting this into a public method actually adds a computation: matchedTimeDimensionDateRangeFor() is now called once per description from preAggregationDescriptionFor() and once more from lambdaSourceDateRange() (via buildLambdaQuery). Each call re-runs this.query.allBackAliasMembers(), which is not memoized (BaseQuery.js:5472 → backAliasMembers(flattenAllMembers()), which traverses symbols and evaluates member SQL — see the warning comment at BaseQuery.js:5406).
Worth memoizing either allBackAliasMembers() or this method per foundPreAggregation, since the whole point of the change is to make lambda requests cheaper.
| if (data.rowCount === this.options.maxSourceRowLimit) { | ||
| throw new Error(`The maximum number of source rows ${this.options.maxSourceRowLimit} was reached for ${this.preAggregation.preAggregationId}`); | ||
| const appliedRowLimit = this.lambdaQuery.maxSourceRowLimit ?? this.options.maxSourceRowLimit; | ||
| if (data.rowCount === appliedRowLimit) { |
There was a problem hiding this comment.
Minor, and the PR already flags it as out of scope: now that the limit is inlined into the SQL by the compiler rather than substituted at runtime, an === check is more fragile than before — any divergence between the two values (see the config-option issue on BaseQuery.js) turns the guard from "throw" into "silently return truncated data". >= costs nothing here and fails closed.
| if (data.rowCount === appliedRowLimit) { | |
| if (data.rowCount >= appliedRowLimit) { |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #11708 +/- ##
===========================================
- Coverage 79.91% 59.03% -20.88%
===========================================
Files 498 237 -261
Lines 104694 18975 -85719
Branches 3841 3851 +10
===========================================
- Hits 83661 11202 -72459
+ Misses 20472 7208 -13264
- Partials 561 565 +4
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:
|
1 participant
Check List
Issue Reference this PR resolves
#11682
Description of Changes Made
A
rollupLambdawithunion_with_source_dataissued a live source query on every request that was lower-bounded byFROM_PARTITION_RANGEand unbounded above, so it aggregated and CSV-downloaded the whole tail of the source table even when the requested range sat entirely inside already-built partitions — ~7.0 s and 1,271,521 rows per request atgranularity: minutein the reporter's repro, to return a single number. The source query is now bounded by the same range that already selects partitions (thematchedTimeDimension/filtersblock moves out ofpreAggregationDescriptionFor()intoPreAggregations.matchedTimeDimensionDateRangeFor(), so nothing is recomputed and the two halves of the union cannot disagree), andPreAggregationPartitionRangeLoaderskips the download outright when the requested range ends at or before the last partition'sbuildRangeEnd. Separately, themaxSourceRowLimitvalve turned out to be inert under Tesseract — the default planner — becausequery_properties_compiler.rsreadsrowLimitviaparse::<usize>().ok(), so the__MAX_SOURCE_ROW_LIMITsentinel resolved toNoneand noLIMITwas emitted at all; the limit is now resolved to a number and echoed onLambdaQueryso the row-count check matches the SQL that actually ran. Requests carrying no date range are deliberately untouched and still expose all fresh data, which the smoke suite asserts. Three pre-existing bugs found nearby are left out of scope and will be filed separately:downloadLambdaTabledoes not convert itsFROM_PARTITION_RANGEsubstitution to UTC (off-by-offset lower bound on non-UTC timezones), multi-usage descriptions never add the lambda table to their per-usage unions, and therowCount === maxSourceRowLimitcheck should be>=.Generated source SQL for the reporter's model,
dateRange: ['2024-02-01', '2024-02-29']:WHERE ts > $1 GROUP BY 1 LIMIT $2WHERE ts > $1 AND (ts >= $2 AND ts <= $3) GROUP BY 1 LIMIT 200000WHERE ts > $1 GROUP BY 1WHERE ts > $1 AND (ts >= $2 AND ts <= $3) GROUP BY 1 LIMIT 200000Verified (every new test confirmed red on unmodified source first):
cubejs-schema-compilerunitcubejs-query-orchestratorunitcubejs-schema-compilerintegration postgres,rollup lambdacubejs-testingsmoke-lambdaquery with 2 dimensionsandquery monthThe new schema-compiler cases were also run under
CUBEJS_TESSERACT_SQL_PLANNER=falseand pass on both planners.