fix(tesseract): Resolve join for hint-less member expressions on views - #11501
fix(tesseract): Resolve join for hint-less member expressions on views#11501MazterQyou wants to merge 1 commit into
Conversation
|
Claude finished @MazterQyou's task in 4m 5s —— View job Review: approach is sound — 2 medium, 3 low (5 inline comments)Full review
VerdictThe diagnosis is right and the fix is in the right place. The old I also checked the Verified the new snapshot against the seed data: Findings
Security / performanceNo security surface here — no user input reaches SQL text through this path; the change only reorders which cube hints seed join resolution. Performance impact is confined to finding 1 (avoidable DocsNothing user-facing changed, so no What I could not verifyI did not compile or run the tests: this is a fresh checkout with no Cargo build cache for the |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #11501 +/- ##
==========================================
- Coverage 83.96% 79.46% -4.50%
==========================================
Files 257 480 +223
Lines 80919 98861 +17942
Branches 0 3636 +3636
==========================================
+ Hits 67940 78563 +10623
- Misses 12979 19778 +6799
- Partials 0 520 +520
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:
|
cbc785f to
55a179d
Compare
|
Claude finished @MazterQyou's task in 5m 2s —— View job Re-review: all 5 prior findings addressed — 1 medium, 3 low (4 new inline comments)Full review
Prior findings — all addressed, threads resolved
Snapshot arithmetic — checked against
|
| # | Sev | Where | Issue |
|---|---|---|---|
| 1 | Medium | multi_fact_join_groups.rs:307-316 |
The join-map last resort only fires for multi-hop views (joinMap is filtered to len > 1), so COUNT(*) alone still hard-errors on a single-cube view like orders_view — the commonest view shape, and precisely the BI-profiling case the PR body cites |
| 2 | Low | multi_fact_join_groups.rs:309-313 |
Root taken from the first joinMap path → depends on view cube ordering; and COUNT(*) on customer_overview means 4 alone vs 13 alongside other measures. Parity with legacy, but should be documented |
| 3 | Low | mock_schema.rs:648-661 |
Populating join_map changes hint collection for every existing view fixture (Single(x) → Vector([x]), a distinct join_tree_cache key). Only 3 assertions needed updating, which is plausible — please confirm the full suite is green |
| 4 | Low | multi_fact_join_groups.rs:112-120 |
JoinHints::push dedups only adjacent Singles, so identical Vectors still repeat in pooled_hints — pre-existing, but pooled_hints is now a cache key of its own |
Finding 1 is the only one I'd want a decision on before merge, and "out of scope, follow-up filed" is a perfectly good answer — the current state is strictly better than the invalid type: unit value, expected struct JoinDefinitionStatic failure either way. The rest are comment/robustness polish.
Code quality
The from_base_hints rewrite is a genuine improvement over the previous filter-then-map: one pass, and the "multi-stage measures get no entry but do count towards the pool" rule is now stated where it happens rather than implied by a filtered_measures vector two blocks up. The inherited flag reads slightly better than a is_some() re-check would, though match inherited_pooled_hints { Some(p) => …, None => … } would make the two modes structurally exclusive rather than coupled through a bool.
The error message is specific and actionable. It interpolates cube_name twice (as the referenced cube and as the join-map owner) which reads a little repetitively for the common case where they're the same string, but it is correct.
Security / performance
No security surface: nothing user-controlled reaches SQL text through this path, and the change only reorders which cube hints seed join resolution. Performance is a wash — collect_join_hints is now also called for multi-stage measures (one extra traversal each, previously skipped), offset by no longer cloning base_hints into the pool per measure. pooled_hints adds one JoinHints clone per MeasuresJoinHints.
Testing
Four new tests covering the three fallback branches plus the terminal error, which is the right decomposition. Two gaps: the positive assertions only fire when a Postgres is reachable (try_execute_pg_from_options returns Option), so in environments without one those three tests degrade to "the planner didn't panic"; and there's no case for a hint-less view ME alongside only multi-stage measures, which is the path the reworked pooling in from_base_hints specifically enables.
Docs
Nothing user-facing changed, so no /docs-mintlify update is needed. The doc comments carry real weight here — they explain the legacy-parity reasoning rather than restating the code, and the per-test comments state the expected numbers and why, which is what makes the 13 in the multi-fact snapshot reviewable at all.
What I could not verify
I could not build or run the tests — cargo check on the cubesqlplanner crate isn't permitted in this job's sandbox, and this is a cold checkout with no build cache. Every finding above is from static reading of the diff, the fixtures, the seed SQL and the JS schema compiler; the snapshot arithmetic is hand-computed. Finding 3 in particular is a request for confirmation, not a claimed regression.
| // Like the schema compiler, only multi-hop join paths land in the join | ||
| // map: a direct cube needs no path to be reached. | ||
| let join_map = self | ||
| .view_cubes | ||
| .iter() | ||
| .map(|view_cube| { | ||
| view_cube | ||
| .join_path | ||
| .split('.') | ||
| .map(|part| part.to_string()) | ||
| .collect::<Vec<_>>() | ||
| }) | ||
| .filter(|path| path.len() > 1) | ||
| .collect::<Vec<_>>(); |
There was a problem hiding this comment.
Good fidelity fix — the mock previously left join_map as None, so no view test exercised the enrichment in collect_join_hints (join_hints_collector.rs:106-123) at all.
Worth calling out that this is a behaviour change for every existing view fixture, not just the new tests: with a join_map present, a root-cube member's hint is now rewritten from Single("cube_a") to Vector(["cube_a"]) (path[0..=0]). The three assertion updates in tests/join_hints_collector.rs are the visible part, but JoinHints is the join_tree_cache key (Hash/Eq) and the value handed to join_for_hints, so Single(x) and Vector([x]) are distinct keys everywhere. That's a wide blast radius for a fixture tweak — could you confirm the full cubesqlplanner suite (including the SQL snapshot tests over integration_views.yaml / orders_with_customers) is green with no regenerated snapshots? I wasn't able to build the crate in this job to check myself.
Minor fidelity nit: production derives the path via evaluateReferences(null, it.joinPath, { collectJoinHints: true }) and pushes one entry per cube in includedCubes while processing dimensions only (CubeSymbols.ts:817-825), whereas this splits the raw join_path over every view_cube. Equivalent for these fixtures; a divergence to keep in mind if a fixture ever gets a cube with no included dimensions.
Signed-off-by: Alex Qyoun-ae <4062971+MazterQyou@users.noreply.github.com>
55a179d to
8796a72
Compare
|
Claude finished @MazterQyou's task in 4m 36s —— View job Re-review: 3 of 4 prior findings addressed — 0 medium, 2 low (2 new inline comments)Full review
Prior findings
What I re-verified
New findings
Neither blocks. Finding 1 is about a doc claim over-promising an invariant one constructor can break; finding 2 is a coverage gap plus a mechanical reorder. Code qualityThe The ambiguous-root rejection is the right call over picking a root: the alternative is a silent result change on a YAML reordering with no diagnostic. The error text names the actual cause. The doc block on TestingFive tests now, covering all three fallback branches plus both terminal errors, which is the right decomposition. Remaining gaps: the multi-stage-only pooling path (finding 2), and the three positive assertions only fire when a Postgres is reachable ( Security / performanceNo security surface — nothing user-controlled reaches SQL text through this path; the change only decides which cube hints seed join resolution. Performance is roughly neutral: DocsNothing user-facing changed, so no What I could not verifyI could not build or run anything: there is no Cargo target dir in this checkout and a cold build of the Rust workspace exceeds this job's budget; · branch |
| /// Keeping the list free of adjacent repeats keeps it canonical, which | ||
| /// matters because `JoinHints` is used as a join tree cache key. |
There was a problem hiding this comment.
The verbatim-repeat rule closes the Vector gap from the last round, and the test pins down that only adjacent repeats go. Two things keep the "canonical, because it's a cache key" claim weaker than it reads, both worth a word in this comment (or a follow-up):
-
from_itemsbypassespushentirely (line 21-23), and it is the exit path of the main producer —collect_join_hintsrebuilds the list withJoinHints::from_items(collected_hints)after the join-map enrichment (join_hints_collector.rs:125). So a hint list can leave the collector in a formpushwould never have produced. -
Prefix subsumption isn't handled. For a view with join map
[customers, orders], a measure touching both cubes collects[Single(customers), Single(orders)], which enrichment rewrites to[Vector([customers]), Vector([customers, orders])]. The first item is a strict prefix of the second — semantically nothing, but a distinctHash/Eqfrom the minimal[Vector([customers, orders])], so it's a separatejoin_tree_cacheentry and a separateJoinKey(hence potentially a separate join group).
Neither is introduced here, and (2) is now more reachable than before this PR because the mock — and any real view — gives root-cube members a Vector form. Not a blocker; the doc comment just shouldn't promise an invariant from_items can break.
| for m in measures { | ||
| if !has_multi_stage_members(m, true)? { | ||
| filtered_measures.push(m.clone()); | ||
| let own_hints = collect_join_hints(m)?; | ||
| // Pool the incremental hints only: `base_hints` is already in there, | ||
| // and re-appending it per measure would make the hint list - and with | ||
| // it the join tree cache key - non-canonical. | ||
| if !inherited { | ||
| pooled_hints.extend(&own_hints); | ||
| } | ||
| // Multi-stage measures plan their joins separately, so they get no | ||
| // entry of their own - but their hints still count towards the pool. | ||
| if has_multi_stage_members(m, true)? { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Pooling multi-stage measures while still giving them no measure_hints entry is exactly the right resolution of the previous wording problem — the message can now honestly claim nothing in the query offers a cube.
Two follow-ups on this loop:
Still untested. This is the branch that makes COUNT(*) on a view resolvable when the only other members are multi-stage, and nothing in the suite exercises it. All four new tests use plain measures or nothing at all, so the continue-after-pooling behaviour is load-bearing but unverified. A case with a hint-less view ME plus a single rolling-window / multi-stage measure on the same view would cover it, and would also pin down which tree the ME lands in — note it's a tree assembled from a measure that is planned in its own subquery, which is a semantics worth locking into a snapshot rather than leaving implicit.
Wasted traversal in the inherited path. When inherited is true, own_hints is unused for multi-stage measures, yet collect_join_hints(m) still runs a full symbol traversal before the continue. for_measures is called per multiplied-measure bucket and per measure in query_properties.rs:590, so this repeats. Cheap reorder:
| for m in measures { | |
| if !has_multi_stage_members(m, true)? { | |
| filtered_measures.push(m.clone()); | |
| let own_hints = collect_join_hints(m)?; | |
| // Pool the incremental hints only: `base_hints` is already in there, | |
| // and re-appending it per measure would make the hint list - and with | |
| // it the join tree cache key - non-canonical. | |
| if !inherited { | |
| pooled_hints.extend(&own_hints); | |
| } | |
| // Multi-stage measures plan their joins separately, so they get no | |
| // entry of their own - but their hints still count towards the pool. | |
| if has_multi_stage_members(m, true)? { | |
| continue; | |
| } | |
| for m in measures { | |
| let is_multi_stage = has_multi_stage_members(m, true)?; | |
| // Multi-stage measures plan their joins separately, so they get no | |
| // entry of their own - but their hints still count towards the pool, | |
| // which is already filled in when it is inherited. | |
| if inherited && is_multi_stage { | |
| continue; | |
| } | |
| let own_hints = collect_join_hints(m)?; | |
| // Pool the incremental hints only: `base_hints` is already in there, | |
| // and re-appending it per measure would make the hint list - and with | |
| // it the join tree cache key - non-canonical. | |
| if !inherited { | |
| pooled_hints.extend(&own_hints); | |
| } | |
| if is_multi_stage { | |
| continue; | |
| } |
2 participants
Check List
Description of Changes Made
This PR fixes the
invalid type: unit value, expected struct JoinDefinitionStaticerror for queries whose member expressions on a view carry no join hints of their own, such ascount(*)alongsidecount(distinct <view dimension>). Related tests are included.