refactor(tesseract): render member references as symbols

[waralexrom]

Summary

A member that a select reads from one of its sources was resolved while rendering, by looking its name up in a per-query map. The map was built bottom-up from the select's projections and applied to everything rendered beneath it, so a member it failed to record silently rendered its own expression against a source that may not carry the columns for it.

This replaces that map with a symbol. MemberSymbol::ColumnRef is a leaf that renders as the column or literal it names, carrying the member it stands for as an identity annotation rather than a dependency, so it keeps the member's name and alias and can hold any position the original held — column resolution, member-position lookup and filter matching keep working on it.

Changes

• Introduce MemberSymbol::ColumnRef. RootSqlNode dispatches it to a bare evaluate node of its own, so masking, granularity truncation and time shifts never see one: a reference describes no calculation, and nothing may wrap it.
• Express whatever still applies to the member as a symbol standing above the reference instead. A measure reading its row-level input from a source below becomes a measure whose kind aggregates a direct-reference SqlCall over the reference (measure_over_reference), which removes ungrouped_measure_references entirely.
• Apply substitution per select to its whole symbol environment — schema, filters, group by, order by, window partitions — as the last transform, after render modifiers are stamped.
• Substitute inside a granularity wrapper rather than replacing it: a filter renders the dimension behind the wrapper, so replacing it would compare a date range against a truncated column.
• Treat a reference as terminal during substitution, in both the collector and the render-reference map. Substituting one again would rebuild a wrapper around its own reference without end.
• Keep render_references for one case only: the SQL of a join condition is built while the FROM is still being assembled, before there is a select symbol environment to rewrite. Its call sites go from six to three, all of them join conditions.
• make_order_by takes the select's substitutions, so an ORDER BY item absent from the schema — a measure the query filters on but does not select — is rewritten for all three callers rather than only the top-level query.
• Two measure lists skip members that are not measures instead of erroring on them: a member expression built by the SQL API sits in the same list and renders its own SQL.

Resolution itself stays bottom-up. Moving it into the logical plan is separate work.

Testing

cargo test -p cubesqlplanner --features integration-postgres — 1381 tests pass, including the integration tests that execute their generated SQL against a real Postgres and snapshot the resulting rows.

New coverage for each mechanism the refactor rests on: substitution stopping at a reference (bare and nested), the render-reference map declining to intercept one, find_member_positions reaching a time dimension behind a reference, an ORDER BY on a filtered measure outside the selection, a pinned calc-group value in a multi-stage dimension join, and the sub-query dimension join of a multiplied measure.

Not in scope: a calc group pinned to a single value renders that value into the join condition and is covered here. A calc group with several values cross-joins its values table after the join that references it, so that reference is out of scope whichever way it renders; this does not run on master either and needs the FROM ordering fixed separately.

Check List

• Tests have been run in packages where changes have been made if available
• Linter has been run for changed code
• Tests for the changes have been added if not covered yet
• Docs have been added / updated if required (no user-facing change)

🤖 Generated with Claude Code

[claude]

Claude finished @waralexrom's task in 9m 16s —— View job

---

Review: looks sound — no correctness findings; 2 low (stale rationale comment, discarded work on the pre-agg path)

Full review

Todos

• Gather context (commits, full diff, changed files read in place)
• Review ColumnRef symbol mechanics (identity, deps, is_reference, equality)
• Review substitution (substitute_by_name, filter variant, termination)
• Trace every removed render_references call site against the new substitution
• Review the physical-plan builder processors
• Review tests
• Post findings

What I verified

I traced each of the six former resolve_references_for_* sites and checked the new path renders the same thing:

• Layer equivalence for measure_over_reference. On master ungrouped_measure_references sat above CaseSqlNode/MeasureFilterSqlNode and below final_measure_node_processor/MaskedSqlNode/MultiStageWindowNode. measure_over_reference drops exactly case + measure_filters + measure_drill_filters and keeps kind, mask, measure_order_by, multi-stage/rolling context and render_modifier — the same cut. MeasureKind::over_input_sql preserves aggregate_wrap for every kind (aggregate_wrap doesn't branch on CountSql), so count/MultipliedCount/Aggregated wrap identically, and Calculated renders member_sql directly, which is what the old interception produced. Rank ignoring its input matches master, where the rank wrap never consulted the intercepted value.
• Member expressions. The two new let Ok(..) else { continue } skips are genuinely behaviour-preserving: RootSqlNode routed MemberExpression to default_processor, so it never reached the measure chain where ungrouped_measure_references lived. The "as it did before" comments are accurate.
• Identity is preserved where it's load-bearing. full_name/alias/cube_name come from origin; MemberSymbol::eq is full_name + discriminant, and Schema::find_column_for_member compares resolve_reference_chain(), which walks a ColumnRef through to its origin — so column resolution, find_member_positions, resolve_alias_for_member and dimension_coalesce_refs all still match a substituted symbol. is_measure()/is_dimension() returning false for a ColumnRef is not reachable in a harmful position: schema membership is decided by which Vec a symbol lives in, and make_order_by's is_measure() runs on the unsubstituted order-by item.
• Termination. ColumnRefSymbol declares both slots skip, so apply_recursive cannot descend into origin; combined with the guard in replacement() and in RenderReferencesSqlNode, a substitute that reads its own member through a reference can't be rebuilt around itself. Covered by tests from three angles (bare, nested, render-reference map).
• Join conditions. Every remaining member-bearing ON clause is accounted for: logical_join's on_sql and the calc-group branch of add_multistage_dimension_join use the surviving render_references; add_multistage_dimension_join's non-calc-group branch substitutes against the join's own ReferencesBuilder; keys_aggregate_strategy / inner_join / full_join / rolling-window conditions are all Expr::Reference and never needed the map. add_subquery_join's Expr::new_member resolves through make_cube_references, which is what the new sub_query_dim_join_key_for_multiplied_measure test pins.
• filter/having scope. The old root-level RenderReferencesSqlNode applied to everything a select rendered, so substituting filter as well as having is not a widening — it's the same reach expressed structurally. SingleSource::Cube resolves to None, so a plain cube join's WHERE is unaffected.
• Pre-aggregation path. Discarding all but calc_group_literals reproduces the old clear_render_references() + re-add exactly, and the pre-agg reference nodes live below RootSqlNode, which a ColumnRef bypasses — no double interception.

Nice touches: RootSqlNode's _ => becoming an explicit MemberExpression(_) arm (a future variant now fails to compile rather than silently falling into the default chain), and RawReferenceValue / ungrouped_measure_references / render_references_mut / clear_render_references leaving the tree with zero remaining references.

Findings

#	Severity	Where	What
1	low	aggregate_multiplied_subquery.rs:105-113	Rationale comment describes the render_references-populated-later mechanism this PR removes; the join_context_factory it justifies may now be redundant given make_cube_references
2	low	query.rs:230-237	The pre-aggregation branch throws away everything collected 30 lines above; gating the collection on !is_pre_aggregation keeps the intent local

Both posted as inline comments. No prior review threads existed on this PR, so nothing to resolve or dedupe against.

Nits (not posted inline)

• aggregate_multiplied_subquery.rs:215 uses if !exists where the neighbouring loop at 256 uses if *exists — same meaning, two spellings a few lines apart.
• multi_stage_rolling_window.rs:92 carries the pre-existing format!("date_from") into the new date_from closure; "date_from".to_string() says it without the macro.
• The doc blocks on ColumnRefSymbol and measure_over_reference run well past three lines. I'm not asking for cuts — each paragraph states an invariant the rest of the PR depends on (leafness, origin-is-not-a-dependency, what is and isn't carried over), which is the case where length earns itself.

