Skip to content

feat(sketch-db): Phase 5e — real backfill execution with bit-identical determinism - #32

Merged
zzylol merged 1 commit into
mainfrom
sketchdb/phase5e-backfill-execution
Apr 18, 2026
Merged

feat(sketch-db): Phase 5e — real backfill execution with bit-identical determinism#32
zzylol merged 1 commit into
mainfrom
sketchdb/phase5e-backfill-execution

Conversation

@zzylol

@zzylol zzylol commented Apr 18, 2026

Copy link
Copy Markdown
Contributor

Summary

Turns backfill from shadow-mode into real work. A queued job now reads raw samples, rebuilds the sketch bit-identically to live, and writes per-window precomputes. 1,515 LoC across 3 new modules + wiring.

  • backfill_window_builder.rs — pure build_backfilled_accumulator(config, samples) rebuild primitive.
  • backfill_processor.rs — BackfillWindowProcessor implementing Phase 5c's WindowProcessor trait: groups samples by grouping_labels, batch-writes (PrecomputedOutput, accumulator) pairs, records provenance.
  • backfill_service.rs — tokio drain task with a ReaderFactory abstraction so Phase 5h can plug Prometheus / S3 / ClickHouse readers without touching the service.
  • BackfillRegistry::create_checked enforces §10.5 time-disjoint — end_ms <= schema.created_at_ms.
  • record_window_written / windows_written_by give Phase 5f a provenance list without an on-disk format change.
  • --enable-backfill-worker CLI flag, off by default (preserves shadow-mode); on, uses noop_reader_factory until real readers ship in 5h.

Determinism

Pre-implementation audit found sketch-core's Rust live path is fully deterministic (xxhash seeded by row index, preserved sample order, no order-sensitive FP). For raw-ingest deployments, backfill is bit-identical — locked by a test that builds the same accumulator via live and backfill paths and asserts serialize_to_bytes() equality.

