fix(tesseract): resolve rollup-join keys kept as a rollup's time dimension - #11747
Conversation
…nsion A rollupJoin resolves one hop at a time, looking for a leg rollup that carries that hop's key on each side. The lookup only ever read the rollups' `dimensions`, so a key declared as a rollup's `time_dimension` was invisible and the join was rejected outright, at any chain length. Widening the lookup alone is not enough: a time dimension is stored truncated to the rollup's granularity, under a granularity-suffixed column, and the ON clause was rendered from the bare member alias. Resolve each join member through the leg rollup that actually carries it and keep the resolved column on the join item, so the ON clause reads what the rollups materialize and the query's own time dimension is no longer overwritten by the join-members pass. The join then compares truncated values, so every key of a hop must be stored the same way on both sides — a key truncated to `day` can be compared neither to one truncated to `month` nor to one kept raw as a plain dimension. Both are rejected with an error that says so. A rollup declaring the key plainly is preferred over one keeping it as a time dimension, so a hop that already resolved keeps resolving to the same rollup instead of turning ambiguous. Tesseract's rejection now names the failing hop, the unmatched members and which rollupJoin to fix, as the JS message already did — a bare "No rollups found" is why this was reported as a chain-depth limitation. Chain length is not a limit: the four-cube test covers it, and it passes without any of the changes here. Refs #11362 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Leave the reference page to the docs change already open against it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
rust
|
Claude finished @waralexrom's task in 3m 10s —— View job Review complete — solid fix, well-tested; 2 medium, 3 low. 5 inline comments posted.Full review
GeneralThe diagnosis is right and the fix is in the right place. The debunking of #11362's chain-depth claim is convincing and the four-cube fixture is the right way to keep it debunked. The error-message work is genuinely valuable — "No rollups found" with no hop named is exactly why this got misfiled as a depth limit. Red-checking each test by reverting the corresponding change, and validating the integration test against a real Postgres rather than against SQL shape, is the level of rigor this kind of planner change needs. Findings
Findings 1, 2 and 4 are all limitations rather than regressions — each of these hops failed outright before the change, since the time-dimension key was invisible to the lookup. The concern is that the new errors read as verdicts about the schema when they're really about the resolution strategy. Security / performanceNothing to raise. No new input surface; the added work is filtering over TestingCoverage is good and the cases chosen are the ones that matter. Two gaps, both following from the findings above: a composite hop with two keys stored differently (finding 1), and the raw-vs-truncated fixture extended with a second Not verifiedI did not build or run the Rust or JS suites in this environment; the reported results (1328 planner tests, the Postgres integration file on both planners) are taken as stated. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| // Every key of the hop has to be stored the same way, across both sides at once. | ||
| // The two sides carry no pairing between their members — comparing them side by | ||
| // side would let a raw column on one side line up with a truncated one on the other. | ||
| let members = || from_members.iter().chain(to_members.iter()); | ||
| let stored_the_same_way = members().map(|m| &m.granularity).sorted().dedup().count() <= 1; | ||
| if stored_the_same_way { | ||
| return Ok(()); | ||
| } |
There was a problem hiding this comment.
Composite hops that mix a plain key with a time-dimension key are rejected even when each pair is comparable.
Collapsing all four member lists into one granularity set means the check is "every key of the hop, on both sides, is stored identically" — but a hop can legitimately join on more than one member. For a join like ${CUBE}.customer_id = ${other.id} AND ${CUBE}.created_at = ${other.d}, from_members resolves to [customer_id → None, created_at → Some("day")] and to_members to [id → None, d → Some("day")]. The set is {None, Some("day")}, count 2, so this is rejected with "mixes join keys its rollups store differently" even though customer_id/id pair correctly as raw and created_at/d pair correctly at day.
Not a regression (before this PR the time-dimension key wasn't visible at all, so the hop failed anyway), but the new error message actively misdescribes the schema. If pairing really can't be recovered from ResolvedJoinItem, it'd be worth at least softening the message, or checking from_members' granularity set against to_members' as multisets rather than requiring a single global value. Fix this →
| if let PreAggregationSource::Join(join) = self.source().as_ref() { | ||
| for item in join.items.iter() { | ||
| for member in item.from_members.iter().chain(item.to_members.iter()) { | ||
| let alias = member.alias(); | ||
| res.insert(member.full_name(), QualifiedColumnName::new(None, alias)); | ||
| res.insert( | ||
| member.symbol.full_name(), | ||
| QualifiedColumnName::new(None, member.column.clone()), | ||
| ); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Two things about this map that the change makes more reachable:
-
The join pass still runs after the
time_dimensionspass and still overwrites it. It's now consistent in the common case (both derivealias + "_" + granularityfrom the same base symbol), but if the rollupJoin declarestd_dates.datmonthwhile the hop resolves it atday, the join pass wins and the query's own time dimension rendersd_day.check_join_members_comparableonly compares the two sides of a hop; it never compares against the rollupJoin's own declared granularity. -
This is one flat
member.full_name() → columnmap for the whole pre-aggregation, but the column is now per-leg-rollup. In a multi-hop join where the same member participates in two hops and the two chosen leg rollups store it differently (one plainly, one truncated), the last hop iterated wins and the other hop's ON clause renders a column that side doesn't have. Previously every entry was the bare alias, so this couldn't diverge. A cheap guard would be to error on an inconsistent re-insert here rather than let it resolve to whichever hop came last.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #11747 +/- ##
===========================================
+ Coverage 59.08% 84.64% +25.56%
===========================================
Files 237 261 +24
Lines 19022 86220 +67198
Branches 3865 0 -3865
===========================================
+ Hits 11239 72982 +61743
- Misses 7222 13238 +6016
+ Partials 561 0 -561
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:
|
…d refuse what it can't tell apart A rollup can store the same dimension at several granularities, each in its own column. Taking the first match let declaration order decide which column a join reads, so an ambiguous key is now rejected by name. The comparability error named only the members, though each side's rollup was chosen on its own and that choice is never revisited; it now names the rollup on each side and how each stores the key, which is what tells the author which of the two to change. Members are read from one column per pre-aggregation, so a member shared by two hops that resolves to different columns would leave one ON clause naming a column its side doesn't have. Rejected up front. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @waralexrom's task in 2m 4s —— View job Re-review of
|
| Prior finding | Status |
|---|---|
4 — find_map let declaration order pick the granularity |
Fixed. resolve_join_member now collects all matching time dimensions and rejects len() > 1 by name, with a fixture (..._two_granularities.yaml) |
5 — JSDoc restated the filters |
Fixed. Trimmed to the load-bearing canUseNativeSqlPlannerPreAggregation reason, 3 lines |
| 3(2) — flat member→column map, last hop wins | Fixed. check_join_members_resolve_consistently rejects a member resolving to two columns across hops, before the plan is built |
| 2 — greedy per-side pick isn't backtracked | Addressed as asked — the comment at pre_aggregations_compiler.rs:528 now states it plainly. Resolved the thread; the limitation stands, documented |
| 1 — composite hop mixing a plain and a truncated key | Still open. The message is better (it names each rollup and how it stores each key) but the rule is unchanged, so a hop joining customer_id = id AND created_at = d is still rejected. Thread left unresolved |
3(1) — join pass overwrites the time_dimensions pass |
Still open. check_join_members_resolve_consistently compares hops against each other, never against the rollupJoin's own declared granularity, so a rollupJoin declaring d at month over legs storing day still renders d_day. Thread left unresolved |
The error-message rework is the strongest part of this commit. Naming both rollups and how each stores the key turns "these keys disagree" into "change one of these two rollups", which is the actual next action.
New finding
| # | Severity | Where | Issue |
|---|---|---|---|
| 6 | Medium | pre_aggregations_compiler.rs:685 |
The ambiguity error propagates out of the whole candidate scan, so one ambiguous rollup in rollups: fails the hop even when another candidate resolves cleanly. Deferring the error until widened is empty keeps both properties |
Testing
The new ..._two_granularities.yaml fixture and its test are right for the ambiguity rejection. Two gaps remain, both matching open findings: a two-candidate side where one is ambiguous (finding 6), and a composite hop mixing a plain and a truncated key (finding 1).
Not verified
I did not build or run the Rust or JS suites in this environment.
A rollup storing the join key at several granularities is no candidate for the hop, but failing the whole scan on it hides the rollups that can serve it — the list is walked in declaration order, so a usable rollup listed after it was never reached. Keep the reason instead and report it only when nothing else stands in either, where it is the actual cause rather than a generic rejection. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @waralexrom's task in 1m 21s —— View job Re-review of
|
| Finding | Why it's still open |
|---|---|
1 — a composite hop mixing a plain key with a truncated one is rejected (check_join_members_comparable) |
Rule unchanged; the message now names both rollups and how each stores each key, which is a real improvement, but customer_id = id AND created_at = d is still refused. Documented, not fixed |
3(1) — the join pass in pre_aggregation.rs:136-145 runs after and overwrites the time_dimensions pass |
check_join_members_resolve_consistently compares hops against each other only; a rollupJoin declaring d at month over legs storing day still renders d_day for the query's own time dimension |
Both are limitations rather than regressions (each of these shapes failed outright before the PR), and both are stated in code comments now. Nothing here I'd hold the PR on.
One small thing, not worth an inline thread
Which reason survives in ambiguous depends on member iteration order: the inner loop breaks on the first member that fails, so a rollup that lacks member A but stores B ambiguously records nothing, while the reverse order records the ambiguity. And a rollup can be reported as ambiguous when fixing its ambiguity wouldn't make it a candidate anyway (it's also missing another key). The message is still true about that rollup, just not necessarily the actionable cause — acceptable for a best-effort hint, which is what the comment claims it is.
Testing
The distinguishing case for finding 6 is now covered. The two remaining gaps line up with the two open threads: a composite hop mixing a plain and a truncated key, and a rollupJoin whose own declared granularity differs from the hop's.
Not verified
cargo check/cargo test and the JS suites were not run — the build commands aren't permitted in this environment, so the reported results (1328 planner tests, the Postgres integration file on both planners) are taken as stated.
2 participants
Summary
A
rollup_joinwhose join key is declared as a leg rollup'stime_dimension— rather than as a plain dimension — never resolved, at any chain length. Both the hop lookup and the ON-clause rendering assumed the key lives in a plainly named column, and a time dimension does not: a rollup stores it truncated to itsgranularity, under a granularity-suffixed name.The chain-depth limitation reported in #11362 does not exist. Four- and five-cube chains match on master on the same terms a three-cube chain does; the reported failure came from an interior rollup not declaring the key on its own side of the new hop. The four-cube test added here passes without any of the changes below, and is there to keep it that way.
Changes
PreAggregationJoinMember { symbol, column, granularity }). The ON clause now reads the column the rollups materialize, and the join-members pass no longer overwrites the query's own time dimension inall_dimensions_refererences.daycan be compared neither to one truncated tomonthnor to one kept raw as a plain dimension; both are rejected with an error naming the members and how each is stored. The two sides carry no pairing between their members, so the check is on the hop as a whole rather than side by side.Multiple rollups found.rollup_jointo fix, as the JS message already did. A bareNo rollups found that can be used for rollup joinis why this was filed as a depth bug.preAggregationForQuerydescriptor, and a throw there kills the whole native call even though the SQL is already correct. It is gated oncanUseNativeSqlPlannerPreAggregation: the legacy planner renders the ON clause from the bare alias, so widening it unconditionally would turn its clean rejection into SQL that fails at execution.Testing
rollup_join_keys.rs— six planner tests over four new fixtures: the four-cube chain, the hop an interior rollup has no key for, the time-dimension key, the two rejected storage mismatches, and the ambiguity guard. Each was red-checked by reverting the corresponding change.pre-aggregations.test.ts(unit) — the chain matching and assembling, the rejection naming the hop, and the granularity mismatch.pre-aggregations.test.ts(integration, Postgres) —rollupJoin pre-aggregation on a time dimension join keymaterializes both leg rollups and asserts the returned rows. Reverting the rendering fix and rebuilding the native addon makes it fail against the database withcolumn "td_dates__day" does not exist, which is the actual defect rather than a shape read off the SQL.pre-aggregationsunit; the Postgres pre-aggregation integration file on both planners (48 passed under Tesseract, 52 under the legacy planner).Note
The join happens on the stored buckets, not on the raw values it is modeled on, so it fits keys already aligned to that granularity — a date dimension joined to facts by day. The reference page is deliberately untouched here; the docs change already open against it is the place for that.
Refs #11362