refactor(tesseract): move render-time flags and maps into symbol forms by waralexrom · Pull Request #11425 · cube-js/cube

waralexrom · 2026-07-30T09:45:11Z

Summary

Render-time behavior of Tesseract members was decided by name-keyed maps and boolean flags on SqlNodesFactory, matched again during SQL generation — fragile against granularity suffixes, reference chains and alias overrides. This PR makes the symbol the full description of what renders: derived forms are materialized on the symbols of the selects they belong to, and the render chain becomes a plain interpreter.

Stacked on # (symbol dependency traversal), which it builds on.

Changes

planner/symbols/transforms/ — all symbol-mutating logic moved out of the symbol types into plain transform functions (symbol level), plus logical_plan/transforms/ for schema-level lifts; symbol struct fields are pub(super) so transforms rebuild via full struct literals
TimeDimensionSymbol.tz_converted_at_source replaces the dimensions_with_ignored_timezone render map (pre-aggregation reads, rolling-window inputs)
MeasureKind::AggregatedState (HLL state of count_distinct_approx) replaces the count_approx_as_state flag; stamped at logical planning (pre-aggregation builds, multi-stage leaves under a rolling stage); every decision match over MeasureKind/AggregateWrap is exhaustive — no wildcard arms
MeasureSymbol.render_modifier (RawValue / UngroupedFinal / RollingMerge / MultiStageRank / MultiStageWindow {partition}) replaces the ungrouped, ungrouped_measure, rolling_window, multi_stage_rank, multi_stage_window factory flags; stamped per select by the physical processors, dispatched by a single render node; window partitions travel as member symbols instead of pre-rendered strings
MeasureRenderModifier::applies_to is the single authority for form applicability: stamping consults it, render nodes assert it (loud internal errors instead of silent fallthroughs)
Masking derives row-grain semantics from the measure's form; the positional final-chain/evaluate-node asymmetry is explicit (row_level_semantics)
Dead is_splitted_source field and two dead sql-node files removed; MemberSymbol::PartialEq documented as member identity (content excluded)

Known deliberate deviations (details in commit messages): a count_distinct_approx reachable only through a dimension dependency tree renders its final value instead of an HLL state (the old behavior was latently wrong); measures referenced inside mask filters render unmodified through the unmasked root (moves with the planned masking rework).

Testing

New integration guard tests (tests/integration/ungrouped_forms.rs) with baseline snapshots captured on the pre-change code: masking × ungrouped (including a mask with row-level dependencies), ungrouped × multiplied count, ORDER BY of an unselected measure, rolling count-distinct — these were red on an intermediate broken variant and are green now
Rust: 1092 unit + integration-postgres tests, clippy clean
JS schema-compiler integration suite under the native planner (CUBEJS_TESSERACT_SQL_PLANNER=true): 43 suites, 496 tests
Each migration step passed an adversarial equivalence review (per-path tracing against the pre-change code); an architecture review validated the kind-variant / render-modifier / composable-fact taxonomy

🤖 Generated with Claude Code

codecov · 2026-07-30T09:57:17Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.43%. Comparing base (d8d009b) to head (3874ed6).

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #11425      +/-   ##
==========================================
- Coverage   83.95%   79.43%   -4.52%     
==========================================
  Files         257      480     +223     
  Lines       80887    98778   +17891     
  Branches        0     3636    +3636     
==========================================
+ Hits        67908    78464   +10556     
- Misses      12979    19796    +6817     
- Partials        0      518     +518

Flag	Coverage Δ
cube-backend	`59.00% <ø> (?)`
cubesql	`83.95% <ø> (ø)`

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.