For DC-ingest deployments (backend receives pre-built sketches from DC's Go sketchlib), bit-identicalness requires a cross-language audit — tracked as separate follow-up work.

Separation

Worker pipeline (PrecomputeEngine → SeriesRouter → Worker → active_panes → output_sink) is NOT reused. The pure stateless create_accumulator_updater factory IS reused (via the build_backfilled_accumulator facade). Module docs explain the choice; swapping to a copy-pasted factory is a one-line change if strict duplication is required.

Test plan

  • 14 new tests across 3 modules: aggregation correctness, end-to-end worker + processor, bit-identical parity, time-disjoint validation (3 branches), service drain lifecycle + shutdown.
  • 647 lib tests pass (up from 633).
  • clippy + fmt clean.

Open

  • 5f: query path consults provenance list for coverage-aware dispatch.
  • 5h: real Prometheus / S3 / ClickHouse readers.
  • sketchlib-go determinism audit for DC-ingest bit-identicalness.

🤖 Generated with Claude Code

@zzylol @claude
feat(sketch-db): Phase 5e — real backfill execution with bit-identica…
53909bb 
…l determinism

Turns backfill from shadow-mode into real work: a queued job now
actually reads raw samples from the exact DB, rebuilds the sketch
bit-identically to what live ingest would have produced, and writes
the per-window precomputes to the store.

## What's landed

### Core modules (all under `src/stores/sketch_db/`)

* `backfill_window_builder.rs` — pure function
  `build_backfilled_accumulator(config, samples) -> Box<dyn AggregateCore>`.
  Rebuilds one window's accumulator from samples in ingest order.
  Honours both SingleSubpopulation and MultipleSubpopulation
  dispatch (update_single / update_keyed).

* `backfill_processor.rs` — `BackfillWindowProcessor` implementing
  the Phase 5c `WindowProcessor` trait. Per window: looks up
  AggregationConfig by agg_id, groups samples by grouping_labels
  (same partitioning as live ingest), builds one accumulator per
  group, batch-writes `(PrecomputedOutput, accumulator)` tuples
  via `Store::insert_precomputed_output_batch`, then records
  `(agg_id, window_range)` in the registry for Phase 5f coverage.

* `backfill_service.rs` — long-running tokio task that polls the
  registry for Queued jobs and runs them through
  `BackfillWorker::run_job`. One job at a time in v1 (multi-worker
  + priority is §11.4 follow-up). Config-driven `ReaderFactory`
  abstraction so deployments can plug Prometheus / S3 / ClickHouse
  readers without touching the service code.

### Registry enhancements (`backfill.rs`)

* `BackfillRegistry::create_checked(schemas, agg_id, range, ...)`
  enforces §10.5 time-disjoint invariant: rejects `end_ms >
  schema.created_at_ms`. Live ingest owns `[created_at, ∞)`;
  backfill owns `[0, created_at)`. Disjoint by construction means
  no locks needed between live and backfill writers on the same
  `(agg_id, window)`.
* `record_window_written` / `windows_written_by` — per-job
  provenance list so Phase 5f's coverage tracker can distinguish
  `Backfilled { job_id }` from `Missing` without a new on-disk
  field.
* `CreateError` enum with `UnknownAgg` / `Overlap` variants for
  HTTP-friendly error rendering.
* `WrittenWindow` type alias to satisfy clippy + document intent.

### main.rs wiring

* New CLI flag `--enable-backfill-worker`. Off by default
  (preserves Phase 5d shadow-mode semantics). On, spawns
  `BackfillService` with `noop_reader_factory()` in v1.
* Graceful shutdown via `BackfillServiceHandle::shutdown()`.
* When the flag is set but precompute isn't enabled, logs a warn
  and skips the service (needs the schema registry).

## Determinism decision

Audit concluded that sketch-core's Rust-side live ingest path is
fully deterministic:

- CMS / CountSketch / HLL / HydraKLL use xxhash seeded by row
  index (not process-local RNG).
- Sample order is preserved through SeriesRouter → Worker into
  the per-(agg_id, group_key) accumulator.
- KLL / DDSketch sampling / bucketing is deterministic by design.
- No order-sensitive FP reductions.

Open question: sketchlib-go (DC-side) cross-language determinism.
For deployments where live goes through DC's modified-OTLP path,
the backend receives pre-built sketches. Getting backfill to
produce byte-identical outputs in that case requires a separate
Go-vs-Rust audit — tracked as follow-up. For raw-ingest
deployments (Prometheus remote write), backfill is bit-identical
today; a test in `backfill_processor.rs` locks that invariant:
same sample stream → live path's SumAccumulator and backfill's
build_backfilled_accumulator produce identical serialised bytes.

## Separation decision

User direction: "backfill functions should all be separate, not
reusing the live path." The interpretation split two notions of
"live path":

1. **Worker pipeline** (PrecomputeEngine → SeriesRouter → Worker
   → active_panes → output_sink): fully NOT reused. Backfill has
   its own service + worker + processor + output call chain.
   Zero shared state, zero shared async runtime context.

2. **`create_accumulator_updater` pure factory**: IS reused. A
   60-line stateless match statement that backfill calls through
   the `build_backfilled_accumulator` facade. The alternative
   (copy-paste the match) trades drift risk for isolation without
   any latency-isolation benefit.

Module docs document this choice. If stricter duplication is
required, `build_backfilled_accumulator` is one match-statement
swap away. Bit-identical parity test would still pass either way.

## Test plan

- [x] 2 new tests in `backfill_window_builder`: sum aggregation
  correctness, empty-samples edge case.
- [x] 8 new tests in `backfill_processor`: happy path (one-write-
  per-window), unknown agg_id handling, empty samples, end-to-end
  via BackfillWorker across 4 windows,
  **bit-identical parity test** (live-built vs backfill-built
  SumAccumulator serialise to identical bytes), and
  `create_checked`'s three branches (overlap rejection,
  boundary-at-created_at acceptance, unknown-agg rejection).
- [x] 4 new tests in `backfill_service`: drain queued job to
  Complete, reader factory failure marks job Failed, multiple
  jobs processed in job_id order, shutdown terminates loop.
- [x] 647 lib tests pass (up from 633).
- [x] `cargo clippy --workspace --all-targets --tests -- -D warnings` clean.
- [x] `cargo fmt -- --check` clean.

## What's still open

* **5f**: query path consults `windows_written_by` to distinguish
  `Backfilled` vs `Missing` coverage at query-segment level.
* **5h**: real `PrometheusReader` / `S3GorillaReader` /
  `ClickHouseReader` implementations wired into a production
  `ReaderFactory`.
* **sketchlib-go determinism audit**: separate cross-repo work to
  extend the bit-identical guarantee to DC-ingest deployments.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@zzylol
