fix(tesseract): report multi-stage reads of out-of-grain dimensions - #11514
Conversation
A multi-stage member whose SQL reads a dimension that is not part of the grain it is computed at emitted SQL referencing the dimension's own cube alias, which the CTE's FROM never brings into scope. Postgres rejects such a query with "missing FROM-clause entry"; nothing reported it at plan time. Report the member and the dimension where a multi-stage member's deps are decomposed into children, naming the `grain.include` entry that makes the model plan. Reachability mirrors reference resolution: a member the source exposes as a column stops the walk, anything else is reachable only if every member its own SQL reads is, and a dimension that also reads a raw cube column is unreachable regardless of what its member deps resolve to. Only the slots that reach rendered SQL are considered — `drill_filters` are never emitted and a `mask` only for masked members, so neither can put a column requirement on a CTE. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @waralexrom's task in 6m 49s —— View job Review complete — solid fix, one possible false-positive class to confirm. 1 medium, 2 low, 2 nits (5 inline comments)Full review
What's goodThe diagnosis is precise and the fix sits at the right place:
No security or resource concerns: the change is plan-time only, adds no query surface, and the error text interpolates member names already present in the compiled model. Findings
Finding 1 is the only one I'd want resolved before merge — either accept child-supplied columns, or add a test pinning the rejection so it's a decision rather than an accident. Everything else is documentation or polish. On the unverified Postgres runThe structural argument holds up — I traced |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| let deps = rendered_dependencies(dimension); | ||
| !deps.is_empty() | ||
| && !reads_raw_cube_column(dimension) | ||
| && deps.iter().all(|dep| { |
There was a problem hiding this comment.
dimension_is_reachable recurses over a DAG with no memo, and each level re-runs rendered_dependencies (which allocates a Vec per slot) plus reads_raw_cube_column (a second full walk of the same slots). On a diamond-shaped dimension graph — several derived dimensions sharing bases — this is exponential in depth rather than linear in nodes.
Depths are small in practice, so this is a nit, but a HashSet<String> of visited full_name()s threaded through the recursion would make it linear and guard against a self-referential chain that would otherwise blow the stack. Combining the two walks into one visit_rendered_slots pass (collect deps and cube refs together) also halves the work.
| return Ok(()); | ||
| } | ||
| Err(CubeError::user(format!( | ||
| "Multi-stage member {member} reads dimension {dimension}, which is not part of the \ |
There was a problem hiding this comment.
Since this is a hard CubeError::user on a path that previously produced SQL, the documented false-positive class ("a constant dimension is reported") is a behaviour break for models that work on 1.3.12+: sql: "'unknown'", sql: "CURRENT_DATE", or any dimension whose expression has no member deps and no cube refs is reported as unreachable by the !deps.is_empty() clause below, even though it renders fine with no FROM requirement.
The trade-off (catching a bare sql: category identifier) is defensible, but it's a query that used to return rows and now fails. Two things would help: a release note calling out the new error and the grain.include workaround, and — if it's cheap — a fixture pinning the constant-dimension case so the false positive is a recorded decision rather than a latent surprise.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #11514 +/- ##
==========================================
- Coverage 83.96% 79.47% -4.49%
==========================================
Files 257 480 +223
Lines 80919 98861 +17942
Branches 0 3636 +3636
==========================================
+ Hits 67940 78568 +10628
- Misses 12979 19773 +6794
- 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:
|
A mask reaches the rendered SQL for the members it is applied to, so a mask reading a dimension outside the stage grain emits the same dangling cube alias — for masked tenants only, which is where it is hardest to notice. Excluding `mask_sql` unconditionally left that case unreported. Thread the masking predicate through the reachability walk so the mask slot counts as rendered exactly when the member is masked. Also pin two behaviours that were decisions rather than accidents: a `grain.include` on a child widens that child's leaf and not the columns its CTE projects, so it does not satisfy a parent reading the dimension; and the CASE-SWITCH path plans branch dependencies as their own CTEs, which is why it skips the check. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @waralexrom's task in 4m 59s —— View job Re-review of
|
| # | Where | Issue |
|---|---|---|
| 1 | visit_rendered_slots (~1216) |
Mirror asymmetry: an applied mask with no mask.filter replaces the member's own render (masked.rs:102-104), yet the original slots are still required — so an unconditionally-masked member whose own sql reads an out-of-grain dimension is reported even though that read never emits. Fail-earlier rather than fail-wrongly, but it's the exact mirror of what this commit fixes. |
| 2 | dimension_deps.rs:347 |
The new mask test asserts only that a plan error occurs, which the guard produces by construction. Nothing in the suite pins the premise that a mask renders inside the multi-stage CTE (grep maskedMembers tests → only ungrouped_forms.rs and this file). A positive counterpart with the dimension in grain.include, asserting the mask renders against the CTE alias, would close it. |
Minor, not worth a thread: the is_masked closure is now written out twice — default_make_childs:346-350 and check_dimension_is_reachable:410-414. A single fn is_member_masked(&self, m: &Rc<MemberSymbol>) -> bool on the planner, passed as &|m| self.is_member_masked(m), keeps one definition.
Prior threads that still apply (not re-posted)
- Constant-dimension false positive / release note (
check_dimension_is_reachable, ~418). Still a hardCubeError::useron a path that produced SQL on 1.3.12+; no fixture pins the case and no release note accompanies the new error. - No memo on the DAG walk (~1189). Unchanged, and the added
is_maskedparameter adds a third pass over the same slots per node. Still a nit — depths are small — but aHashSetof visitedfull_name()s would also guard a self-referential chain.
Both left unresolved deliberately.
Not verified
No build or test run this round — this crate has no cached target/ on the runner and a cold cargo test -p cubesqlplanner on 4 cores wasn't worth the wall clock for a re-review of a 110-line commit. Findings above come from reading masked.rs, factory.rs, member_query_planner.rs and the planner itself. The author's reported 1203 passed plus CI cover the suite; finding 2 is precisely the gap a green suite can't speak to.
The plans the grain check lets through were asserted on substrings only, so nothing established that they run. Execute the six of them and pin the result sets: the declared-grain measure returns the first-half-of-month totals (420 / 650 / 850 against the seed), and the derived-dimension, query-grain, keys-side, drill-filter and mask shapes return their expected rows. The keys-side result carries the whole month against the `completed` row rather than the `completed` total. That follows from `reduce_by` collapsing the measure below the status grain, so the test says so — the number reads like a mistake otherwise. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @waralexrom's task in 3m 33s —— View job Re-review of
|
| Test | Pinned | Check against seed |
|---|---|---|
declared_grain_plans_and_reads_the_cte_column |
420 / 650 / 850 | day ≤ 15: Jan 100+200+120, Feb 300+200+150, Mar 400+200+250 ✅ (note day 15 is inside the boundary) |
dimension_in_the_query_grain |
completed 300 / 500 / 600 | monthly completed totals ✅ |
dimension_derived_from_a_grain_dimension |
books 230 / 150 / 500 | category_label = 'Literature' → books per month ✅ |
dimension_reachable_from_the_keys_side |
completed 500 / 750 / 1000 | whole-month totals — reduce_by collapsed below status ✅ |
dimension_read_only_by_drill_filters |
500 / 750 / 1000 | monthly totals, drill filter correctly not applied ✅ |
raw_column_read_only_by_a_mask |
all zeros | UPPER(status) = 'X' matches nothing — see nit 2 |
The keys-side number is the one a reader would flag as a bug, and the commit anticipates that: the doc comment at dimension_deps.rs:140-144 explains that reduce_by collapses the measure to a grain without status and the CASE then reads the broadcast row's status. Explaining a surprising-but-correct number in the test that pins it is the right place for that.
Worth noting what this run actually buys: try_execute_pg panics with the full SQL when the statement fails (test_context.rs:704-709), so under --features integration-postgres these six tests now fail loudly on a missing FROM-clause entry regression rather than passing a substring assertion. That is exactly the coverage the earlier rounds could only argue structurally.
New findings
| # | Severity | Where | Issue |
|---|---|---|---|
| 1 | Medium | dimension_deps.rs:222 (+3 more) |
Four snapshots pin a row order no ORDER BY determines — order: names one of two grouped dimensions, and the committed rows are visibly in hash-agg order (cancelled 02, 01, 03). format_simple_query_results never sorts. reduce_by.rs:60-62 already lists every grouped dimension for this reason. |
| 2 | Nit | dimension_deps.rs:371 |
The mask snapshot is twelve zeros — UPPER(status) = 'X' matches no seed row, so the pinned result set would be identical if the dimension rendered from the wrong column. 'COMPLETED' makes it discriminating. |
| 3 | Nit | dimension_deps.rs:301-313 |
month_query("amount_with_drill_filters") is built twice; every other test in the file hoists let query = … first, which is what the same commit did to test_declared_grain_plans_and_reads_the_cte_column. |
Also minor, not worth a thread: three passing shapes were left unexecuted — test_declared_grain_keeps_the_query_grain (asserts on plan text, fair enough), test_dimension_read_only_by_an_inactive_mask_is_not_reported and test_masked_multi_stage_time_dimension_is_not_reported. The inactive-mask pair is the false-positive class most worth executing, since "the mask is not rendered" is a claim about emitted SQL.
Finding 1 is the only one I'd act on before merge, and it's cheap: one extra order: key per query plus re-accepted snapshots.
Prior threads that still apply (not re-posted)
Unchanged by this commit — it touches only tests:
- Mask asymmetry: an applied mask with no
mask.filterreplaces the member's own render, yet the original slots are still required (multi_stage_query_planner.rs:1220). - No positive mask counterpart (
dimension_deps.rs:380). Still open, and this commit sharpens the point: the executed mask test is the inactive one, so nothing yet demonstrates that an applied mask renders inside the CTE — the premise the guard's mask branch rests on. - Constant-dimension false positive / release note (
~418). - No memo on the DAG walk (
~1189).
Not verified
No test run — the crate has no cached target/ here and the execution paths are behind --features integration-postgres with a live Postgres, which this runner doesn't have; that's the CI job's business. The result-set verification above is arithmetic against the seed file, and the ordering finding is read directly off the committed snapshots.
Row order: four of the executed snapshots pinned an order no ORDER BY determines — the queries group by a dimension and the month while ordering by the dimension alone, so the committed rows were hash-aggregate output. Name every grouped dimension in `order:`, as the neighbouring suite does. Mask coverage: nothing established the premise the mask branch rests on — that an applied mask is rendered inside the multi-stage CTE. It is, and the new test pins it against the CTE column. Writing that test turned up a limitation worth recording: an unconditional mask replaces the member's aggregate, so its dimension read sits outside any aggregate and has to be in the stage's own GROUP BY. A declared leaf grain puts the column in the source but not in that GROUP BY, and the database rejects the result. The reachability check asks only whether a column exists, so it does not tell the two apart — noted on the test and on the mask branch. Also compare the masked-dimension measure against a literal the seed matches, so its result set discriminates instead of being twelve zeros. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @waralexrom's task in 2m 32s —— View job Re-review of
|
| # | Where | Issue |
|---|---|---|
| 1 | integration_multi_stage.yaml:486 |
Nit: grain.include: [orders.category] on the new fixture member is inert — the test groups by category in the query — while the member name and fixture comment point at the mechanism the test's own doc comment says is insufficient. Drop it, or note it's redundant. |
Also minor, not worth a thread: test_dimension_read_only_by_an_inactive_mask_is_not_reported is still the one false-positive shape left unexecuted, and "the mask is not rendered" is a claim about emitted SQL. Cheap to add now that the seed and helpers are in place.
Prior threads that still apply (not re-posted)
Untouched by this commit:
- Constant-dimension false positive / release note (
multi_stage_query_planner.rs:418) — still a hardCubeError::useron a path that produced SQL on 1.3.12+, with no fixture pinning the case and no release note. - No memo on the DAG walk (
~1189) — nit; aHashSetof visitedfull_name()s would also guard a self-referential chain.
Resolved as addressed: mask asymmetry, missing positive mask counterpart, snapshot row order, twelve-zero mask snapshot.
Not verified
No test run. The execution paths are behind --features integration-postgres with a live Postgres, which this runner doesn't have, and the crate has no cached target/ here. Everything above is read off the diff and the seed: the snapshot ordering claim is direct observation of the committed rows, the result sets are arithmetic against integration_multi_stage_tables.sql, and the mask-replacement behaviour is masked.rs:102-104 plus the -1 the new snapshot actually contains.
| grain: | ||
| include: | ||
| - orders.category |
There was a problem hiding this comment.
Nit: this grain.include is inert for the test that uses it, and the member name (…_in_grain) plus the comment above ("with the dimension the mask reads declared") point a reader at the mechanism the test's own doc comment says would not be sufficient.
test_active_mask_reading_a_grouped_dimension_plans puts orders.category in the query's dimensions, so the column is in parent_state and in every stage's GROUP BY regardless of this declaration — dropping the three lines leaves the test passing and the snapshot unchanged. The comment in the test is explicit that a declared leaf grain alone puts the column in the source but not in the stage's GROUP BY, so keeping the declaration here is the one thing likely to make the next reader conclude the opposite.
Either drop it and rename to something like amount_masked_by_grouped_category, or keep it and say on the fixture comment that it is redundant with the query grain and does not carry the case. Not blocking — the assertions and the result set are both right about what they check.
2 participants
Summary
A multi-stage member whose SQL reads a dimension that is not part of the grain it is computed at emitted SQL referencing the dimension's own cube alias — a table nothing in the CTE's
FROMbrings into scope. Postgres rejects it withmissing FROM-clause entry, and nothing reported it at plan time. This PR reports the member and the dimension instead, naming the declaration that fixes the model.This is a regression. Through v1.3.11 the planner raised
member X has no source;1deddcc4e3(#9434, pre-aggregations) commented out that guard, and v1.3.12 onward produced broken SQL silently. Restoring the original guard is not an option — it recognises onlySingleSource::Cube, so pre-aggregation sources fail it (verified: 607 of 1191 tests break on ordinary queries).Broken output, Postgres dialect:
Now:
Declaring
grain.includeis both the fix and a workaround that already works on released versions — no upgrade needed to unblock a model.Changes
default_make_childs, where a multi-stage member's deps are decomposed into children.CubeError::user— this is a model error, not a planner failure.visit_rendered_slotsis the single slot list both the dependency walk and the cube-ref test read, so the two cannot drift.drill_filtersare never emitted, so they never constrain the CTE; amaskis emitted only for the members it is applied to, so it counts for those and is excluded otherwise.reduce_bydrops still reaches the CTE from the keys side, and is not reported.dimension_deps.rs: reported / plans withgrain.include/ not reported falsely (derived dimensions,drill_filters, inactive masks, keys side, query grain). Seven of them execute against Postgres and pin the result sets.Testing
cargo test -p cubesqlplanner --features integration-postgrescargo test -p cubesqlplannercargo clippy --all-targets --features integration-postgresExecuted against Postgres. Seven plans the guard lets through now run on a live database, and every pinned result set was recomputed by hand from
integration_multi_stage_tables.sql— e.g. the declared-grain measure returns 420 / 650 / 850 for day ≤ 15 (Jan 100+200+120, Feb 300+200+150, Mar 400+200+250).try_execute_pgpanics with the full SQL on a failed statement, so these tests now fail loudly on amissing FROM-clause entryregression instead of passing a substring assertion.Every behavioural change was checked against a negative control: reverting it individually makes the corresponding test fail with the expected broken SQL. Snapshot row order is pinned by naming every grouped dimension in
order:, so the result sets do not depend on hash-aggregate output order.Semantics were taken from the legacy planner rather than invented — it materialises the same dimension into the leaf grain, so the intended shape is not in doubt.
Reviewed over five adversarial rounds; each round found a false-positive class that would have rejected working models (derived dimensions,
drill_filters/maskslots, expressions mixing a member with a raw column, unfiltered cube refs). All closed and pinned by tests.Known limitations
Documented in the code, and worth a release note for the first one:
sql: categoryandsql: "'x'"are indistinguishable — neither carries a cube ref — so a constant dimension read by a multi-stage member now needs agrain.includeentry it did not need before. A model that worked for years starts failing; declaring the dimension resolves it.UPPER({CUBE.status}) || categorycarries no cube ref either, so it plans and fails at the database. Same ambiguity, opposite direction.GROUP BY. A declared leaf grain puts the column in the source but not in thatGROUP BY. The check asks only whether a column exists and does not tell the two apart, so that shape plans and the database rejects it. Pinned and explained ontest_active_mask_reading_a_grouped_dimension_plans.rolling_windowmeasures are not covered — they take a different planning path (try_plan_rolling_window) and keep their pre-existing defect, where the dimension lands outside both the aggregate and theGROUP BY.🤖 Generated with Claude Code