…ms module Symbols are immutable values; every derived-copy operation now lives in planner/symbols/transforms as a plain function instead of a method on the symbol types: - unroll_rolling (was MeasureSymbol::new_unrolling) - patch_measure (was MeasureSymbol::new_patched) - into_multiplied / regular_in_multiplied (was into_multiplied / convert_multiplied_to_regular; kind-level logic stays on MeasureKind) - apply_static_filter_to_symbol and friends (moved from symbols/common, absorbing the replace_case methods) - substitute_by_name (extracted from FullKeyAggregateMeasures::render) - strip_join_prefix (was MemberSymbol::with_stripped_join_prefix plus per-type strip_join_prefix methods) Symbol struct fields are pub(super) so transforms rebuild them via full struct literals: adding a field fails to compile until every transform classifies it, the same guarantee symbol_deps! gives for traversal.

…ymbol property A time dimension read from a pre-aggregation rollup or from a rolling window input CTE carries an already timezone-converted value. This was tracked in SqlNodesFactory as a full-name set consulted at render time — matching members by name during SQL generation, invisible in the plan. The property now lives on the symbol itself: TimeDimensionSymbol gets an ignore_timezone flag, and the selects that read such sources are built from schemas whose time dimensions (and their occurrences inside filter trees) carry the flag. TimeDimensionNode reads it from the symbol; the factory set, its setter and both fill sites are gone. New transform layers, by input level: - planner/symbols/transforms: ignore_timezone_for (recursive symbol rewrite) and map_filter_symbols/map_filter_item_symbols — a generic filter-tree symbol walker that static-filter application now shares. - logical_plan/transforms: ignore_timezone_in_schema — schema-wide lift of the symbol transform.

…th a measure kind form The mergeable HLL state of count_distinct_approx was a render-time boolean: SqlNodesFactory.count_approx_as_state, threaded from the physical build (pre-aggregation builds) and from the multi-stage evaluation context (leaves under a rolling window), consulted by the final-measure nodes during SQL generation. The state form is now a MeasureKind variant, mirroring MultipliedCount: AggregatedState renders as AggregateWrap::CountDistinctApproxState (hll_init; hll_merge when read back from a rollup). It materializes at logical planning via the transforms::measures_as_state tree rewrite — in QueryProperties finalize for pre-aggregation builds and in the multi-stage leaf CTE under an aggregating stage — so the logical plan itself shows that a leaf produces a state. Removed: the factory flag and both final-node fields, PushDownBuilderContext.render_measure_as_state, EvaluationContext.measure_as_state, and the pre_aggregation_query parameter of PhysicalPlanBuilder::build. Every decision match over MeasureKind, AggregateWrap and their inner aggregation-type enums is now exhaustive — a future form variant fails to compile at each decision point instead of falling through a wildcard arm. One deliberate behavior change: a count_distinct_approx measure reachable only through a dimension dependency tree (e.g. a subquery dimension) used to render as an HLL state inside the dimension subquery under the flag; it now renders as a final value, since a dimension value cannot be a state blob.

cube_calc_groups.rs and original_sql_pre_aggregation.rs were not wired into the module tree and matched a MemberSymbol variant that does not exist. MemberSymbol's PartialEq compares member identity (full_name + variant); the doc comment now states that symbol content does not participate, so derived forms of a member compare equal to the original and this equality must not be used to distinguish them.

…tiplied measures, order by, rolling Result snapshots capture the semantics these combinations must keep: a masked measure stays masked in an ungrouped query (including a mask whose SQL has row-level dependencies), a multiplied count keeps its distinct form, ORDER BY of an unselected measure sorts by the row-level value, and a rolling count-distinct leaf emits the raw distinct key. None of these interactions were covered before.