zzylol merged commit bf874e9 into main Apr 18, 2026
@zzylol
zzylol deleted the sketchdb/phase5e-backfill-execution branch April 18, 2026 16:48
zzylol added a commit that referenced this pull request Apr 18, 2026
@zzylol @claude
refactor(sketch-db): merge simple_map_store into sketch_db module tree
2d1a77d 
The sketch DB has two structurally-layered pieces that have been
living as sibling modules:
  - `src/stores/sketch_db/`     (logical layer: schemas, timelines, backfill)
  - `src/stores/simple_map_store/` (physical layer: per-key storage backend)

This sibling layout hid the dependency direction: `sketch_db`
depends on the `Store` trait implemented by `SimpleMapStore`, not
vice versa. The physical store is one concrete implementation of
the sketch DB's storage abstraction, not a peer concept.

Move `simple_map_store/` INTO `sketch_db/` as a submodule so the
module tree reflects the layering: everything sketch-DB-related,
including its physical backend, lives under one umbrella.

## What changed

- `git mv asap-query-engine/src/stores/simple_map_store → asap-query-engine/src/stores/sketch_db/simple_map_store`
- `stores::simple_map_store::*` → `stores::sketch_db::simple_map_store::*` across 20 files (16 backend + 1 bench + 3 internal self-references inside the moved directory).
- `stores/mod.rs` drops the `pub mod simple_map_store` declaration but keeps `pub use ... SimpleMapStore` re-export so `crate::stores::SimpleMapStore` still works for all callers.
- `sketch_db/mod.rs` adds `pub mod simple_map_store` + `pub use simple_map_store::SimpleMapStore`.

## What didn't change

- `crate::stores::SimpleMapStore` public path — callers that import via the top-level `stores::` continue to work unchanged.
- `Store` trait still lives at `stores::traits` — it's the neutral interface both the logical layer and the physical impl share.
- No code logic changed. No tests added or removed.

## Test plan

