refactor(tesseract): move render-time flags and maps into symbol forms - #11425
refactor(tesseract): move render-time flags and maps into symbol forms#11425waralexrom wants to merge 8 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## tesseract-symbol-dependencies-refactor #11425 +/- ##
==========================================================================
+ Coverage 79.40% 83.91% +4.51%
==========================================================================
Files 480 257 -223
Lines 98646 80714 -17932
Branches 3636 0 -3636
==========================================================================
- Hits 78333 67734 -10599
+ Misses 19795 12980 -6815
+ Partials 518 0 -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:
|
8a1b7c7 to
6369eeb
Compare
…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).
7014ba7 to
89fe764
Compare
|
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 reviewTodos
General assessmentThis 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:
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 Findings
Details and suggested fixes are in the five inline comments. Smaller notes (not worth inline threads)
VerificationI reviewed the current tree state rather than a |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| let ungrouped = self.row_level_semantics | ||
| && match node.as_ref() { | ||
| MemberSymbol::Measure(m) => matches!( | ||
| m.render_modifier(), | ||
| Some(MeasureRenderModifier::RawValue | MeasureRenderModifier::UngroupedFinal) | ||
| ), | ||
| _ => false, | ||
| }; |
There was a problem hiding this comment.
Conditional masks lose their CASE WHEN in ungrouped queries when the mask SQL has dependencies.
row_level_semantics is only true on the node wrapping the final measure chain. When that node defers a dependency-carrying mask (dependencies_count() > 0 → Ok(None) at L89), rendering falls through to the evaluate-position node, where row_level_semantics == false, so ungrouped is forced to false there regardless of the measure's render modifier.
The grouped branch at L114-125 then runs. In an ungrouped query group_by_members is empty — QueryProcessor only calls set_group_by_members when !modifers().ungrouped (processors/query.rs:186) — so all_in_group_by is always false and the node returns the bare masked_sql, dropping the conditional entirely.
Concrete case: ungrouped: true + a masked measure with both a mask filter and a mask.sql that references a member (e.g. {CUBE.amount} * 0 - 1). Rows that satisfy the mask filter should render CASE WHEN <filter> THEN <original> ELSE <mask> END; they render <mask> instead. Since the query is ungrouped, the CASE WHEN would have been valid at row grain here — the guard is only needed for aggregates.
The new fixture covers masks without filters (integration_masking.yaml), so this path is untested. Either propagate the row-grain decision to the evaluate-position node (e.g. derive it from the symbol form rather than the node position), or add a test pinning the intended output for ungrouped + conditional mask.
| let mut new = schema.clone(); | ||
| new.time_dimensions = new | ||
| .time_dimensions | ||
| .iter() | ||
| .map(|d| transforms::mark_tz_converted_at_source(d, &names)) | ||
| .collect::<Result<Vec<_>, _>>()?; |
There was a problem hiding this comment.
Narrower reach than the map it replaces: only schema.time_dimensions is rewritten.
The removed dimensions_with_ignored_timezone map was consulted at render time by full name, so it suppressed the conversion for any node rendered in the select whose name matched — including a time dimension reached through schema.dimensions. This transform only rebuilds the time_dimensions vector, and QueryProcessor (processors/query.rs:139-150) likewise derives its name set from schema.time_dimensions only.
That a plain dimensions entry can resolve to a time dimension is assumed elsewhere in the planner — e.g. multi_stage_query_planner.rs:704-708 walks query_properties.dimensions() and pulls out resolve_reference_chain().as_time_dimension(), and multi_stage_rolling_window.rs:132 compares dim.resolve_reference_chain() against the rolling time dimension for exactly that reason.
Failure mode: a query over a pre-aggregation whose time dimension arrives via schema.dimensions (reference chain / view re-export) renders convert_tz() a second time over an already-converted rollup column, shifting the result by the tz offset.
If the narrowing is deliberate (i.e. a time dimension can never reach schema.dimensions in a pre-aggregation read), it's worth stating that invariant in the doc comment; otherwise dimensions should be mapped too.
| measure_drill_filters: measure.measure_drill_filters.clone(), | ||
| measure_order_by: measure.measure_order_by.clone(), | ||
| mask_sql: measure.mask_sql.clone(), | ||
| render_modifier: measure.render_modifier.clone(), |
There was a problem hiding this comment.
Carrying render_modifier across an unroll can produce a self-inconsistent symbol: this transform clears rolling_window and multi_stage, which are exactly the predicates MeasureRenderModifier::applies_to uses for RollingMerge (is_cumulative()) and MultiStageRank/MultiStageWindow (is_multi_stage()). An unrolled measure that still carried one of those would now fail the new applies_to assertions in multi_stage_window.rs:49 / multi_stage_rank.rs:41, or silently route to RollingWindowNode's non-cumulative delegate.
Safe today only because the single call site (multi_stage_query_planner.rs:711) runs at logical-planning time, before any stamping. Given the PR's stated rule that the symbol is the full description of what renders, setting render_modifier: None here (or asserting it is None) would make that ordering constraint enforced rather than incidental.
| None => &self.aggregated, | ||
| Some(MeasureRenderModifier::RollingMerge) => &self.rolling_merge, | ||
| Some(MeasureRenderModifier::RawValue) => &self.raw_value, | ||
| Some(MeasureRenderModifier::UngroupedFinal) => &self.ungrouped_final, |
There was a problem hiding this comment.
Nit / consistency: the PR describes applies_to as "the single authority — stamping consults it, render nodes assert it", but this dispatcher asserts nothing. RawValue/UngroupedFinal are unconditionally applicable so there is nothing to check, but RollingMerge requires is_cumulative(), and a mis-stamped one lands in RollingWindowNode, whose MemberSymbol::Measure(_) arm silently delegates to the aggregated chain — the exact silent fallthrough the rank/window nodes were changed to reject. An applies_to check on the RollingMerge arm here would close the gap.
| "}; | ||
| let query = format!("{}{}", query, MASKED_MEMBERS); | ||
|
|
||
| ctx.build_sql(&query).unwrap(); |
There was a problem hiding this comment.
These guards assert on executed rows, while the change they protect is about rendered SQL — build_sql is only checked for Ok. On these small seeds several rendering regressions produce identical row sets (e.g. an extra/missing sum() wrap over already-unique rows, a redundant convert_tz inside a granularity truncation, a CASE WHEN … IS NOT NULL THEN 1 vs. a raw pass-through where every value is non-null). And with --features integration-postgres off, try_execute_pg returns None and the tests pass having asserted nothing at all.
Adding insta::assert_snapshot!(ctx.build_sql(query).unwrap()) alongside the result snapshot would make these actual render guards and keep them meaningful without a live Postgres. (The result snapshots are still worth keeping — they're what catches semantically-wrong-but-valid SQL.)
1 participant
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), pluslogical_plan/transforms/for schema-level lifts; symbol struct fields arepub(super)so transforms rebuild via full struct literalsTimeDimensionSymbol.tz_converted_at_sourcereplaces thedimensions_with_ignored_timezonerender map (pre-aggregation reads, rolling-window inputs)MeasureKind::AggregatedState(HLL state ofcount_distinct_approx) replaces thecount_approx_as_stateflag; stamped at logical planning (pre-aggregation builds, multi-stage leaves under a rolling stage); every decision match overMeasureKind/AggregateWrapis exhaustive — no wildcard armsMeasureSymbol.render_modifier(RawValue/UngroupedFinal/RollingMerge/MultiStageRank/MultiStageWindow {partition}) replaces theungrouped,ungrouped_measure,rolling_window,multi_stage_rank,multi_stage_windowfactory flags; stamped per select by the physical processors, dispatched by a single render node; window partitions travel as member symbols instead of pre-rendered stringsMeasureRenderModifier::applies_tois the single authority for form applicability: stamping consults it, render nodes assert it (loud internal errors instead of silent fallthroughs)row_level_semantics)is_splitted_sourcefield and two dead sql-node files removed;MemberSymbol::PartialEqdocumented as member identity (content excluded)Known deliberate deviations (details in commit messages): a
count_distinct_approxreachable 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
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 nowCUBEJS_TESSERACT_SQL_PLANNER=true): 43 suites, 496 tests🤖 Generated with Claude Code