…e render modifier The row-level rendering of measures was two render-time booleans: SqlNodesFactory.ungrouped / ungrouped_measure, set per select by the physical processors, branching the final-measure chain and switching MaskedSqlNode into its ungrouped mode. The form now lives on the symbol: MeasureSymbol.render_modifier — Option<MeasureRenderModifier> with Ungrouped (raw row value: measure subqueries, ungrouped multi-stage leaves) and UngroupedQueryValue (row value in an ungrouped query; count-likes render a not-null indicator). It materializes when each select is built, from the same inputs the flags used: the pushdown context's measure_for_ungrouped takes precedence over the select's ungrouped modifier, and ORDER BY symbols of unselected measures are included. The factory builds all three measure chains and MeasureRenderModifierSqlNode routes per-measure. Masking keeps its positional asymmetry explicitly: only the node wrapping the final measure chain applies row-level mask semantics (deferring dependency-carrying masks to the evaluate-position node, which always masks with grouped semantics), now derived from the measure's modifier instead of select-level flags. Removed: both factory flags and setters, MaskedSqlNode::new_ungrouped, and the dead MeasureSymbol.is_splitted_source field. Known deviation: a measure referenced inside another member's mask filter renders through the unmasked root without a modifier (final aggregation) where the old select-level flags applied; this corner moves with the masking rework.

…ags with measure render modifiers The last per-select render decisions living on SqlNodesFactory move to the measure symbols of the selects they describe: - RollingMerge replaces the rolling_window flag: the rolling-window select stamps it on its measures, and the dispatcher routes them to the RollingWindowNode chain (window-partial merge by kind), which the factory now builds unconditionally. - MultiStageRank / MultiStageWindow { partition } replace the multi_stage_rank / multi_stage_window partition strings: the partition travels as member symbols and is rendered through the regular chain, resolving to the same qualified columns via the select's render references. The processor keeps the alias-existence guard and stamps a schema copy used for both projections and ORDER BY. The rank/window nodes keep their chain position and kind checks but dispatch by the measure's modifier. The modifier enum carries data now, so it is Clone (not Copy), and masking derives row-grain semantics only from the two ungrouped variants — rolling and windowed measures keep grouped mask semantics as before. SqlNodesFactory is left with reference maps, cube aliases, time shifts and group-by member names only.

…rm applicability at render MeasureRenderModifier::applies_to is the single authority for which measures take a form: stamping consults it (a modifier lands only on compatible measures instead of every measure of the select), and the rank/window render nodes assert it, failing loudly on an incompatible combination instead of silently falling through. The dispatcher arm for rank/window measures is an internal error — they must be intercepted by their dedicated nodes. Renames for accuracy: Ungrouped → RawValue (a row-level value re-aggregated by an enclosing select), UngroupedQueryValue → UngroupedFinal (the final row-level output of an ungrouped query), ignore_timezone → tz_converted_at_source (the fact, not an instruction; it composes with future derived forms). Documented the dual meaning of a None modifier (no decision yet / final aggregation — coincident because stamping only fills None) and the transform rebuild-style rule (full struct literal when a transform decides per field, clone-and-mutate for single-field stamps).

claude · 2026-07-30T17:40:35Z

Claude finished @waralexrom's task in 6m 57s —— View job

Review complete — strong refactor, no blockers. 1 medium, 1 medium-question, 3 low (5 inline comments)

Full review

Todos

Gather context (diff, commits, prior review threads — none existed)
Review symbol transforms & render-modifier model
Review physical-plan processors / stamping sites
Review sql_nodes render chain
Review tests & snapshots
Post review

General assessment

This is a genuinely good structural change. Replacing name-keyed render maps and boolean factory flags with forms materialized on the symbol removes a whole class of "matched again at render, missed because of a granularity suffix / alias override / reference chain" bugs. Things I specifically liked:

Exhaustive matching over MeasureKind / AggregateWrap. Every arm in measure_kinds/mod.rs lists variants explicitly instead of using _ =>. Adding a kind now fails to compile at each decision site — that is the right trade for a taxonomy this load-bearing.
MeasureRenderModifier::applies_to as a single predicate consulted by stamping and asserted by the rank/window nodes. Turning silent fallthroughs into loud internal errors is the correct direction.
The transforms/ module doc on rebuild style (full struct literal when classifying per field, clone-and-mutate when stamping one field) is an unusually well-chosen convention, and patch_measure.rs / unroll_rolling.rs follow it.
MemberSymbol::PartialEq documented as member identity with the explicit warning that derived forms compare equal. Given how much this PR leans on derived forms, documenting that trap was necessary.
The None == "no decision yet" == "default form" note on MeasureRenderModifier honestly explains a precedence scheme that would otherwise read as an omission.
Dropping is_splitted_source and two dead sql-node files is welcome cleanup.