- [x] Pure rename, zero logic delta.
- [x] 647 lib tests pass (same count as PR #32).
- [x] `cargo clippy --workspace --all-targets --tests -- -D warnings` clean.
- [x] `cargo fmt -- --check` clean.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@zzylol zzylol mentioned this pull request Apr 18, 2026
Merged
3 tasks
zzylol added a commit that referenced this pull request Apr 18, 2026
@zzylol @claude
refactor(sketch-db): merge simple_map_store into sketch_db module tree (
3dbd3b0 
#33)

The sketch DB has two structurally-layered pieces that have been
living as sibling modules:
  - `src/stores/sketch_db/`     (logical layer: schemas, timelines, backfill)
  - `src/stores/simple_map_store/` (physical layer: per-key storage backend)

This sibling layout hid the dependency direction: `sketch_db`
depends on the `Store` trait implemented by `SimpleMapStore`, not
vice versa. The physical store is one concrete implementation of
the sketch DB's storage abstraction, not a peer concept.

Move `simple_map_store/` INTO `sketch_db/` as a submodule so the
module tree reflects the layering: everything sketch-DB-related,
including its physical backend, lives under one umbrella.

## What changed

- `git mv asap-query-engine/src/stores/simple_map_store → asap-query-engine/src/stores/sketch_db/simple_map_store`
- `stores::simple_map_store::*` → `stores::sketch_db::simple_map_store::*` across 20 files (16 backend + 1 bench + 3 internal self-references inside the moved directory).
- `stores/mod.rs` drops the `pub mod simple_map_store` declaration but keeps `pub use ... SimpleMapStore` re-export so `crate::stores::SimpleMapStore` still works for all callers.
- `sketch_db/mod.rs` adds `pub mod simple_map_store` + `pub use simple_map_store::SimpleMapStore`.

## What didn't change

- `crate::stores::SimpleMapStore` public path — callers that import via the top-level `stores::` continue to work unchanged.
- `Store` trait still lives at `stores::traits` — it's the neutral interface both the logical layer and the physical impl share.
- No code logic changed. No tests added or removed.

## Test plan

- [x] Pure rename, zero logic delta.
- [x] 647 lib tests pass (same count as PR #32).
- [x] `cargo clippy --workspace --all-targets --tests -- -D warnings` clean.
- [x] `cargo fmt -- --check` clean.

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
zzylol added a commit that referenced this pull request Apr 18, 2026
@zzylol @claude
feat(sketch-db): Phase 5h — PrometheusReader (real RawSampleReader fo…
9e34a84 
…r backfill)

First real `RawSampleReader` in-tree. Turns the `BackfillSource::Prometheus
{ url }` variant from "factory returns no-reader error" into an actual
working read path, so `--enable-backfill-worker` against a Prometheus-
backed deployment now produces real backfills instead of shadow-mode
Failed jobs.

## What's landed

`src/stores/sketch_db/prometheus_reader.rs`:

* `PrometheusReader::new(base_url) + with_step(d) + with_timeout(d)` —
  HTTP client wrapper over `/api/v1/query_range`. Step defaults to 15s
  (typical Prometheus scrape interval); overridable per-deployment.
* `impl RawSampleReader` that:
  1. Translates `LabelFilter { metric, equality }` into a PromQL
     matcher `metric{k1="v1",k2="v2"}` with stable key ordering and
     proper escape for quotes/backslashes/newlines.
  2. Issues a `GET /api/v1/query_range` with `query`, `start`, `end`,
     `step` params.
  3. Parses the `{status: "success", data: {resultType: "matrix",
     result: [{metric, values}]}}` JSON shape into `Vec<RawSample>`.
  4. Maps every failure into `RawSampleReaderError::{InvalidRange,
     Upstream, Decode}` so the worker can surface the root cause on
     `BackfillJob::error_message`.
* `ms_to_fractional_seconds` / `fractional_seconds_to_ms` helpers for
  the ms ↔ Prometheus-fractional-seconds conversion.
* `render_series_key` rebuilds `metric{labels...}` from Prometheus's
  response metric map so downstream grouping and `extract_group_key`
  see the same shape they see for live samples.

`backfill_service::default_reader_factory()`:

* Routes `BackfillSource::Prometheus { url }` to `PrometheusReader::new`.
* Returns a clear "reader for <source> not yet implemented" error for
  `S3Gorilla` / `ClickHouse` / `OtherSketch` so the controller sees
  exactly which reader is missing.

`main.rs` now uses `default_reader_factory()` under
`--enable-backfill-worker` instead of `noop_reader_factory`, so the
service actually does work against Prometheus targets.

## On determinism (§10.5)

`/api/v1/query_range` with step = scrape_interval returns one point
per step per series. For deployments whose live ingest is Prometheus
remote write (backend-side sketch-core construction), this gives the
same sample sequence live saw, so the bit-identical parity test from
PR #32 still holds. For deployments whose live ingest goes through
DC's OTLP sketch-building path, approximate equivalence holds; exact
bit-identical would need `/api/v1/read` + sketchlib-go parity work.

Module doc flags that `/api/v1/read` (the protobuf remote-read
protocol) is the upgrade path if we ever need sub-scrape-interval
resolution; trait contract is unchanged.

## Test plan

- [x] 8 unit tests: `build_promql` ordering + escape + bare-metric,
  `render_series_key` ordering + bare + fallback, fractional-seconds
  round-trip, inverted-range → `InvalidRange`.
- [x] 8 integration tests against a spawned axum mock Prometheus:
  happy-path multi-series parse + param propagation, PromQL matcher
  synthesis, HTTP 5xx → Upstream, Prometheus `status: error` →
  Upstream, malformed JSON → Decode, ingest-order preservation
  per series, empty result, wrong `resultType` → Decode.
- [x] 677 lib tests pass (up from 661).
- [x] clippy `--workspace --all-targets --tests -- -D warnings` clean.
- [x] `cargo fmt -- --check` clean.

## What this unblocks

A Prometheus-backed deployment can now:

1. Create a backfill job via `POST /api/v1/db/backfill` with
   `source: {Prometheus: {url: "http://prom:9090"}}`.
2. Start the backend with `--enable-backfill-worker`.
3. Watch the job transition `Queued → Running → Complete` in
   `/api/v1/db/backfill/jobs/<id>`, with per-window precomputes
   written to the store bit-identically to what live ingest would
   have produced.

`S3Gorilla` and `ClickHouse` readers are the remaining Phase 5h-2 /
5h-3 work; `OtherSketch` (lossless widening from an existing sketch)
is Phase 5i.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@zzylol zzylol mentioned this pull request Apr 18, 2026
Merged
4 tasks
zzylol added a commit that referenced this pull request Apr 18, 2026
@zzylol @claude
feat(sketch-db): Phase 5h — PrometheusReader (real RawSampleReader fo…
e932447 
…r backfill) (#35)

First real `RawSampleReader` in-tree. Turns the `BackfillSource::Prometheus
{ url }` variant from "factory returns no-reader error" into an actual
working read path, so `--enable-backfill-worker` against a Prometheus-
backed deployment now produces real backfills instead of shadow-mode
Failed jobs.

## What's landed

`src/stores/sketch_db/prometheus_reader.rs`:

* `PrometheusReader::new(base_url) + with_step(d) + with_timeout(d)` —
  HTTP client wrapper over `/api/v1/query_range`. Step defaults to 15s
  (typical Prometheus scrape interval); overridable per-deployment.
* `impl RawSampleReader` that:
  1. Translates `LabelFilter { metric, equality }` into a PromQL
     matcher `metric{k1="v1",k2="v2"}` with stable key ordering and
     proper escape for quotes/backslashes/newlines.
  2. Issues a `GET /api/v1/query_range` with `query`, `start`, `end`,
     `step` params.
  3. Parses the `{status: "success", data: {resultType: "matrix",
     result: [{metric, values}]}}` JSON shape into `Vec<RawSample>`.
  4. Maps every failure into `RawSampleReaderError::{InvalidRange,
     Upstream, Decode}` so the worker can surface the root cause on
     `BackfillJob::error_message`.
* `ms_to_fractional_seconds` / `fractional_seconds_to_ms` helpers for
  the ms ↔ Prometheus-fractional-seconds conversion.
* `render_series_key` rebuilds `metric{labels...}` from Prometheus's
  response metric map so downstream grouping and `extract_group_key`
  see the same shape they see for live samples.

`backfill_service::default_reader_factory()`:

* Routes `BackfillSource::Prometheus { url }` to `PrometheusReader::new`.
* Returns a clear "reader for <source> not yet implemented" error for
  `S3Gorilla` / `ClickHouse` / `OtherSketch` so the controller sees
  exactly which reader is missing.

`main.rs` now uses `default_reader_factory()` under
`--enable-backfill-worker` instead of `noop_reader_factory`, so the
service actually does work against Prometheus targets.

## On determinism (§10.5)

`/api/v1/query_range` with step = scrape_interval returns one point
per step per series. For deployments whose live ingest is Prometheus
remote write (backend-side sketch-core construction), this gives the
same sample sequence live saw, so the bit-identical parity test from
PR #32 still holds. For deployments whose live ingest goes through
DC's OTLP sketch-building path, approximate equivalence holds; exact
bit-identical would need `/api/v1/read` + sketchlib-go parity work.

Module doc flags that `/api/v1/read` (the protobuf remote-read
protocol) is the upgrade path if we ever need sub-scrape-interval
resolution; trait contract is unchanged.

## Test plan

- [x] 8 unit tests: `build_promql` ordering + escape + bare-metric,
  `render_series_key` ordering + bare + fallback, fractional-seconds
  round-trip, inverted-range → `InvalidRange`.
- [x] 8 integration tests against a spawned axum mock Prometheus:
  happy-path multi-series parse + param propagation, PromQL matcher
  synthesis, HTTP 5xx → Upstream, Prometheus `status: error` →
  Upstream, malformed JSON → Decode, ingest-order preservation
  per series, empty result, wrong `resultType` → Decode.
- [x] 677 lib tests pass (up from 661).
- [x] clippy `--workspace --all-targets --tests -- -D warnings` clean.
- [x] `cargo fmt -- --check` clean.

## What this unblocks

A Prometheus-backed deployment can now:

1. Create a backfill job via `POST /api/v1/db/backfill` with
   `source: {Prometheus: {url: "http://prom:9090"}}`.
2. Start the backend with `--enable-backfill-worker`.
3. Watch the job transition `Queued → Running → Complete` in
   `/api/v1/db/backfill/jobs/<id>`, with per-window precomputes
   written to the store bit-identically to what live ingest would
   have produced.

`S3Gorilla` and `ClickHouse` readers are the remaining Phase 5h-2 /
5h-3 work; `OtherSketch` (lossless widening from an existing sketch)
is Phase 5i.

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
zzylol added a commit that referenced this pull request May 11, 2026
@zzylol zz_y @claude
refactor(controller): retire algebra/+planner/+config/+query_language…
0bda240 
…/+analyzer.rs (#130)

Per design.md §5 target layout. Restructure controller/src/ from the
historic layout (algebra/ + planner/ + config/ as ill-defined catch-alls
mixed with stage_split.rs + analyzer.rs scattered top-level files) into
the design.md modular split: query_parser/ (L1) → language_logical_plan/
(L2) → intent_algebra/ (L3) → sketch_algebra/ (L4 IR) → optimizer/ (L4
framework) → physical/ (L5 framework) → emit/ (L5 emitters) → pipeline.rs
(L1→L5 driver).

This is Step 2b/c/d/e/f from the architectural cleanup chain that started
with #129 (Step 2a — capability consolidation).

## Old → new path mapping

| Old | New |
|---|---|
| `algebra/expr.rs` | `intent_algebra/legacy_expr.rs` (kept; 91 consumers — unification is a follow-up) |
| `algebra/lower.rs` | `intent_algebra/legacy_lower.rs` |
| `algebra/{directory,physical,allocator,plan}.rs` | `physical/{sketch_catalog,planner,allocator,plan}.rs` |
| `algebra/optimizer.rs` | `optimizer/engine.rs` |
| `planner/{cost_model,delta,online,pareto,tco,wire_cost}.rs` | `optimizer/cost/{mod,delta,online,pareto,tco,wire}.rs` |
| `planner/rules.rs` | `optimizer/rules/mod.rs` |
| `planner/baseline_planner.rs` | `optimizer/baseline.rs` |
| `planner/stage_split.rs` | `physical/stage_split.rs` |
| `analyzer.rs` | `pipeline.rs` |
| `stage_split/` | `physical/colored_dag/` (with colored_dag.rs → dag.rs) |
| `query_language/` | `query_parser/language/` |
| `config/workloads.rs` | `workload.rs` |
| `config/{agent,backend,asapquery_backend,precompute,stage_config,stage_config_otap,stage_config_telegraf}.rs` | `emit/{agent,backend,asapquery_backend,precompute,stage_config,otap,telegraf}.rs` |
| (new) | `physical/topology.rs`, `emit/trait_def.rs`, `optimizer/trait_def.rs`, `deployment_model.rs` |

## Pragmatic concessions (documented TODOs)

- **`algebra/expr.rs` (1,230 lines) → `intent_algebra/legacy_expr.rs`**:
  the brief assumed only one stray consumer of `algebra::expr::AggIntent`
  existed, but reality is ~91 sites across query_parser, language_logical_plan,
  planner, algebra, and config use the legacy `QueryExpr` / `AggIntent` /
  `WindowSpec` IR. Treating these as "duplicates to delete" would have
  required rewriting every consumer in one PR. Moved the file with a
  doc-comment marking it as legacy IR pending unification with the
  canonical `intent_algebra::{agg_intent,query_expr,schema}` types.
  Same for `algebra/lower.rs` → `intent_algebra/legacy_lower.rs`.

- **`config/stage_config.rs` (3,020 lines)** moved whole as
  `emit/stage_config.rs` rather than split into the design.md-prescribed
  `emit/{opamp,streaming_config,inference_config}.rs`. The monolith
  mixes OTel-collector YAML emit, ASAPQuery-backend JSON emit,
  storage-routing JSON emit, and many shared internals; a clean split
  needs ownership reorganisation, not file renames.

- **Back-compat module aliases** in `lib.rs` (`pub use intent_algebra::
  legacy_expr as algebra;` etc.) — `controller/src/main.rs` references
  `crate::stage_split::*`, `crate::algebra::*`, `crate::planner::*`,
  `crate::config::*`, `crate::analyzer::*` (the bin crate, not the lib).
  Keeping shims in `lib.rs` lets main.rs continue compiling without
  per-line edits. Remove the shims once main.rs migrates.

- **`hll_accuracy()` / `countmin_accuracy()`** moved from `legacy_expr.rs`
  → `sketch_algebra/capability.rs` (their structural home — sketch-family
  error bounds). `legacy_expr.rs` retains thin `pub use` re-exports so
  in-file callers (e.g. `AggIntent::default_cardinality`) keep
  compiling.

## docs/design.md updates

- §3 layer table — rows rewritten with new `controller/src/*` paths +
  italicised `*Refactor 2026-05 absorbed ...*` notes.
- §5 Target repo layout — prepended blockquote: multi-crate split
  deferred until ≥2 deployment models ship; lookup table mapping
  `crates/core/*` targets to current single-crate paths.
- §6 Core crate details — prepended blockquote: 16-row lookup table.
- **§16 NEW** — ADR section. §16.1 documents PR #129's capability
  consolidation; §16.2 documents this PR's retirement plan with the
  full mapping table and four TODOs.

## Build + test

- `cargo build --release -p controller` → clean (4 warnings, pre-existing)
- `cargo build --release -p query_engine_rust` → clean (3 warnings)
- `cargo test -p controller --lib` → **633/633 pass** (unchanged from #129)
- `cargo test -p query_engine_rust --lib -- engines::warm_tier` → **13/13 pass**

## Diff: 55 files, +568 / -309 (file renames + import-path rewires)

## Follow-ups

1. Migrate consumers of `intent_algebra::legacy_expr::*` onto canonical
   `intent_algebra::{query_expr,agg_intent,schema}` types; delete the
   `legacy_*.rs` files.
2. Split `emit/stage_config.rs` (3,020 lines) per design.md §5.
3. Implement the placeholder traits (`OptimizerRule`, `PlanEmitter`,
   `DeploymentModelRegistry`). Existing free-function emitters + rule
   loops keep behaviour stable.
4. Migrate `controller/src/main.rs` off the back-compat module aliases
   in `lib.rs` (`crate::algebra::*` etc.); remove the shims.
5. (Separate task #32) `sketch_algebra/` touch-up: drop
   `AggIntent::HistogramQuantile` from semantic IR; map Min/Max to
   QuantileApprox; consolidate SketchCapability vs SketchCapabilities;
   rename params.rs → sketch_params.rs; add `FrequencyEstimate`
   capability variant + `CountSketchWithHeap` handle.

Co-authored-by: zz_y <zz_y@node0.zz-y-304941.softmeasure-pg0.clemson.cloudlab.us>
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
zzylol added a commit that referenced this pull request May 11, 2026
@zzylol zz_y @claude
refactor(controller): trait impls + main.rs alias removal (PR #130 TO…
0bfc332 
…DO 3+4) (#132)

Two of the four PR #130 follow-up TODOs land here. The other two are
blocked by architectural reality (see below).

## TODO 3 — Placeholder traits made functional ✅

### `OptimizerRule` trait
- `controller/src/optimizer/trait_def.rs`: extended trait with `name()`,
  `category()`, `apply()`. Added two new `RuleCategory` variants
  (`Cse`, `Decorrelate`) beyond the original four.
- `controller/src/optimizer/engine.rs`: per-rule `OptimizerRule` impls
  for all 12 engine rules (delegating to the existing `RewriteRule`
  free fns). New `default_rules_as_optimizer_rules()` helper returns
  `Vec<Box<dyn OptimizerRule>>` for callers that want the trait surface.
- `controller/src/optimizer/mod.rs`: blanket impl for every
  `sketch_algebra::rules::Rule` (lives outside sketch_algebra/ to
  respect Step #32's territory boundary).

### `PlanEmitter` trait
- `controller/src/emit/trait_def.rs`: kept the associated-type shape;
  added 4 concrete impls — `OpampEmitter`, `OpampGatewayEmitter`,
  `StreamingConfigEmitter`, `InferenceConfigEmitter` — each wraps the
  existing free-function emitter. `InferenceConfigInput<'a>` bundles
  per-tenant routing inputs (tenant id + metric plans + Mode-3
  metrics). `emit_borrowed` variant avoids forcing `'static` on the
  trait-method path.

### `DeploymentModelRegistry`
- `controller/src/deployment_model.rs`: extended from stub
  `HashMap<DeploymentModelId, ()>` to a real `HashMap<…, DeploymentModel>`
  where `DeploymentModel` owns `(id, rules: Vec<Box<dyn OptimizerRule>>,
  emitters: EmitterSet)`. `Default::default()` registers
  `asaplifecycle` with the engine's 12-rule library + the 4 demo
  emitter names so `pipeline::run_pipeline` doesn't need manual
  registration.

## TODO 4 — main.rs migrated off back-compat aliases ✅

Rewrote all 52 reference sites in `controller/src/main.rs`:
- `algebra::*` → `optimizer::engine::*` / `physical::*` /
  `intent_algebra::legacy_expr::*` (depending on context)
- `analyzer::*` → `pipeline::*`
- `config::*` → `emit::*` / `workload::*`
- `planner::*` → `optimizer::*` / `physical::stage_split` /
  `optimizer::cost::online as online_cost_model`
- `stage_split::*` → `physical::colored_dag::*`

Removed all back-compat shims from `lib.rs`:
- `pub use emit as config`
- `pub use pipeline as analyzer`
- `pub mod algebra { … }`
- `pub use physical::colored_dag as stage_split`
- `pub mod planner { … }`

One leftover internal ref (`crate::planner::rules::bind_workload_typed`
inside `emit/mod.rs::collect_metric_to_family`) retargeted to
`crate::optimizer::rules::bind_workload_typed`.

`lib.rs` shrank from 184 → 95 lines.

## TODO 1 — legacy_expr migration: BLOCKED 🚫

The canonical `intent_algebra::{query_expr,agg_intent,schema}` is NOT a
structural superset of the legacy `intent_algebra::legacy_expr`. The
canonical `QueryExpr` has 5 variants (`Scan`, `Window`, `Aggregate`,
`LetBinding`, `Ref`); legacy has ~26 (`Source`, `Filter`, `Project`,
`Aggregate`, `Window`, `SketchAgg`, `WindowedAgg`, `Partition`, `Dedup`,
`TopK`, `Merge`, `Join`, `JoinSketch`, `SetOp`, `Sort`, `Limit`,
`Subquery`, `LetBinding`, `WindowFunc`, `HistogramQuantile`,
`PromQLSubquery`, `BinaryOp`, `Ref`) plus a full `ScalarExpr` IR
(Column, Literal, BinaryOp, UnaryOp, FunctionCall, ScalarSubquery,
InList, InSubquery, Between, IsNull, Case, Cast, VectorBinaryOp).

The canonical IR's own module docs flag this: *"the full design.md list
is larger; they are deferred to follow-up phases as the planner grows
consumers for them"*.

Migrating consumers requires first GROWING the canonical IR to absorb
these variants — a substantial separate refactor, not a follow-up
cleanup. 50+ consumer sites in query_parser/, language_logical_plan/,
optimizer/engine.rs, physical/{allocator,planner,plan,stage_split}.rs
depend on the legacy variant set.

Recommend retitling the follow-up as "grow canonical L3 to absorb
legacy variants" rather than "migrate consumers". The legacy_expr
module stays in place pending that work.

## TODO 2 — stage_config.rs split: DEFERRED 🟡

The 3,020-line `emit/stage_config.rs` monolith mixes OTel YAML emit,
backend JSON emit, and storage-routing emit with intricately shared
helpers (`sketch_kind_to_processor_name`, `build_otlp_exporter`,
`sketch_kind_tag`, …) and 1,580 lines of tightly-interleaved tests
(line 1441 onward).

The wrapper structs from TODO 3 above already provide the polymorphic
`PlanEmitter` surface (`OpampEmitter` / `StreamingConfigEmitter` /
`InferenceConfigEmitter`), so the file-level split is no longer
load-bearing for the trait work. Recommend addressing as a separate
PR with dedicated time budget if/when the monolith becomes a
review-bottleneck.

## Build + test

- `cargo build --release -p controller` — clean
- `cargo build --release -p query_engine_rust` — clean
- `cargo test --release -p controller --lib` — **646 passed** (was 633; +13 trait tests)
- `cargo test --release -p controller --bin controller` — **27 passed**
- `cargo test --release -p query_engine_rust --lib -- engines::warm_tier` — **13 passed**

## Caveat — `controller/src/store/`

PR #130 left `lib.rs` declaring `pub mod store;` but `controller/src/store/`
is in `.gitignore` (line ~10 of repo root .gitignore — the broad
`store/` pattern). The directory is untracked; this needs a separate
decision on whether it should be tracked or remain a runtime artifact.
The build works because the directory exists locally on the dev box;
fresh clones would fail. Not in scope for this PR; flagging as
follow-up.

## Diff: 8 files, +785 / -207

Co-authored-by: zz_y <zz_y@node0.zz-y-304941.softmeasure-pg0.clemson.cloudlab.us>
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@zzylol