fix(tesseract): retry pre-aggregation matching within one external type - #11812
fix(tesseract): retry pre-aggregation matching within one external type#11812waralexrom wants to merge 2 commits into
Conversation
…t external types A share measure whose numerator filters by brand and whose denominator lifts that filter through `filter.exclude` matches two different pre-aggregations. When those land in different external types the whole query drops to the fact table, because a set of per-stage matches spanning CubeStore and the source database cannot be read by one SQL query and the matcher gives up instead of retrying inside a single external type. Adds the fixture, a seed, and five tests: the split case, its rows compared against the fact table, the wider pre-aggregation covering every stage on its own, the narrow one covering nothing, and a whole-query match left untouched. The first two fail until the retry exists; the other three guard what the retry must not disturb. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Each stage of a multi-stage query takes its own most precise pre-aggregation, which can leave the resulting set spanning both external types. Such a set is correctly refused, since no single query can read CubeStore and the source database at once — but the refusal was terminal, so the query dropped all the way to the fact table even when one wider pre-aggregation could have covered every stage on its own. Which pre-aggregation each stage reached, and therefore whether the query was served at all, came down to their declaration order. The refusal now reports why a matching pass failed, so a set split across external types is retried against candidates of a single external type, CubeStore first. Per-stage precision is given up only in that retry, and only where the alternative is reading the fact table. A stage that matched nothing still fails immediately, as no subset of the candidates can cover it, and queries matched as a whole never enter this path — so a plain query keeps the pre-aggregation it already matched. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @waralexrom's task in 3m 56s —— View job Review complete — no blocking issues; 4 inline comments (0 high, 2 medium, 2 low)Full review
VerdictThe change is well scoped and the reasoning in the description holds up against the code. I traced the two properties it rests on:
Inline comments
Things I liked
Out of scope, noted for contextTwo pre-existing gaps in the same area that this change doesn't introduce but does interact with, in case they're worth separate issues:
What I could not verify
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| for external in [true, false] { | ||
| let candidates: Vec<_> = compiled_pre_aggregations | ||
| .iter() | ||
| .filter(|pa| pa.external.unwrap_or(false) == external) | ||
| .cloned() | ||
| .collect(); | ||
| if let MultiStageMatch::Matched(rewritten) = | ||
| self.match_multistages(root, &candidates)? | ||
| { | ||
| return Ok(Some(rewritten)); | ||
| } | ||
| } |
There was a problem hiding this comment.
Two things about this loop worth pinning down.
The [true, false] preference is unexercised. In the fixture only by_brand_day (external) can cover both stages, so test_stages_split_across_external_types_serve_from_one_engine would pass with the order reversed too. Since "CubeStore wins" is called out as a deliberate policy in the PR description, a case where both groups can cover — asserting the external one is chosen — would be the test that actually holds the policy in place against a future reordering.
Non-Matched outcomes from the retry are swallowed. A single-external pass can only return Matched or UnmatchedStage today, because the group filter (pa.external.unwrap_or(false)) and the usage flag (PreAggregation::external(), set from pre_aggregation.external.unwrap_or_default() at line 486) are the same predicate. If those two ever drift — e.g. a rollupLambda whose external is recomputed differently in the compiler — an ExternalTypesSplit from inside a single group would be silently treated as "no match" and the invariant would go unnoticed. An explicit match makes the assumption checkable:
match self.match_multistages(root, &candidates)? {
MultiStageMatch::Matched(rewritten) => return Ok(Some(rewritten)),
// A single-external group cannot split; the filter here is the same
// predicate the usage's `external()` reports.
MultiStageMatch::ExternalTypesSplit => debug_assert!(false, "single-external pass split"),
MultiStageMatch::UnmatchedStage => {}
}
| // Every stage is covered, just not by one engine. Retrying within a | ||
| // single external type gives up per-stage precision for a set that one | ||
| // query can actually read, and the alternative is reading the fact | ||
| // table. CubeStore goes first as the default and faster store. |
There was a problem hiding this comment.
Four lines where the load-bearing part is the fallback ordering — "we trade per-stage precision for a readable set, and external goes first". The rest restates the enum comment above. Suggest trimming to the sentence a later edit would need in order not to reverse the order:
| // Every stage is covered, just not by one engine. Retrying within a | |
| // single external type gives up per-stage precision for a set that one | |
| // query can actually read, and the alternative is reading the fact | |
| // table. CubeStore goes first as the default and faster store. | |
| // The set is covered, just not by one engine. Retrying within a single | |
| // external type trades per-stage precision for a set one query can read; | |
| // CubeStore goes first as the default and faster store. |
| // Outcome of one matching pass over a single set of candidate pre-aggregations. | ||
| enum MultiStageMatch { | ||
| Matched(Rc<RootQuery>), | ||
| // A stage matched no candidate, so no subset of the candidates covers it | ||
| // either. | ||
| UnmatchedStage, | ||
| // Every stage matched, but the matches span both external types, so no one | ||
| // query can read them all. | ||
| ExternalTypesSplit, | ||
| } |
There was a problem hiding this comment.
Nit: RowGrain directly above uses /// doc comments; this enum uses //, so the variant semantics — which are the non-obvious part of the whole change — don't show up in rustdoc. Worth switching to /// for consistency within the file.
| ); | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Two scenarios the PR description makes claims about but no test pins:
disable_external_pre_aggregations— the description argues the retry can never fire because external candidates are filtered out incompile_all_pre_aggregations, so a pass cannot split. That is the one configuration where this change is asserted to be a strict no-op, and it's the cheapest thing to guard: same fixture, externals disabled, expect the pre-existing outcome (no usages).- Neither group covers — a shape where the stages split and no single external type can serve them, expecting
pre_aggrs.is_empty()after two failed retries. Right now the only "returns nothing" path covered isUnmatchedStage(viatest_narrow_pre_aggregation_alone_cannot_serve_the_query), not the exhausted-retry path.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #11812 +/- ##
===========================================
+ Coverage 60.16% 80.22% +20.05%
===========================================
Files 239 500 +261
Lines 19203 105851 +86648
Branches 3886 3886
===========================================
+ Hits 11554 84916 +73362
- Misses 7099 20385 +13286
Partials 550 550
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
Problem
A multi-stage measure whose stages match different pre-aggregations gets no
pre-aggregation at all when those matches land in different external types:
/sqlreportspreAggregations: []and the query reads the raw source. Withboth matches in the same external type the query is served normally.
Reported shape: a
brand_sharemeasure whose denominator lifts the brandfilter through
filter.exclude. The numerator matches a brand-grained rollup(
external: true, CubeStore), the denominator a day-grained one(
external: false, source DB) — and the query falls through to the fact table,the worst available outcome on a large table.
Cause
Stages are matched independently, each taking the first candidate that fits
in declaration order. Once every stage has matched, a set spanning both
external types is refused — correctly, since no single query can read CubeStore
and the source database at once.
The defect is that the refusal was terminal. There is no retry, so a set
that could have been covered by one wider pre-aggregation in a single engine
was discarded along with everything else. Whether the query was served came
down to the order the pre-aggregations happen to be declared in: declaring the
wider one first makes every stage reach it and the query is served, at the cost
of plain queries matching the wide rollup instead of the precise one.
What changed
A matching pass now reports why it failed instead of a bare
None:UnmatchedStage— a stage matched no candidate. Returns immediately, sincematching is monotone in the candidate set and no subset can cover it.
ExternalTypesSplit— every stage matched, but across both external types.Matching is retried against candidates of a single external type, CubeStore
first as the default and faster store.
This is the fallback order the reported case needs: most precise per stage,
then one engine, then the fact table. Per-stage precision is given up only in
the retry, and only where the alternative is reading the fact table.
Two properties keep this contained:
before the multi-stage path is reached, so the retry cannot move it onto the
wider pre-aggregation. The retry only ever fires on a set that today yields
zero pre-aggregations.
over a subset of candidates, so it can only produce plans the matcher already
sanctioned — the same plans that reordering the model would have produced.
How it was verified
fix (
preAggregations: [],left: 0 / right: 2); three guard what the retrymust not disturb — the wider pre-aggregation covering every stage alone, the
narrow one covering nothing, and a whole-query match still choosing
by_day.re-aggregates across the brands stored in the brand-grained rollup, and the
shares come back
0.30/0.40, matching the fact-table plan exactly. Abrand predicate leaking into that read would show as a share of
1.0.cubesqlplannersuite: 1377 passed, 0 failed.cargo fmtandcargo clippyclean.Risks
across external types — a case that currently returns nothing. A stage that
matched nothing still fails on the first pass, so queries with no usable
pre-aggregation do not pay for it.
policy choice rather than a cost estimate; there is no ranking between
candidates anywhere in the matcher today.
previously read the source, so their results now depend on rollup freshness
like any other pre-aggregated query.
disable_external_pre_aggregationsis unaffected: external candidates arefiltered out before matching, so a pass can never be split and the retry
never runs.
🤖 Generated with Claude Code