chore!: accumulated review nits (column-metric and viewer fixes, timing names, lost tests, stale comments)

[zzylol]

Why

The clean reviews of the merged stack and the later PRs (#542 … #724) left a list of nits for one follow-up PR. Many were fixed since. This PR checks every item against current main, fixes the ones still open that no in-flight PR owns, and records the status of every item in the table below.

What

Grouped by area. Behaviour is unchanged except for the three bug fixes, and each of those has a test that fails without the fix.

Bug fixes

• types: column_metric assumed an aggregate's group keys come first and its measures last. A per-entity reduction (count_over_time) keeps every label and replaces the sample value in place, so its value column got the absolute metric and a label after it got the count's.
• Viewer: stageScalarText printed (time() 0 2) for an Arithmetic node, because ArithmeticOpKind serializes as a string ("Add"), whose first key is "0".
• Viewer: validateStagePipeline treated a scalar_roots that is not an array as empty instead of reporting it.

Materialization timing (ir::timing, ExecutionDataStateError)

• Names that said "maintenance" or "phase" but mean ingestion-time materialization: EvaluationUnderMaintenance → QueryTimeValueUnderIngestionSummary, InvalidMaintenanceBinary → InvalidIngestionTimeBinary (now documented), split_shared_by_phase → split_shared_by_timing.
• MaterializationAssignment's Debug prints each assigned node's kind and address, not its whole sub-DAG.
• The edge-rule table now states that a query-time MaintainPopulation does not read a MatAtIngestion input's stored output.
• New tests: an ingestion-fixed node rejects MatAtQuery; an explicit NotMaterialized keeps a fixed timing; a fixed-timing conflict survives the split.

Missing tests

• Three IR regression tests lost when the legacy IR was removed are back: filtered vs unfiltered aggregates, without() over a renamed sample column, and series identity over vector(1).
• Pass 1 HAVING: a scalar-reference consumer (Filter(scan, EXISTS G)) keeps a key-only HAVING above the output; with stacked Filters, only the adjacent one counts as HAVING.
• Materialization: the population test now asserts that something is stored, and a new test checks that a mixed placement (one summary stored, one rebuilt) gives the same answers.
• Tumbling panes: the samples are now a parameter instead of a thread-local. New tests cover an empty middle pane, counters over panes with an outer offset, and gaps and overlaps between panes.
• Executor: factory-built and DAG-built heap states merge interchangeably.
• Stage 3: a summary shared by two queries takes the smaller failure-probability share, in either order; with a declared counter, the CMS+heap candidate gets per-node prices.
• execution_timed_dag: an empty workload gives NoRoots, and a literal-only workload gives a DAG with no nodes. Exact-state roots and their state run at query time.

Small cleanups

• PATTERN_A is an array of PatternAQuery (named fields) instead of unnamed tuples.
• Removed the pass-through same() from devtools candidate_labels.
• The Pareto front computes each candidate's worst latency once, not once per comparison.
• Pass 2 test locals still named after the old records (groups, variants, sites) now use the *Candidate vocabulary.

Stale comments

• "Read out" → evaluation.
• AbsoluteValue/RelativeValue → absolute/relative error.
• The KMV|HLL merge comment now explains HLL's idempotent merge, and the stale "no KMV kernel" note is gone.
• The misplaced compile_node doc is back on compile_node.
• compile_exact_evaluation now says its counter window has offset 0.
• Example 1's 27 candidates are explained as 3 × 9.
• parsed_workload now names the query-language frontends and Stage 1.
• The factory updater's claim about DAG state is limited to the heap families.
• A SQL fold test no longer claims that its target is unmet.

How

• column_metric asks a new aggregate_measure_at which measure an output column holds. It follows the layout aggregate_output_schema builds: a per-entity measure sits at its input column; a cross-series measure follows the kept labels, and a count_values label comes before its count unless an earlier column already has that name.
• The front calls dominates_at with latencies computed once per candidate. dominates keeps its behaviour by calling it.
• Everything else is renames, comment edits and tests.

Key code interfaces added / modified

crates/types/src/post_asap/execution_data_state.rs

/// A plan-construction-time data_state violation; the type stays.
pub enum ExecutionDataStateError {
    /// Bad plan: a maintained population's maintenance/evaluation contract does not hold.
    InvalidMaintainedPopulation,                                              // unchanged
    /// Bad plan: a query-time value placed beneath a summary built at ingestion time.
    QueryTimeValueUnderIngestionSummary { edge: &'static str, child: ExecutionDataState }, // was EvaluationUnderMaintenance
    /// Bad plan: an edge whose child data_state the parent does not accept.
    IllegalChildDataState { edge: &'static str, child: ExecutionDataState },  // unchanged
    /// Bad plan: SummaryAgg over non-exact summary state.
    UnsupportedStateComposition { family: String },                           // unchanged
    /// Bad plan: one shared node assigned two timings.
    ConflictingTiming { first: ExecutionDataState, second: ExecutionDataState }, // unchanged
    /// Bad plan: a root run at ingestion time that is not stored.
    UnstoredIngestionRoot { operator: &'static str },                        // unchanged
    /// Bad plan: an explicit materialization that needs the other timing than the fixed one.
    MaterializationConflictsWithFixedTiming { operator: &'static str, fixed: ExecutionTiming, materialization: Materialization }, // unchanged
    /// Bad plan: a filtered summary build at ingestion time.
    FilteredSummaryAtIngestion { operator: &'static str },                   // unchanged
    /// Bad plan: an ingestion-time BinaryOp the update path cannot evaluate (not plain
    /// arithmetic, different per-series rows, or an unsupported schema).
    InvalidIngestionTimeBinary,                                               // was InvalidMaintenanceBinary
    /// Bad plan: a checked division without exactly one valid guard at query time.
    InvalidCheckedDivision,                                                   // unchanged
    /// Bad plan: an exact operator reads a non-plain column.
    NonPlainOperand { column: String, dtype: String },                       // unchanged
    /// Unsupported: an ASAP operator with no timing contract yet.
    UnimplementedOperator { operator: &'static str },                        // unchanged
    /// Bad plan: MatAtIngestion over a sub-DAG that runs only at query time.
    UpstreamNotAtIngestion { operator: &'static str },                       // unchanged
    /// Internal bug: export reached a node without a timing.
    UntimedNode { operator: &'static str },                                  // unchanged
    /// The exported physical DAG failed its own validation (e.g. NoRoots for an empty batch).
    InvalidPhysicalDAG(PhysicalASAPDAGValidationError),                       // unchanged
}

crates/types/src/ir/timing.rs

/// Copy every sub-DAG reached with two different timings from `roots`, one copy per
/// timing, and remap `assignment` onto the copies; Stage 2 calls it before timing a workload.
pub fn split_shared_by_timing(                                                  // was split_shared_by_phase
    roots: &[Rc<OperatorNode>],
    assignment: &MaterializationAssignment,
) -> (Vec<Rc<OperatorNode>>, MaterializationAssignment) { … }

/// Debug output of an assignment: each assigned node as `Kind@address` with its
/// materialization, plus the default; the type itself stays.
impl std::fmt::Debug for MaterializationAssignment { … }                       // was derived

crates/devtools/src/planner_layering_examples.rs

/// One of #509 Example 3's Pattern A queries, with the window and offset its text states;
/// the example workloads and their tests read it.
pub struct PatternAQuery {                                                     // new
    /// The PromQL text.
    pub promql: &'static str,
    /// Its range window, in ms.
    pub window_ms: u64,
    /// Its offset before T, in ms.
    pub offset_before_t_ms: u64,
}
/// Pattern A's five queries.
pub const PATTERN_A: [PatternAQuery; 5] = [ … ];                               // was [(&str, u64, u64); 5]

crates/types/src/types.rs (private)

/// The measure an aggregate's output `column` holds, or `None` for a key, a kept label or
/// a count_values label, using the layout aggregate_output_schema builds; column_metric uses it.
fn aggregate_measure_at<'a>(node: &OperatorNode, reduction: &Reduction, measures: &'a [AggIntent],
    child: &OperatorNode, column: usize) -> Option<&'a AggIntent> { … }        // new

crates/asap-aware-mapping/src/stage3_plan_selection.rs (private)

/// `dominates` with each usage's worst-query latency already computed; the Pareto front calls it.
fn dominates_at((a, la): (&ResourceUsage, f64), (b, lb): (&ResourceUsage, f64)) -> bool { … } // new

Before this PR

• count_over_time(m[5m]) (output [ts, value, job]): column_metric gave column 1 (the count) the absolute metric and column 2 (job) the L1-scaled count metric.
• The viewer showed (time() 0 2) for time() + 2 and silently accepted "scalar_roots": {"0": …}.
• Error names said "maintenance", and split_shared_by_phase said "phase", where both mean timing.

After this PR

• The same query gives the count's L1-scaled metric to column 1 and the absolute metric to job.
• The viewer shows (time() Add 2) and reports "scalar_roots must be an array".
• Every item on the nit list has a status (table below).

Item status

#	From	Item	Status	Where / why
1	#542	same_reference in pass/major.rs should use ir::cse::same_node	Not applicable	pass/major.rs was deleted with MajorPass (#674); no same_reference helper remains
2	#542	Reword the doc at major.rs ~57	Not applicable	file deleted (#674)
3	#666	Comment the dropped FILTER at summary_coverage_e2e.rs ~166	Already fixed by #666 (the test doc says "without its FILTER clause")
4	#666	pass/mod.rs ~270 should say "shared by Rc"	Fixed here	comment in execution_timed_dag
5	#666	Root/state timing assertions; literal-only and empty execution_timed_dag tests	Fixed here	execution_timed_dag_of_an_empty_and_a_literal_only_workload; timing asserts in exact_state_roots_are_finalized
6	#666	Document the nested NoRoots error	Already fixed by #666 (doc on execution_timed_dag)
7	#665	"dag" → "DAG" in promql_lowering.rs ~930	Fixed here
8	#665	library-api.md ~41-43 root signatures	Owned by docs PR (#732)
9	#543	Port: ir/cse filtered vs unfiltered aggregates	Fixed here	ir::cse::tests::filtered_and_unfiltered_aggregates_do_not_merge
10	#543	Port: without() over a renamed sample column	Fixed here	ir::aggregate_schema::tests::without_aggregate_drops_a_renamed_sample_value
11	#543	Port: series identity over a direct vector literal	Fixed here	ir::schema_support::tests::series_identity_accepts_direct_vector_literal
12	#543	tools/dag-viewer/README.md ~176 link	Owned by docs PR (#732)
13	#667	Query-time MaintainPopulation over a MatAtIngestion input is refused: apply the stored-output rule or document it	Fixed here	documented in the ir::timing edge-rule table (behaviour unchanged)
14	#667	Query-time SummaryAgg reads ingestion rows from an unstored fixed-timing node	Open, tracked in issue	rejecting it changes which plans validate
15	#667	Two materialization_e2e tests should assert something is stored	Fixed here	the population test now asserts it; the quantile test already did
16	#667	Mixed-placement test	Fixed here	mixed_placement_answers_like_query_time
17	#667	Document that fixed_window_rate_candidates needs MatAtIngestion	Owned by legacy-core deletion (replacement.rs)
18	#667	post-asap-ir.md ~108; document the split remap	Owned by docs PR (#732) for the .md; the remap is already in the split_shared_by_timing doc
19	#667	Rename leftover "Maintenance" names	Fixed here	the asap-types ones (see the audit rows); OperationPlacement::Maintenance and recurrence.rs belong to the legacy-core deletion
20	#667	MaterializationAssignment's Debug output is verbose	Fixed here	manual Debug: kind and address per node
21	#561	Scalar-reference consumer blocks a key-only HAVING push-down	Fixed here	a_scalar_reference_consumer_keeps_a_key_only_having_above
22	#561	Stacked-Filter test	Fixed here	only_the_adjacent_of_stacked_filters_is_having
23	#667 r3	Ingestion-fixed node given MatAtQuery	Fixed here	ingestion_fixed_node_rejects_mat_at_query
24	#667 r3	Explicit NotMaterialized on a fixed node	Fixed here	explicit_not_materialized_keeps_a_fixed_timing
25	#667 r3	Conflict error survives split_shared_by_phase	Fixed here	a_fixed_timing_conflict_survives_the_split
26	audit	"read out" → evaluation	Fixed here	code comments; lib.rs/cost_model.rs belong to the legacy-core deletion, #573 lines to the docs PR
27	audit	Rename EvaluationUnderMaintenance, InvalidMaintenanceBinary	Fixed here	QueryTimeValueUnderIngestionSummary, InvalidIngestionTimeBinary
28	audit	Rename maintenance_operation_plan_cost_rate, summary_maintenance_cost_per_update	Owned by legacy-core deletion (cost_model.rs)
29	audit	split_shared_by_phase: "phase" means timing	Fixed here	split_shared_by_timing
30	audit	AccuracyModel::local_guarantee	Owned by legacy-core deletion (it edits AccuracyModel; callers in replacement.rs)
31	audit	execution_timed_dag → physical_asap_dag [judgment]	Follow-up PR	after the legacy-core deletion
32	audit	Rewrite the crate and facade module docs per stage	Owned by legacy-core deletion (rewrites the lib.rs docs)
33	audit	Structural items	Owned by #572
34	post-merge	stage-viewer.md 135–139	Owned by docs PR (#732)
35	post-merge	Pass 1 files carry no stage number; parsed_workload.rs "frontend stage"	Fixed here	parsed_workload.rs reworded; file renames go to #572
36	post-merge	Leftover legacy wording	Owned by legacy-core deletion
37	post-merge	User decisions (state pricing, tie message, KLL δ<0.01, candidate-count asserts)	Already fixed by #682
38	#686	stageScalarText Arithmetic op label reads "0"	Fixed here	bug fix, failing test first
39	#686	Empty "Stage 3 selected no plan: " after a Pass 1 failure	Already fixed by #698
40	#682 r3	Heap entries priced at 24 B; no per-key overhead	Owned by #726
41	#687	SQL GROUP BY over an inner DDSketch gets None	Left open	a feature (the fold needs a resolution), not a nit
42	#687	Test doc overstates "unmet"	Fixed here
43	#682 r5	extend-asap-aware-mapping.md:571	Owned by docs PR (#732)
44	#693	Negative tests for the HLL scope condition	Already fixed by #715
45	#693	max_distinct_per_evaluation doc: longest window	Already fixed by #715
46	#693	Leftover estimator in construct_summary_agg	Owned by legacy-core deletion (replacement.rs)
47	#693	State that δ is per output row	Already fixed by #715
48	#693	accuracy-models.md:215	Owned by docs PR (#732)
49	#696	Misplaced require_sound_merge doc	Fixed here
50	#696	HLL idempotence comment in the KMV|HLL arm	Fixed here	also drops the stale "no KMV kernel" note
51	#696	Test merging an empty counter pane	Fixed here	exact_increase_over_an_empty_middle_pane_matches_single_build
52	#696	TimeShift unwrap looks through one shift	Not applicable	pane shifts fold into the shift below (pane_chain::shifted): a range reads through at most one
53	#695/#698	HydraCmsGroup::approx_memory_bytes excludes the grid	Left open	needs shared-grid dedupe in every row-buffering operator; not small
54	#695/#698	validate_hydra_cms rejects Constant(2.0)	Not applicable	Pass 1 only emits Constant(1.0)
55	#695/#698	Viewer: non-array scalar_roots should be an error	Fixed here	bug fix, failing test first
56	#706	Factory and DAG keyed states interchangeable	Fixed here	factory_and_dag_heap_states_are_interchangeable
57	#706	WeightedFrequencyUpdater doc: DAG state only for heap families	Fixed here
58	#706	Old CMS/CS heap updaters unused	Already fixed by #721
59	#706	update_keyed .expect on overflow	Left open	the trait returns (); returning the error is a trait change (the DAG build already returns it)
60	#708	accuracy_failing test fails on metric, not size	Already fixed by #718 (an unset metric accepts any, so the strict target fails on size)
61	#708	test_server.py skipUnless back to the class	Fixed here
62	#708	for_quantile doc; L2 intent target_in(ABSOLUTE)	Already fixed by #722 (for_quantile gone; L2 is Relative)
63	#708	Stale AbsoluteValue/RelativeValue wording	Fixed here	composition.rs reasons, univmon.rs test doc
64	#708	Reconciliation dominates ignores the metric	Owned by legacy-core deletion (deletes reconciliation.rs)
65	#708	Viewer lacks the L1-scaled option	Left open	viewer feature
66	#707	column_metric's Aggregate arm assumes keys first, measures last	Fixed here	bug fix, failing test first
67	#707	node_metric projection paths inconsistent for grouped CASE over L2	Open, tracked in issue	fixing it changes those queries' sizing metric
68	#707	Cap error message should use MAX_GROUP_MEMBERS	Already fixed by #724 (cap removed)
69	#712	G counts rows, not values	Owned by #689
70	#712	Document input_cardinality	Already fixed by #728 (replaced by per-source facts)
71	#712	A sizing error aborts select_plan; stage1_to_stage3 drops roots via .ok()?	Already fixed by #722 (a rejection now; the .ok()? resize already succeeded in select_plan)
72	#712	Placement copy fails silently on node mismatch	Left open	unreachable (resizing keeps the node structure), so no failing case to pin
73	#712	Inline the 'equal_split' literal	Owned by legacy-core deletion (replacement.rs)
74	#712	Docs still name AccuracyBudgetAllocator	Owned by docs PR (#732)
75	#712	Tests: shared summary's largest n; CountSketch enlargement	Fixed here	a_shared_summary_takes_the_smallest_failure_probability_share; CountSketch was already covered (#712)
76	#713	@ shifts collapse panes in counter_window	Not applicable	SummaryMerge coverage refuses panes read through @, and Pass 2 offers none
77	#713	Tests: counters over panes with an outer offset; gap/overlap rejection	Fixed here	a_counter_over_panes_with_an_outer_offset_matches_single_build, counter_panes_with_a_gap_or_an_overlap_are_rejected
78	#713	Example 1 doc '27' wording	Fixed here	3 × 9
79	#713	compile_exact_evaluation offset doc	Fixed here
80	#713	Thread-local PHASE_MS test helper	Fixed here	samples are a parameter
81	interfaces	/api/plan example sends "delta"; server.py drops it	Left open	viewer feature (δ input)
82	interfaces	new_with_planning_inputs doc mentions the removed allocator	Owned by legacy-core deletion (replacement.rs)
83	#717	Selection.accepted doc	Already fixed by #722
84	#717	summary_sharing.rs module doc ("tests ignored")	Not applicable	still accurate: re-run, the three ignored tests still fail (#580)
85	#717	"ImplicitExact" in per_computation_candidates.rs	Already fixed by #722
86	#717	"null without a target" in stages.js	Already fixed by #722
87	#717	No test for SelectionError::EntryCount	Already fixed by #722 (an_entry_count_mismatch_is_an_error)
88	#717	Count-Min P2 pricing coverage	Fixed here	a_declared_counter_prices_the_count_min_heap_candidate
89	#717	accepted → Vec<AcceptedAccuracy>	Already fixed by #722
90	#717	accuracy_targets() computed twice	Left open	author's choice; one clone per entry
91	#717	QueryWorkloadEntry: Default only for tests	Left open	author's choice; the default is documented
92	#717	physical_candidate_count re-implements every operator	Left open	refactor beyond a nit
93	#722	Quantile resized for a stricter sibling loses its metric	Owned by legacy-core deletion (replacement.rs)
94	#722	Siblings with different scopes share one ε	Owned by legacy-core deletion
95	#722	replacement.rs:1558 metric comparison	Owned by legacy-core deletion
96	#722	Names: AccuracyTarget.error, Rejection.valid, accuracy_target(intent), --metric/--epsilon, PATTERN_A	Fixed here	PATTERN_A is named here; accuracy_target waits for the legacy-core deletion (legacy callers); the serialized and CLI names go in a follow-up PR, after the legacy-core deletion
97	#724	merged_error infers "has heaps" from any Approximate entry	Owned by #727
98	#724	same() pass-through wrapper	Fixed here
99	#724	Test locals variants/groups/site	Fixed here
100	#724	devtools same_computation unmemoized	Not applicable	fine at today's sizes (the note says so)
101	#724	union() sort-and-sweep	Left open	a sweep may reorder the boxes that coverage compares; needs care beyond a nit
102	#724	Remove the #727 fallback	Owned by #727
103	#723	ingest_workers O(n·e) per state	Owned by #734 (#679, rewrites price())
104	#723	Front recomputes worst_latency_ms	Fixed here	dominates_at
105	#723	CandidateCost → CandidateUsage, NodeCost.per_second renames	Left open	author's choice
106	#723	raw_data keyed by Debug string	Owned by #734 (#679, price())

Counts: 41 fixed here, 18 already fixed, 26 owned by another PR or issue, 7 not applicable, 2 open and tracked in issues, 1 follow-up PR (row 96 is partly fixed here, with its other renames in the same follow-up PR), 11 left open.

Verification

Only the tests covering the changes were run, on the head after rebasing onto main (ac8493a2, #734):

• cargo nextest run -p asap-types: 273 passed.
• -p asap-aware-mapping: the asap_aware_cse, stage2_physical_asap_optimization and accuracy::composition modules, the new pass and Stage 3 tests, the cluster-objective tests and --test per_computation_candidates: all passed.
• -p asap-physical-operators --test deployment: 10 passed.
• -p asap-planner --test summary_sharing: 9 passed, 3 ignored. When run, the three ignored tests still fail (Pass 2 sharing and Stage 1 coverage parity with the retired MajorPass #580).
• -p asap-integration-tests: materialization_e2e, tumbling_pane_merge, the Pattern A tests of planner_layering_example3, and planner_layering_example4's materialized_output_covers_its_consumers and pareto_front: all passed.
• -p asap-devtools --test stage_pipeline -E 'test(display_cap)': passed.
• python3 -m unittest test_viewer test_server in tools/dag-viewer: passed.
• cargo fmt --all -- --check and cargo check --tests on the touched crates: clean, no warnings.
• The three bug-fix tests (a_per_entity_count_states_its_value_column_not_its_last_column, ScalarTextTests, and the "scalar roots not an array" case) were seen to fail before their fixes.

Screenshots: not applicable. No measurements: the front change is a constant-factor saving with the same result.

Limitations and follow-ups

• Rows 14 and 67 stay open, each tracked in its own issue. The renames in rows 31 and 96 go in a follow-up PR after the legacy-core deletion. The other "Left open" rows are features or the author's choice.
• Rows owned by the local legacy-core deletion, feat(asap-aware-mapping)!: price sketch nodes from measured operation costs in Stage 3 #734 (Calibrate plan selection's costs from measured sketch costs #679), [Bug] Stage 3 sizes a sketch's memory by its full capacity, not by the samples it holds #726, Pass 1 offers CountSketch for a count, but the executor cannot evaluate a CountSketch total #727, Track exact vs approximate per column so key-only filters over approximate inputs keep their guarantee #689, Reorganize crates and modules by #509 stages and the #511 unified IR #572 and the docs PR (docs: describe the planning stages, devtools and types now on main #732) are left to those.