chore: fix the small nits collected from clean reviews

[zzylol]

Why

Clean reviews of the stack from the first Pass 2 PRs through the stage-crate split (#741) left small nits, collected for one follow-up PR. The code has moved a lot since: crates were rearranged by stage, the legacy core was deleted, and #740 renamed the fields and flags that read poorly. So many of the nits no longer apply. This PR goes through every nit on today's main. It fixes the ones that still apply and are mechanical, marks the obsolete ones, and lists the ones that need a design decision or are too big.

What

• Stage 1 stays frontend-free outside its tests. hll::classic_failure_statistic was public only so a Stage 3 test could build the statistic it binds. It is pub(crate) again, and the test uses test_support::hll_classic_failure_statistic, which is compiled only with the test-support feature. The Stage 1 crate doc and its Cargo.toml comment no longer claim the crate "never depends on a front end": with test-support on, it depends on asap-frontend-promql to lower test queries.
• QueryRecurrence::fixed_interval_ms(). The free function workload_candidates::fixed_interval_ms(&recurrence) is now a method on the type in asap-types. Stage 2 no longer imports a helper from Stage 1 for it.
• Viewer: the Stage 3 list uses the rejection cause. It used to call a candidate "costlier" whenever the document had a cost for it. It now calls a candidate that failed a check "rejected" and one that lost under the cost model "costlier". The failed-check cause list is no longer copied into the tests: the Rust test asks RejectionCause::failed_a_check, and the viewer test reads FAILED_CHECK_CAUSES from stages.js.
• Executor: a precompute range over stacked time shifts reads raw samples. boundary_schema looked through only one TimeShift, so TimeRange(TimeShift(TimeShift(scan))) was treated as a stored summary population and failed.
• Small cleanups. Stage 3 keys its per-source raw-data map by Source instead of format!("{source:?}"). The measured_operation_costs module doc gets back the "CPU" that refactor!: rename AccuracyTarget.error, Rejection.valid, accuracy CLI flags, execution_timed_dag and accuracy_target #740 dropped. The locals holding CapabilitySharingCandidates are renamed from groups/group/group_options to candidates/candidate/candidate_options.

How

• QueryRecurrence::fixed_interval_ms has the same body as the old function. Both callers change: Stage 1's stage1_logical_asap_optimization, and Stage 2's builds_one_pane_per_evaluation.
• test_support::hll_classic_failure_statistic forwards to the crate-private function. Rustdoc links from public docs to it became plain code spans.
• stages.js splits REJECTION_CAUSES into FAILED_CHECK_CAUSES followed by the cost-model losses. stageCandidateRows picks a row's status from rejection.cause. RejectionCause now derives Deserialize, so the stage_pipeline test can parse the written cause back.
• boundary_schema uses while let over TimeShifts instead of if let.
• Source derives Hash, and the raw-data map is HashMap<&Source, (u64, PhysicalASAPNodeId)>.

Key code interfaces added / modified

crates/types/src/workload/mod.rs · crates/logical-asap-optimization/src/workload_candidates.rs

/// How often a query is evaluated. The type stays; this PR adds a method.
impl QueryRecurrence {
    /// The fixed interval at which the query repeats, in milliseconds: `Some`
    /// only for a fixed-interval repetition (with or without a phase) with a
    /// positive interval. Stage 1's window-composition rule and Stage 2's
    /// pane-retention check read it.
    pub fn fixed_interval_ms(&self) -> Option<u64>;   // new; was workload_candidates::fixed_interval_ms
}

// workload_candidates.rs
pub fn fixed_interval_ms(recurrence: &QueryRecurrence) -> Option<u64>;   // removed: use QueryRecurrence::fixed_interval_ms

crates/logical-asap-optimization/src/accuracy/estimators/hll.rs · crates/logical-asap-optimization/src/test_support.rs

// hll.rs
/// The symbolic statistic classic HLL's failure probability at `precision`
/// and `relative_error` over `source` is left as, until Stage 3 binds the
/// source's distinct bound.
pub(crate) fn classic_failure_statistic(precision: u8, relative_error: f64, source: &str) -> String;   // was pub

// test_support.rs (feature `test-support`)
/// The same statistic, for Stage 3 tests that bind it; compiled only
/// with `test-support`.
pub fn hll_classic_failure_statistic(precision: u8, relative_error: f64, source: &str) -> String;   // new

crates/plan-selection/src/lib.rs

/// Why Stage 3 did not select a candidate. The type stays; it now also
/// derives `Deserialize`, so the snake_case JSON name parses back.
#[derive(Debug, Clone, Copy, PartialEq, Eq, serde::Serialize, serde::Deserialize)]   // was Serialize only
#[serde(rename_all = "snake_case")]
pub enum RejectionCause {
    /// Failed check: sizing it for a query's failure probability failed.
    FailureProbabilityResizeFailed,    // unchanged
    /// Failed check: a query's accuracy target is not met.
    AccuracyTargetMissed,              // unchanged
    /// Failed check: it scans raw data at query time, but the deployment keeps none.
    RawDataNotRetained,                // unchanged
    /// Failed check: a query's latency is over its `max_latency_ms`.
    LatencyBoundExceeded,              // unchanged
    /// Failed check: under cost model 3, it is over one of the cluster's limits.
    OverClusterCapacity,               // unchanged
    /// Loss: under cost models 1 and 2, its price is higher than the selected one's.
    Costlier,                          // unchanged
    /// Loss: under cost model 3's lowest-latency objective, it is slower.
    Slower,                            // unchanged
    /// Loss: under cost model 3's least-memory objective, it uses more memory.
    MoreMemory,                        // unchanged
    /// Loss: on the Pareto front, but not its lowest-latency member.
    NotLowestLatencyOnParetoFront,     // unchanged
    /// Loss: another candidate dominates it on the Pareto front.
    Dominated,                         // unchanged
    /// Loss: it ties with the selected candidate and comes later in enumeration order.
    TiedLostOnEnumerationOrder,        // unchanged
}

crates/types/src/ir/operator/operator_properties.rs

/// The leaf data source of a `Scan`. The type stays; it now also derives
/// `Hash`, so Stage 3 can key its per-source maps by it.
#[derive(Debug, Clone, PartialEq, Eq, Hash, Serialize, Deserialize)]   // gains Hash
pub enum Source {
    /// A time series leaf (PromQL): rows `(ts, value, *labels)`.
    TimeSeries { metric: String },   // unchanged
    /// A tabular leaf (SQL); its columns are on `Scan.schema`.
    Table { table_ref: String },     // unchanged
}

tools/dag-viewer/stages.js

/// The `rejected[].cause` values of a candidate that failed a check
/// (`RejectionCause::failed_a_check`); such a candidate is not priced.
const FAILED_CHECK_CAUSES = [/* the five failed-check causes */];   // new
/// Every cause Stage 3 writes: the failed checks, then the cost-model losses.
const REJECTION_CAUSES = FAILED_CHECK_CAUSES.concat([/* the six losses */]);   // was one literal list
/// The Stage 3 list's rows; `status` is now picked by the rejection's cause
/// ("rejected" for a failed check, "costlier" for a loss), not by whether
/// the document has a cost for the candidate.
function stageCandidateRows(doc);   // behavior changed

API example

The viewer's Stage 3 list for the committed Example 1 document, as test_viewer.py loads it (output from running this at the head commit):

# js: V8 with node-style.js and stages.js loaded, as the viewer page has them.
js = stages_js()
# doc: the stage_pipeline document for #509 Example 1
# (sum by (job) (rate(http_requests_total[1m])), repeating every 10 s, exact).
doc = example1()
# rows: one row per candidate. Each row's status now comes from
# doc["stage3_plan_selection"]["rejected"][i]["cause"].
rows = js.call("stageCandidateRows", doc)
# Stage 3 wrote 38 accuracy_target_missed, 16 costlier and 2
# tied_lost_on_enumeration_order causes, and selected P1. The list shows:
#   Counter({'rejected': 38, 'costlier': 18, 'selected': 1})
# A rejection whose cause is a failed check stays "rejected" even if a
# document carries a cost for it; a loss stays "costlier" without one
# (test_status_follows_the_rejection_cause_not_the_cost).

The recurrence method, from the workload entry that reaches it:

// entry: a dashboard query repeating every 10 s, normalized for planning.
let entry = QueryWorkloadEntry::from(&RepeatingEntry {
    query: Query("sum by (job) (rate(http_requests_total[1m]))".into()),
    demand: RepeatedDemand::FixedInterval(RepetitionInterval(10_000)),
    requirements: QueryRequirements::default(),
    predictability: Predictability::Predictable { known_at: None },
});
// interval_ms: what Stage 1's window-composition rule and Stage 2's
// pane-retention check read, Some(10_000) here; None for a zero interval,
// a schedule, an estimated rate, a one-time query or an unknown recurrence.
let interval_ms = entry.recurrence.fixed_interval_ms();

Before this PR

• asap_logical_asap_optimization::accuracy::estimators::hll::classic_failure_statistic was part of Stage 1's public API only for one Stage 3 test, and Stage 1's docs said it never depends on a front end, which is false with test-support on.
• Stage 2 imported fixed_interval_ms from Stage 1's workload_candidates.
• The viewer showed a candidate as "costlier" whenever its cost was present, whatever the cause said. The failed-check cause list was written three times (stages.js, the stage_pipeline test and the viewer test).
• boundary_schema(TimeRange(TimeShift(TimeShift(scan)))) returned Invalid("stored population requires one typed summary state").

After this PR

• Stage 1's public API no longer has the statistic format. Later stages' tests use the test-support helper.
• A recurrence's fixed interval is recurrence.fixed_interval_ms().
• The viewer's Stage 3 statuses follow cause. The failed-check causes are listed once in stages.js and once in Rust (RejectionCause::failed_a_check).
• A range over any number of time shifts of a time series scan reads raw samples.

Nit table

Every nit from the collected review list, checked against main at 58999df.

Nit (source review)	Status
hll::classic_failure_statistic public only for a Stage 3 test (#741)	done, 4ae3fe0
Stage 1 lib doc "never depends on a front end" (#741)	done, 4ae3fe0 (lib doc and Cargo.toml comment)
fixed_interval_ms → QueryRecurrence::fixed_interval_ms() (#741)	done, 6353ef0
Move AccuracyModel to plan-selection (#741)	not done: another PR in progress (#572)
measured_operation_costs module doc lost "CPU" (#740)	done, 83304e7
Failed-check cause list duplicated in stages.js and a devtools test (#740)	done, 3c5d2e0
Viewer decides costlier vs rejected by cost presence, not cause (#740)	done, 3c5d2e0 (failing test first)
merged_error infers "has heaps" from any Approximate entry (#724)	not done: needs an explicit heap column in the family-capability table (an interface decision)
same() pass-through wrapper (#724)	obsolete: removed from devtools candidate_labels.rs
Test locals named variants/groups/site (#724)	obsolete in tests; the non-test locals holding CapabilitySharingCandidates are renamed in 2c1d711
devtools same_computation unmemoized (#724)	not done: fine at today's sizes (as the review said)
union() sort-and-sweep (#724)	not done: efficiency change, not mechanical
#727 fallback removal (#724)	tracked in #727
ingest_workers costs O(n·e) per state (#723)	not done: needs a producer index passed to its two callers; fine at today's DAG sizes
The Pareto front recomputes worst_latency_ms (#723)	obsolete: choose computes each candidate's latency once
CandidateCost → CandidateUsage (#723, author's choice)	not done: the type also holds the price, so "Cost" still fits; author's call
NodeCost.per_second → computations_per_second (#723, author's choice)	obsolete: the field is now cost_per_second plus multiplier
raw_data keyed by Debug string (#723)	done, 83304e7
Quantile resized for a stricter sibling loses its metric (becomes Rank) (#722)	not done: needs a decision on which metric a shared sizing target carries
Siblings with different scopes share one ε (#722)	not done: design
replacement.rs:1558 compares targets in different metrics (#722)	obsolete: replacement.rs was deleted with the legacy core
AccuracyTarget.error, Rejection.valid, accuracy_target(intent), CLI --metric/--epsilon (#722)	obsolete: renamed in #740 (max_error, cause, sizing_target, --max-error/--failure-probability/--accuracy-metric; no old flag is left in devtools or the viewer)
Unnamed PATTERN_A tuple (#722)	obsolete: now [PatternAQuery; 5]
Selection.accepted doc "None for a query without a target" (#717)	obsolete: rewritten
summary_sharing.rs module doc "tests ignored" (#717)	obsolete: it says three stay ignored, and three are #[ignore] with the #580 reason
"ImplicitExact" in per_computation_candidates.rs; "null without a target" in stages.js (#717)	obsolete: both gone
No test for SelectionError::EntryCount (#717)	obsolete: plan-selection has one
Count-Min P2 pricing coverage lost (#717)	obsolete: a_declared_counter_prices_the_count_min_heap_candidate
accepted → Vec<AcceptedAccuracy> (#717)	obsolete: done
accuracy_targets() computed twice (#717)	not done: one clone per query; avoiding it changes select_plan's signature
QueryWorkloadEntry: Default only for tests (#717)	not done: the default is now documented and the integration-tests helper crate and many tests use it; author's call
physical_candidate_count implements every operator a second time (#717)	obsolete: it reuses implement and materialization_candidates
Older stack nits (#542 same_reference in pass/major.rs; #666 FILTER comment, "shared by Rc", NoRoots doc, empty/literal-only DAG test; #665 "dag" comment, library-api.md roots; #543 lost regression tests, README link; #561 HAVING tests; #667 conflict/NotMaterialized/split tests, mixed placement, MaintainPopulation restriction, "Maintenance" names)	obsolete: code deleted or already fixed. The tests exist (e.g. filtered_and_unfiltered_aggregates_do_not_merge, without_aggregate_drops_a_renamed_sample_value, series_identity_accepts_direct_vector_literal, a_scalar_reference_consumer_keeps_a_key_only_having_above, only_the_adjacent_of_stacked_filters_is_having, fixed_timing_keeps_an_agreeing_assignment_and_rejects_a_conflict, explicit_not_materialized_keeps_a_fixed_timing, a_fixed_timing_conflict_survives_the_split, mixed_placement_answers_like_query_time, physical_asap_dag_of_an_empty_and_a_literal_only_workload), and the timing module documents the MaintainPopulation restriction
#667: query-time SummaryAgg reading ingestion rows from an unstored fixed-timing node; MaterializationAssignment Debug output	not done: not re-audited in this PR; needs its own check
Stage-alignment audit names (maintenance → materialization names, split_shared_by_phase, execution_timed_dag)	obsolete: renamed or removed
AccuracyModel::local_guarantee (audit)	not done: AccuracyModel is being moved in another PR
"read out" wording; design-doc lines (#573, stage-viewer.md, extend-asap-aware-mapping.md, accuracy-models.md, AccuracyBudgetAllocator docs)	not done: design docs are tracked in #677
#686 stageScalarText op label; empty "Stage 3 selected no plan"	obsolete: both fixed
#682 heap entries priced at 24 B vs about 60 B in sketchlib; no per-key overhead for sketch state	not done: cost-model numbers, needs a decision
#687 SQL GROUP BY src over DDSketch gets None	not done: needs a PerEntity fold resolution (design)
#687 test doc "unmet"	obsolete: the doc now says "unbound"
#693 negative scope tests for HLL (join, computed rows, Hydra, another source)	not done: not re-audited
#693 max_distinct_per_evaluation doc (longest window, per output row); leftover estimator name	obsolete: doc says both; construct_summary_agg is gone
#696 misplaced require_sound_merge doc; HLL idempotence comment	obsolete: both in place
#696 TimeShift look-through only one shift	done, 9de15ed (failing test first)
#696 test merging an empty counter pane	not done: needs an executor fixture
#695/#698 HydraCmsGroup::approx_memory_bytes excludes the grid	obsolete: the grid is counted once through shared_state
#695/#698 validate_hydra_cms rejects Constant(2.0)	not done: no non-negative proof exists for a non-unit constant weight, and no planner path emits one (design)
#695/#698 viewer non-array scalar_roots	obsolete: validation error exists
#706 WeightedFrequencyUpdater doc; old CMS/CS heap updaters	obsolete: doc limits "DAG state" to the heap families; the old updaters are gone
#706 update_keyed .expect on overflow	not done: making it fallible changes the AccumulatorUpdater trait; the comment explains why validated input cannot reach it
#706 factory vs DAG keyed states interchangeable test	not done: needs an executor fixture
#707 column_metric Aggregate arm assumes keys first	obsolete: aggregate_measure_at maps the column
#707 cap error message should use the constant	not done: the message still says "more than 20", but PerComputationCandidateError::Unsupported takes a &'static str, so using MAX_CAPABILITY_SHARING_MEMBERS needs a new error variant (an interface change)
#708 test fails on metric, not size; skipUnless on the class; AbsoluteValue/RelativeValue wording	obsolete: an unset metric now accepts any metric, so the test fails on ε; the decorator is on the class; the wording is gone
#708 viewer cannot set δ (server forwards only max_error and accuracy_metric)	not done: a small viewer feature (server field, editor input, test); separate PR
#708 reconciliation ignores the metric; viewer lacks the L1-scaled option	not done: not re-audited
#712 a single candidate's sizing error aborts select_plan	obsolete: now a FailureProbabilityResizeFailed rejection
#712 equal_split literal; new_with_planning_inputs doc	obsolete: gone
#712 G counts rows; input_cardinality doc; silent placement copy; tests for a shared summary's largest n and CountSketch enlargement	not done: not re-audited
#713 @ shifts collapse panes in counter_window	not done: a correctness question for the executor (an @ shift's offset is ignored); needs its own fix and test
#713 tests for counters over panes with an outer offset and for gap/overlap rejection; compile_exact_evaluation offset doc; PHASE_MS helper	obsolete: a_counter_over_panes_with_an_outer_offset_matches_single_build and counter_panes_with_a_gap_or_an_overlap_are_rejected exist; the offset doc is in place; PHASE_MS is gone
#713 Example 1 doc "27" wording	not done: not re-audited

Verification

• Failing tests written first:
  • asap-executor::precompute a_range_over_stacked_shifts_reads_raw_samples failed with Invalid("stored population requires one typed summary state"), and passes after the fix.
  • test_viewer.CandidateRowTests.test_status_follows_the_rejection_cause_not_the_cost failed with ('costlier', 'rejected') != ('rejected', 'costlier'), and passes after the fix.
• Tests of the changed code, all pass:
  • cargo nextest run -p asap-types -E 'test(fixed_interval_ms)': 1 passed.
  • -p asap-logical-asap-optimization, test(hll) | test(capability) | test(workload_candidates) | test(summary_capability) | test(window_composition): 15 passed; test(asap_aware_cse::): 22 passed.
  • -p asap-physical-asap-optimization, test(pane) | test(interval) | test(recurrence): 5 passed.
  • -p asap-plan-selection, test(classic_hll) | test(raw_data): 5 passed.
  • -p asap-devtools --test stage_pipeline: 22 passed.
  • -p asap-executor --test precompute: 2 passed.
  • python3 -m unittest test_viewer in tools/dag-viewer: 37 run, OK, 4 skipped (the skipped tests need a built stage_pipeline).
• cargo fmt --all -- --check: clean.
• cargo clippy -p asap-types -p asap-logical-asap-optimization -p asap-physical-asap-optimization -p asap-plan-selection -p asap-executor -p asap-devtools --all-targets --all-features --locked -- -D warnings: clean. Also cargo clippy -p asap-logical-asap-optimization --lib without features, to check that the now crate-private function is still used.
• Not run: the full workspace suite (GitHub CI runs it after merge).