[Refactor] Remove dead code, single-impl abstractions, and the duplicate planning pipeline

[milindsrivastava1997]

Concrete maintenance problem

The Rust workspace has about 180k lines: roughly 97k production and 83k test. A repo-wide over-engineering audit found a lot of dead code, abstractions with only one implementation, and two planning pipelines running side by side. Together these make the code hard to follow.

The compiler couldn't flag any of this, because data_plane/src/lib.rs:1 and 14 control-plane modules turn off its dead-code warnings with #![allow(dead_code, unused_imports, …)].

Method: reference counts across the repo, done once with test code included and once with it stripped, plus cargo rustc -- --force-warn dead_code --force-warn unused_imports on both library crates. Grep-based counts can miss uses through macros or common method names, so every deletion needs cargo check and the test suite before it merges.

Before and after structure

Findings are ranked by the largest cut first. Line counts are estimates and include tests that exist only to cover the deleted code.

1. Old sketch cost-estimator chain (≈ −2,750 lines). The files are control_plane/src/physical/{allocator,plan,planner,window_fusion,deployment}.rs. Their only production output is the plan_summary field in the /api/v1/plan response (control_plane/src/main.rs:1149), and nothing in the repo reads that field. Delete the chain and the field.

2. Test-only PromQL planning inside the data plane (≈ −1,200 lines). This covers asap_query_engine/post_asap_planner.rs, the *_from_summary_executor functions in live_serve.rs, and execute_post_asap_* in post_asap_readout.rs. Production never reaches this code: engine.rs:703 and engine.rs:1057 branch on #[cfg(test)] / #[cfg(not(test))], so tests run a different query path than production. After: tests install a QueryPlan the way production does, and the cfg(test) branches go away.

3. Comments that narrate history (≈ −1,100 lines). About 1,050 comment lines tell project history ("Phase γ", "B7.5 retired", "M2.3.6g", "used to…"). control_plane/src/lib.rs:26 also has a 90-line old→new module-mapping table that points at modules that no longer exist (intent_algebra, optimizer). Keep only comments that explain current intent or invariants, since git history already records the rest.

4. Public items that are dead or used only by tests (≈ −900 lines, about 110 items). The biggest groups:

   • SketchStore::{descriptor_counts, policy_count, item_label_for, …}
   • hot_reload_config::{discard_staged, retire_drained, materialization_statuses}
   • planner_selection::{select_workload, select_summary_or_keep}
   • query_plan::compile_bound*
   • servers/metrics.rs start_ingest_timer and record_ingest_*
   • the public spawn_mock_thanos_* helpers in thanos_query_engine/forward.rs

   Delete these or move them under #[cfg(test)].

5. Modules only their own tests use (−388 lines). control_plane/src/physical/summary_reconcile.rs and control_plane/src/threshold_alloc.rs.

6. Factory traits nobody uses (≈ −150 lines). SingleSubpopulationAggregateFactory, MultipleSubpopulationAggregateFactory, AccumulatorFactory (which has no implementations), and the 4 *AccumulatorFactory structs in storage_engines/types/traits.rs:225-280 and the operator files. None of them is ever constructed.

7. WindowMerger / NaiveMerger / create_window_merger (≈ −70 lines). This is a trait plus a factory with one implementation, kept for merge strategies planned for "later" (sketch_db/query/window_merger.rs:19). Replace it with a merge_all(&[Box<dyn AggregateCore>]) function.

8. Query adapter traits (≈ −80 lines). There are 3 traits (QueryRequestAdapter, QueryResponseAdapter, HttpProtocolAdapter) for 2 near-identical Prometheus-style adapters, and create_http_adapter (adapters/factory.rs:8) is a match with one arm. Collapse them into one trait and construct adapters directly.

9. AsyncQueryNodeRuntime / execute_async (≈ −90 lines). physical_dag.rs:28. The only implementation is a test counter.

10. Other traits with one implementation (≈ −100 lines). Replace each with inherent methods or free functions:

    • RealizationProvider → ExistingRealizations (physical/realization.rs:13)
    • Emitter → ThreeStageEmitter (colored_dag/emitter.rs:173)
    • the copied-in SummaryExecutor, which has one production implementation plus 2 test mocks

11. Files that only re-export something (≈ −70 lines). Import from where each item is actually defined:

    • query_engines/canonical/mod.rs (0 users)
    • physical/topology.rs (0 users)
    • physical/summary_catalog.rs
    • storage_engines/types/storage_backend.rs
    • the old timeline_dispatch/window_merger aliases in query_engines/mod.rs:29 (0 users)
    • the asap_tier alias
    • StoreResult

12. Kill switches whose "off" setting just disables the feature (≈ −60 lines). USE_TYPED_STAGE_SPLIT (physical/stage_split.rs:27, plus else branches in main.rs and replan.rs) and ASAP_SUMMARY_EXECUTOR_LIVE (live_serve.rs:78, checked in 5 serving functions). Turning the second one off breaks all planned serving.

