refactor(tesseract): unify MemberSymbol dependency traversal behind a single declaration - #11409
Conversation
|
Claude finished @waralexrom's task in 5m 0s —— View job Reviewed — solid refactor, but one behavioral regression to resolve before merge: 1 high, 1 medium, 4 lowFull review
GeneralThis is a genuinely good consolidation. Six parallel traversals collapsing into one declaration per node is the right shape, the full-destructuring-without- The new test file is well aimed — I was not able to run Findings
1 — the one that mattersThe PR justifies
The rest of the multi-stage reasoning is correct; only On the macro's safety guaranteeWorth being precise about what the full destructuring buys: it forces a new field to be classified, but nothing stops a real dependency from being classified Security / performanceNo security surface here — no input parsing, no auth, no SQL string construction changes. Performance is neutral-to-positive on the read side (no more intermediate |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| //! A *dependency* of a symbol is another member symbol (or a cube | ||
| //! reference) whose rendered SQL becomes part of that symbol's SQL. | ||
| //! Symbol references that only name members without rendering them — | ||
| //! multi-stage grain / filter member lists, time-shift targets — are | ||
| //! annotations, not dependencies: they are matched by `full_name` and | ||
| //! are invisible to the traversals defined here. |
There was a problem hiding this comment.
This module doc is the load-bearing justification for multi_stage: skip, and it overstates the case: grain.include entries are rendered (pushed into CTE state dimensions / join dimensions), so they don't fit the "matched by full_name" definition. grain.exclude, grain.keep_only, filter.exclude, filter.keep_only and time-shift targets do. Worth narrowing the sentence to name those specifically rather than "grain / filter member lists" — otherwise the next person reading this doc will classify a new list as an annotation on the same reasoning.
| fn visit_deps_mut(&mut self, visitor: &mut dyn DepVisitorMut) -> Result<(), CubeError> { | ||
| match self { | ||
| Self::SqlCall(sql_call) => sql_call.visit_deps_mut(visitor), | ||
| Self::PatchedSymbol(symbol) => visitor.symbol(symbol), |
There was a problem hiding this comment.
Behavior change worth calling out explicitly in the PR body (and covering with a test): the old MemberExpressionSymbol::apply_to_deps did *member_symbol = f(member_symbol)? — f only, no recursion into the patched symbol's own dependencies. Now visitor.symbol(symbol) routes through ApplyVisitor → apply_recursive, so f also descends into the patched symbol's subtree.
Same delta for TimeDimensionSymbol::base_symbol (old: result.base_symbol = f(&self.base_symbol)?). The new semantics look like the correct ones — a static filter or multiplied-count rewrite previously stopped one level short for PatchedSymbol / time-dimension bases — but that makes this PR a behavior fix, not a pure refactor, and the SQL of affected models can change. transform_visits_base_node_once pins the TimeDimension side; there's no equivalent test for PatchedSymbol, which would be cheap to add alongside it.
| fn visit_deps_mut(&mut self, visitor: &mut dyn DepVisitorMut) -> Result<(), CubeError> { | ||
| let mut call = (**self).clone(); | ||
| for dep in call.deps.iter_mut() { | ||
| if let SqlDependency::Symbol(s) = dep { | ||
| visitor.symbol(s)?; | ||
| } | ||
| } | ||
| Ok(Rc::new(result)) | ||
| *self = Rc::new(call); |
There was a problem hiding this comment.
Optional (not a regression — the old apply_recursive did the same): this deep-copies the SqlCall and reallocates the Rc on every traversal, including for calls that have no Symbol deps at all. Since apply_to_deps now runs over the whole tree for every node, an identity transform rebuilds every SqlCall in the graph and destroys all Rc sharing. A cheap early-out keeps the common case allocation-free:
fn visit_deps_mut(&mut self, visitor: &mut dyn DepVisitorMut) -> Result<(), CubeError> {
if !self.deps.iter().any(|d| matches!(d, SqlDependency::Symbol(_))) {
return Ok(());
}
let mut call = (**self).clone();
...
}Same idea would apply to MemberSymbol::visit_deps_mut, though there the clone is harder to avoid.
| pub trait DepVisitor { | ||
| fn symbol(&mut self, symbol: &Rc<MemberSymbol>) -> ControlFlow<()>; | ||
|
|
||
| fn cube_ref(&mut self, _cube_ref: &CubeRef) -> ControlFlow<()> { | ||
| ControlFlow::Continue(()) | ||
| } | ||
| } |
There was a problem hiding this comment.
Nit: the ControlFlow<()> return is currently dead generality — neither collect_deps nor collect_cube_refs ever returns Break, and both discard the outer result (let _ = node.visit_deps(...)). It costs a ? on every generated arm and an ops::ControlFlow import in every node file. If short-circuiting isn't on the roadmap, fn symbol(&mut self, symbol: &Rc<MemberSymbol>) returning () would make the macro arms and the enum impls noticeably plainer; if it is (e.g. a future find_first_dep), a one-line comment saying so would keep the next reader from "simplifying" it away.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #11409 +/- ##
==========================================
- Coverage 83.94% 79.43% -4.52%
==========================================
Files 257 480 +223
Lines 80887 98778 +17891
Branches 0 3636 +3636
==========================================
+ Hits 67904 78460 +10556
- Misses 12983 19800 +6817
- Partials 0 518 +518
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:
|
|
Claude finished @waralexrom's task in 4m 5s —— View job Re-reviewed after
|
| # | Severity | Where | Status |
|---|---|---|---|
| 3 | Low | deps.rs:8 |
Still applies, with a shifted rationale: the definition ("SQL becomes part of that symbol's SQL") is right, but "matched by full_name" isn't true of grain.include — it's re-resolved at the leaf's QueryProperties::build(). One sentence pointing there would save the next reader the trace I just did. |
| 4 | Low | member_expression_symbol.rs:35 |
Still applies — PatchedSymbol transform now recurses where it previously applied f only; untested. |
| 5 | Low | sql_call.rs:553 |
Still applies — optional Rc-sharing early-out. |
| 6 | Low | deps.rs:35 |
Still applies — dead ControlFlow generality. |
Build/test
I could not run cargo test here — the sandbox declined the build command, same as on the previous pass. Everything above is from reading the code and tracing call sites. Please confirm cargo test -p cubesqlplanner locally/CI, in particular that the four new snapshots were generated against a live postgres rather than skipped.
Verdict
With the mechanism confirmed, multi_stage: skip in the symbol_deps! declarations is not just safe but more correct than the code it replaces, and the module doc's core definition holds. The remaining items are documentation, one untested behavior delta, and two nits — nothing blocking.
… single declaration Each symbol node now declares its dependency slots exactly once — via the symbol_deps! macro for structs (full destructuring, so a new field fails to compile until classified) or a hand-written SymbolDeps impl for enums — and both the read side (collect_deps, collect_cube_refs) and the rebuild side (apply_recursive, apply_to_deps) are derived from that declaration by generic algorithms in planner/symbols/deps.rs. This replaces the per-node hand-written get_dependencies / extract_symbol_deps / get_cube_refs / extract_cube_refs / apply_to_deps bodies, which enumerated the same slots independently. Semantic points: - A dependency is a symbol or cube ref whose rendered SQL enters the node's SQL. Multi-stage grain/filter member lists are annotations matched by full_name, not dependencies: apply_to_deps no longer rewrites them. - TimeDimensionSymbol::base_symbol is a transparent slot: reads emit the base's dependencies, transforms receive the slot itself. - Transform semantics are uniform: f is applied to every symbol node exactly once, recursion is handled by the algorithm. - Slot order is a contract (declaration order); reference symbols resolve to their first dependency. - iter_sql_calls/validate, owned_by_cube and DebugSql stay bespoke.
The base symbol is a granularity view, not a dependency: reads look through it (emitting the base's own dependencies), while transforms receive it as a replaceable slot and the wrapper keeps its identity.
…ge include grain Include grain refs are selected inside the multi-stage CTEs; case pruning and calc-group value resolution follow each leaf's own filters (the leaf rebuilds its QueryProperties, re-applying static filters), so a leaf whose filter directive excludes the switch filter keeps the full CASE while a filtered leaf renders the pruned branch.
8a1b7c7 to
6369eeb
Compare
|
Claude finished @waralexrom's task in 1m 49s —— View job Re-checked after the latest push — rebase only, no source or test changes. Review state unchanged: 4 low, 1 coverage nit, nothing blocking.Full review
What this push wasThe
I re-grepped for orphaned traversals: the only remaining Standing verdict (unchanged from the
|
| # | Severity | Where | Status |
|---|---|---|---|
| 3 | Low | deps.rs:8 |
Module doc still says grain/filter member lists are "matched by full_name". True of grain.exclude / keep_only and filter.exclude / keep_only; grain.include is instead re-resolved at the leaf's QueryProperties::build(). One sentence pointing there saves the next reader the trace. |
| 4 | Low | member_expression_symbol.rs:35 |
Still applies. Self::PatchedSymbol(symbol) => visitor.symbol(symbol) in visit_deps_mut routes through ApplyVisitor → apply_recursive, where the old code applied f only. Correct semantics, but a behavior delta with no PatchedSymbol test (transform_visits_base_node_once pins only the TimeDimension side). |
| 5 | Low | sql_call.rs:546 |
Still applies. visit_deps_mut does (**self).clone() unconditionally, including for calls with zero SqlDependency::Symbol deps — an identity transform rebuilds every SqlCall in the graph and drops Rc sharing. Early-out suggested in the thread. |
| 6 | Low | deps.rs:35 |
Still applies. Both collect_deps and collect_cube_refs do let _ = node.visit_deps(...) and neither ever returns Break, so ControlFlow<()> costs a ? per generated arm for no current benefit. Either drop it or leave a one-line note that short-circuiting is planned. |
Coverage nit (also unchanged)
MultiStageDimensionCalculation::join_dimensions is still the one grain.include consumer that reads the metadata symbol directly with no intervening QueryProperties::build(). I traced it again and still don't think it can produce wrong SQL — apply_static_filter_to_symbol only prunes case bodies, pruning is value-preserving on the rows the authorizing filter keeps, and calc groups take the separate is_calc_group branch. But all four tests in include_switch.rs use multi-stage measures (confirmed: every test block is measures: + optional dimensions:; none declares a multi_stage: true dimension). A fifth test — a multi-stage dimension with add_group_by: [orders.status_case_label] under a matching filter — would lock that reasoning down cheaply, since the fixture already exists.
Build
Same limitation as the two prior passes: the sandbox declined cargo check / cargo test, so everything above is from reading code and tracing call sites. Please confirm CI is green post-rebase — in particular that the four include_switch snapshots were generated against a live postgres rather than skipped, since try_execute_pg availability is what gates them.
Verdict
Nothing changed that would change the assessment. The refactor is sound, multi_stage: skip is more correct than the code it replaces, and the four open items are documentation, one untested behavior delta, and two nits. Approved by @ovr already; fine to merge with or without the low-severity cleanups.
2 participants
Summary
Unifies how
MemberSymboldependencies are enumerated and transformed in Tesseract. Previously every symbol node hand-implemented up to six parallel traversals (get_dependencies,extract_symbol_deps,get_cube_refs,extract_cube_refs,apply_to_deps,SqlCall::apply_recursive) that each enumerated the same child slots independently — and had already drifted apart. Now each node declares its dependency slots exactly once, and both the read side and the rebuild side are derived from that single declaration, so they can never disagree.Changes
planner/symbols/deps.rs:SymbolDepstrait (visit_deps/visit_deps_mut),DepVisitor/DepVisitorMutsinks, and generic algorithms (collect_deps,collect_cube_refs,apply_recursive,apply_to_deps) written once.symbol_deps!macro for struct nodes: one field list with modesdep/dep_symbol/dep_transparent/skip, generating both visit methods. Full destructuring without..makes adding a struct field a compile error until the field is classified. Enum nodes implement the trait with exhaustivematches.grain/filtermember lists are annotations matched byfull_name, not dependencies —apply_to_depsno longer rewrites them (MultiStageProperties::apply_to_depsdeleted).TimeDimensionSymbol::base_symbolis adep_transparentslot: reads emit the base's dependencies (never the base itself), transforms receive the slot — codifying the semantics every collector already assumed.fis applied to every node exactly once, recursion is handled by the algorithm.iter_sql_calls/validate,owned_by_cubeandDebugSqlintentionally stay bespoke.Testing
cargo test -p cubesqlplanner: 1083 passed.CUBEJS_TESSERACT_SQL_PLANNER=true: 43/43 suites, 496 tests passed.cargo check --workspaceclean; slot order and composition verified against the removed code node-by-node.🤖 Generated with Claude Code