No security concerns: all inputs are already-compiled data-model symbols, no new string interpolation of user values, no auth surface touched. No performance concerns either — the transforms are Rc-sharing recursive rebuilds run once per select at plan time, negligible next to SQL generation.

Findings

#	Severity	Where	Issue
1	Medium	`sql_nodes/masked.rs:75-125`	Conditional masks lose their `CASE WHEN` in ungrouped queries when the mask SQL has dependencies — the deferral path lands on the node with `row_level_semantics == false`, where `group_by_members` is empty so the grouped guard always fires
2	Medium (question)	`logical_plan/transforms/tz_converted_at_source.rs:19-24`	Only `schema.time_dimensions` is rewritten; the removed name-keyed map reached any node in the select. A time dimension arriving via `schema.dimensions` would get `convert_tz` applied twice over a rollup column
3	Low	`symbols/transforms/unroll_rolling.rs:41`	Preserves `render_modifier` while clearing `rolling_window` / `multi_stage` — the exact predicates `applies_to` checks. Safe only by call-site ordering
4	Low	`sql_nodes/measure_render_modifier.rs:49-52`	Dispatcher asserts nothing; a mis-stamped `RollingMerge` silently delegates to the aggregated chain via `RollingWindowNode`
5	Low	`tests/integration/ungrouped_forms.rs:59`	Guards assert executed rows, not rendered SQL; vacuous without `--features integration-postgres`

Details and suggested fixes are in the five inline comments.

Smaller notes (not worth inline threads)

processors/query.rs:139-148 computes the time-dimension name set twice — once for the filter closure, once inside mark_tz_converted_at_source_in_schema. Passing the set in would keep the two definitions from drifting.
SqlNode::childs() has no callers anywhere in the crate. RollingWindowNode::childs() omits default_processor while the new MeasureRenderModifierSqlNode::childs() returns all four branches — harmless today, but the trait method is either dead code to remove or an invariant to honour consistently.
RollingWindowNode's MemberSymbol::Measure(_) if !is_cumulative arm is now unreachable in the new routing (only RollingMerge reaches it, and applies_to requires is_cumulative). Worth a note if kept as defence-in-depth.
Docs: no /docs-mintlify changes needed — this is internal planner refactoring with no user-facing surface. Correct call.

Verification

I reviewed the current tree state rather than a git diff against origin/master — git diff/git show invocations were not permitted in this sandbox, so the pre-change code was not directly available for line-by-line comparison. Findings 1 and 2 are therefore stated in terms of what the current code does; whether each is a regression from the old flag/map behaviour is what I'm asking about. I did not build or run the Rust test suite (CI and the PR description cover that: 1092 tests, clippy clean).

• branch `tesseract-symbols-rewrite`

- assert RollingMerge applicability in the render-modifier dispatcher - drop the render modifier when unrolling a rolling measure - mark tz-converted-at-source across all schema members, so embedded granularity references carry the mark too; unit test on the transform - pin conditional dependency-carrying mask behavior in ungrouped queries

claude · 2026-07-30T18:32:06Z

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

Follow-up review of `3874ed6` — all 5 prior threads addressed; no new findings

Full review

Todos

Read 3874ed6 (fixes per review)
Verify each of the 5 prior findings
Resolve stale threads (all 5)
Post follow-up review

Verification of each prior finding