13. Router written out 3 times (≈ −60 lines). The production router is at control_plane/src/main.rs:514, with hand-copied test versions at :2268 and :3303. Build it in one fn app(state) -> Router and use that everywhere.

14. MergeableAccumulator<T> (≈ −40 lines). T is always Self, and the call sites look like <X as MergeableAccumulator<X>>::merge_accumulators(...). Replace it with an inherent X::merge(vec).

15. types_v2.rs (≈ −30 lines). Merge it into types.rs, drop its allow(dead_code), and delete the unused BindingName and helper functions.

16. Private dead code found once warnings were forced on (≈ −60 lines).

    • ThroughputTotals, since, throughput_totals (precompute_engine/metrics.rs)
    • MaintenanceRuntime::summary
    • ASAPQueryEngine.prometheus_scrape_interval
    • flusher::metadata_store
    • colored_dag::allocator::binding_stage
    • unused imports in emitter.rs:39

    After cleanup, remove the blanket #![allow(dead_code, …)] attributes so the compiler keeps catching this.

17. Unused dependencies. rusqlite (which compiles SQLite from source), uuid, structopt and urlencoding in data_plane/Cargo.toml, and zstd and tokio-stream in control_plane/Cargo.toml. Separately, reqwest 0.11 and 0.12 are both built.

Net: about −7,000 lines and −6 dependencies, not counting item 18.

18. Two planning pipelines (decision needed, probably −6k lines or more). The README itself calls POST /api/v1/plan the "legacy planning API", but it's still fully alive. It runs through pipeline.rs, replan.rs, physical/workload_planner.rs, physical/plan_cache.rs, physical/deployment_cost/{pareto,tco,online,delta}.rs, and the /plan/auto, /pareto, /tco, /rollback, /diff, /agents and /config handlers. The physical-plan/compile-and-publish compiler is the path that e2e tests and user docs use. In this repo, nothing exercises /plan/auto, /pareto, /tco, /agents or /config. The complication is that the startup workloads.yaml registry still goes through the legacy path. Consolidating onto the physical compiler would remove the largest source of duplicate concepts, but it changes the API, so it needs a separate decision.

Behavior that must remain unchanged

Items 1–17 should cause no behavior change except that the unread plan_summary response field disappears (item 1). These must keep working:

• the physical-plan compile, publish and install flow
• PromQL/SQL serving from installed QueryPlans
• ingest and precompute
• the HTTP routes used by docs and e2e tests

Item 18 removes API surface on purpose and is out of scope until it's decided.

Why the proposed structure is minimally complex

Every item deletes code or collapses a layer. None adds a new abstraction.

Existing unit and end-to-end protection

• scripts/e2e.sh and the process e2e tests in data_plane/tests and control_plane/tests cover the physical-plan path.
• Item 2 changes tests that currently depend on the cfg(test)-only branch (for example, the ASAP_SUMMARY_EXECUTOR_LIVE tests in data_plane/tests/e2e_controller_plans_and_backend_serves.rs). Those tests have to switch to an installed QueryPlan first.

Compatibility or migration impact

• Item 1 drops plan_summary from the /api/v1/plan response.
• Items 4, 6 and 11 remove public Rust items. The only consumers are inside this workspace.
• Item 18 would remove HTTP endpoints and move the workloads.yaml registry onto the physical compiler.

Suggested order: split the work into small PRs, starting with the least ambiguous items (17, 16, 5, 1, 2).

[milindsrivastava1997]

FYI @zzylol

[milindsrivastava1997]

Addendum to item 2: the rest of that block in ASAPQueryEngine::execute_at is also dead in production

Item 2 cites the #[cfg(test)] None => serve_instant_from_summary_executor(...) arm at engine.rs:1057. The block around that arm (engine.rs:1040-1135 at 674b9573) is unreachable in non-test builds too, apart from a plan-swap race:

• lookup succeeds. The first if let (L1014-L1034) always returns, either Ok or through ? from prepare_logical / execute_logical_entry.
• lookup fails. The second block runs the same query_plan.lookup(query) on a fresh snapshot, gets the same QueryNotPlanned, and maps it to CapabilityMiss.

So these parts never run for instant queries in production:

• the Some(plan) => Ok(entry) => serve_instant_from_query_plan arm
• its mark_materializing / mark_ready / mark_serving readiness handling
• the warm/archive stitch at L1125-L1132

The only way in is ArcSwap installing a plan between the two physical_plan_snapshot() calls.

Suggestion: after item 2 removes the cfg(test) arm, delete the whole block. When lookup fails, return CapabilityMiss (with the capability-miss notify) straight away.

Not dead:

• stitch_warm_and_archive / archive_engine are still used by execute_range_promql_modern (L793-L800). That path is reachable for planned range queries with no Logical nodes, because only entries that contain Logical nodes return early through execute_logical_range.
• The trailing no-sketch-index branch is still reachable when sketch_index is None.

🤖 Generated with Claude Code