Caveats

• I could not build or test. cargo is not permitted in this sandbox, so nothing here is machine-verified — the review is a static read of the changed files plus the surrounding call sites. Your reported cargo test -p cubesqlplanner --features integration-postgres run (1381 passing, including SQL executed against a real Postgres) is what backs the refactor; CI should be the gate.
• Testing coverage looks proportionate to the mechanisms introduced, and the out-of-scope multi-value calc group is disclosed in the PR body rather than silently left broken.

· branch `tesseract-references-as-symbols`

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.35%. Comparing base (5019312) to head (b14d426).
⚠️ Report is 4 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master   #11857   +/-   ##
=======================================
  Coverage   80.35%   80.35%           
=======================================
  Files         500      500           
  Lines      106555   106555           
  Branches     3881     3881           
=======================================
  Hits        85619    85619           
  Misses      20386    20386           
  Partials      550      550           

Flag	Coverage Δ
cube-backend	60.23% <ø> (ø)
cubesql	84.76% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[claude]

Claude finished @waralexrom's task in 1m 23s —— View job

---

Re-review of b14d426: both prior findings addressed — no new findings, 0 open

Full review

Todos

• Diff the incremental commit against my prior review
• Verify the pre-aggregation gate is behaviour-preserving
• Check the reworded comments
• Resolve the two now-stale threads

What changed since my last pass

b14d426 touches three files and answers both low findings from the previous round.

Finding 2 — pre-aggregation gate (query.rs). The let substitutions = if is_pre_aggregation { calc_group_literals } else { substitutions } discard is gone; the three collection loops now sit inside if !is_pre_aggregation. Verified behaviour-preserving:

• All three collectors are pure with respect to everything except substitutions: collect_query_dimension_substitution takes &self and only consults dimension_coalesce_refs before delegating; ReferencesBuilder holds a single Rc<From> with no interior mutability, so collect_substitutions_for_member / _for_filter write nowhere else. Skipping them drops no side effect.
• substitutions now starts as calc_group_literals moved rather than cloned, and calc_group_literals has no use after line 136 (grep confirms three occurrences: init, insert, move). On the pre-agg path it therefore reaches substitute_symbols_in_* holding exactly the pinned values — identical to the old discard branch.
• Hoisting measures_for_query and over_full_aggregated_source above the gate is safe: both take &self/&context, and schema.measures is not mutated between the old and new call sites. Both are read below the gate, so hoisting is required rather than incidental.
• over_full_aggregated_source is false for QuerySource::PreAggregation (query.rs:27), so nesting the having collection under !is_pre_aggregation cannot change which selects collect it — it was already unreachable on that path.

Finding 1 — stale rationale (aggregate_multiplied_subquery.rs). The nine-line block naming render_references is replaced by five lines stating a mechanism that exists today. I checked the claim rather than taking it: the PK condition is built with Expr::new_member_with_context(dim, join_visitor_context), which pins the context at construction, so SelectBuilder::build's make_cube_references(from) never reaches it. The join_context_factory is genuinely load-bearing and the new comment says why. My suggestion that it might be redundant was wrong on that point — new_member_with_context, not new_member, is what decides it.

measure_over_reference doc. Trimmed from twelve lines to four; the surviving paragraph is the one the body does not state — why case and the measure filters are dropped. The "what it keeps" list was re-derivable from the struct literal below it.

Findings

None. Both prior threads resolved.

Nits (unchanged from last round, not re-posted)

• aggregate_multiplied_subquery.rs:215 if !exists vs the neighbouring if *exists at 256.
• multi_stage_rolling_window.rs:92 format!("date_from").

Caveats

• Still could not build or test — cargo is not available in this sandbox, so this remains a static read. CI plus your reported cargo test -p cubesqlplanner --features integration-postgres run is the gate.

· branch `tesseract-references-as-symbols`