#	Prior finding	Status
1	Ungrouped conditional dep-carrying mask drops `CASE WHEN`	Answered + pinned. Accepted — the positional asymmetry is pre-existing and `row_level_semantics` reproduces it deliberately. The reason given (deriving row-grain in both positions makes the measure render unmasked — a leak) is the stronger argument; rendering the mask value unconditionally is the safe side of that trade. `test_ungrouped_conditional_dep_mask` + snapshot now pin it (all 9 rows `-1.00`, no original leaking through the `status = 'completed'` rows), so a future masking rework can't silently regress the invariant. Deferring the `CASE WHEN` opportunity is the right call for this PR's scope.
2	`tz_converted_at_source` reach	Fixed, and my failure mode was wrong. Correct: the old map was populated from `schema.time_dimensions` names too, so a dimension-only-arriving time dimension was never suppressed. The real gap — embedded granularity references (`{CUBE.created_at.day}`) holding an unmarked `Rc` after a vector-only rewrite — is the one that mattered, and `mark_tz_converted_at_source_in_schema` now maps `time_dimensions` / `dimensions` / `measures`. The name gate still comes from `time_dimensions` only, so widening the rewrite doesn't widen the suppression set — same semantics as the map, now reaching nested occurrences. `tz_mark_reaches_time_dimensions_embedded_in_other_members` asserts both the pre-state (`Some(false)`) and the post-state, which is what makes it a real guard rather than a tautology.
3	`unroll_rolling` carrying `render_modifier`	Fixed. `render_modifier: None` with a comment naming why (unrolling erases the properties the modifier is stamped against). Ordering constraint now enforced by construction.
4	Dispatcher asserts nothing on `RollingMerge`	Fixed. `Some(modifier @ RollingMerge)` binds and checks `applies_to`, internal error otherwise — same shape as `multi_stage_rank.rs:41` / `multi_stage_window.rs:49`. The `applies_to`-is-the-single-authority claim now holds at every dispatch site.
5	Result-only assertions in the integration guards	Won't fix, accepted. A project-wide "no SQL-string snapshots in the Tesseract suite" convention is a legitimate maintainer call, and the churn argument is real for a renderer under active refactoring. The `integration-postgres`-off point does apply to the whole suite by construction, so it isn't a property of these tests.

All five threads resolved.

New commit review

Nothing new to flag. Reviewed the four changed source sites plus both new tests:

mark_tz_converted_at_source_in_schema — the local mark closure keeps the three vector rewrites from drifting; marking measures is safe because the symbol-level transform is name-gated and idempotent (!td.tz_converted_at_source() short-circuits).
MeasureRenderModifierSqlNode — RawValue/UngroupedFinal correctly stay unchecked (applies_to is unconditionally true for them); the multi-stage arms still hard-error. No arm can now reach a chain its form doesn't fit.
symbol_transforms.yaml — minimal fixture, and the inline comment explaining that {CUBE.created_at.day} compiles into an embedded TimeDimension is the kind of note that keeps a fixture from being "simplified" into uselessness later.
The new integration test's comment states the security invariant it protects, not just the mechanics — worth keeping when the masking rework lands.

Only residual nit, unchanged from last round and not worth a thread: processors/query.rs:139-148 still derives time_dimension_names locally for the filter/having pass while mark_tz_converted_at_source_in_schema recomputes the same set internally. Two definitions of one set; passing it in would make the coupling explicit.

Verification caveat

cargo check was not permitted in this sandbox, so the new test module and fixture are reviewed for API correctness by inspection only (LogicalSchema::set_time_dimensions / set_dimensions / into_rc, TestContext::create_time_dimension / create_dimension, MemberSymbol::apply_recursive all exist with matching signatures). CI covers the build and the run.

• branch `tesseract-symbols-rewrite`

github-actions Bot added the rust Pull requests that update Rust code label Jul 30, 2026

vercel Bot deployed to Preview July 30, 2026 09:46 View deployment

waralexrom force-pushed the tesseract-symbol-dependencies-refactor branch from 8a1b7c7 to 6369eeb Compare July 30, 2026 15:56

Base automatically changed from tesseract-symbol-dependencies-refactor to master July 30, 2026 17:33

waralexrom added 8 commits July 30, 2026 19:34

waralexrom force-pushed the tesseract-symbols-rewrite branch from 7014ba7 to 89fe764 Compare July 30, 2026 17:40

vercel Bot deployed to Preview July 30, 2026 17:42 View deployment