refactor(data_plane): rename engine crate + flatten module layout + rename SimpleEngine→ASAPQueryEngine by zzylol · Pull Request #147 · ProjectASAP/ASAPQuery-backend

zzylol · 2026-05-12T15:29:55Z

Summary

Reorganize the engine crate so the directory layout reflects the data-plane / control-plane split, and finish PR #137's storage-vs-engine naming work by renaming the engine struct itself.

asap-query-engine/ → data_plane/ (folder + cargo package + binary name); controller/ is the sibling "control plane"
Internal flatten under data_plane/src/: four conceptual buckets — drivers/, stores/, precompute_engine/, query_engines/ — plus utils/
SimpleEngine struct → ASAPQueryEngine, matching the new query_engines/asap_query_engine/ folder

Folder map

Before	After
`asap-query-engine/` (crate `query_engine_rust`)	`data_plane/` (crate `data_plane`)
`src/data_model/`	`src/stores/schema/`
`src/precompute_operators/`	`src/precompute_engine/operators/`
`src/engines/`	`src/query_engines/`
`src/engines/simple/`	`src/query_engines/asap_query_engine/`
`src/engines/gorilla/thanos_forward.rs`	`src/query_engines/thanos_query_engine/engine.rs`
`src/engines/gorilla/{engine,tests}.rs`	`src/query_engines/gorilla_query_engine/` (deprecation banner — slated for Phase δ removal once Path A2 verified)
`src/engines/gorilla/{store,postings,s3_cost}.rs`	`src/stores/gorilla_object_store/`
`src/engines/prometheus/`	`src/query_engines/prometheus_query_engine/`
`src/routing/`	`src/query_engines/routing/`
`src/drivers/controller_client.rs` + `src/drivers/query/controller_client.rs`	`src/drivers/controller_client/{config_fetcher.rs, miss_notifier.rs, mod.rs}`
`src/stores/promsketch_store/`	deleted (was commented out)

Test plan

cargo check --workspace clean
cargo test -p data_plane --lib — 807 passed / 2 pre-existing failures / 7 ignored
cargo test -p controller --lib — 710/710
cargo test -p controller --bins — 27/27
Follow-up: external tests/e2e_modified_otlp_sketch_path.rs has pre-existing proto-struct-field-removed compile errors unrelated to this PR; not addressed here.

Notes for reviewers

Old engines::{gorilla,simple,prometheus}::* import paths are kept compiling via cross-module re-exports inside each new *_query_engine/mod.rs (no churn for downstream test modules in drivers/query/servers/http.rs).
The deprecated gorilla_query_engine still re-exports the gorilla-object-store types so external integration tests under tests/ and the http-handler's test module compile without churn. Those re-exports come out when Phase δ deletes the legacy in-process engine.
Docker image name ghcr.io/projectasap/asap-query-engine in benchmarks/docker-compose.yml is left alone — registry concern, separate from the folder rename. Flag in review if you want it renamed too.
This branch is based on a pre-PR-refactor(query-engine): split engine and storage naming #137 / pre-PR-chore(asap-query-engine): remove 3 persistence tests dependent on the removed datafusion serde #146 commit; merge to main may surface a few resolvable conflicts inside files that refactor(query-engine): split engine and storage naming #137 also touched.

🤖 Generated with Claude Code

…op of #137 Builds on top of PR #137's storage/engine naming split. Where #137 stopped at `query-engines/` + kept `data_model/`, `precompute_operators/`, `routing/`, two `controller_client.rs` files, and the `SimpleEngine` type, this PR finishes the reorg: - Crate rename: `asap-query-engine/` → `data_plane/` (folder + cargo package + binary name); the controller sibling is the "control plane" - `query-engines/` → `query_engines/` (snake_case) - Per-engine `_engine` suffix: - `asap_query/` → `asap_query_engine/` - `thanos_query/` → `thanos_query_engine/` - `prometheus/` → `prometheus_query_engine/` - `warm_tier/` promoted from `asap_query_engine/warm_tier/` to a top-level `query_engines/warm_tier/` (it's shared infra, not asap_query-specific) - `data_model/` → `stores/schema/` (storage schema types belong with storage) - `precompute_operators/` → `precompute_engine/operators/` (accumulator impls belong under the engine that orchestrates them) - `routing/` → `query_engines/routing/` (engine dispatcher, scoped to query path) - `drivers/controller_client.rs` + `drivers/query/controller_client.rs` co-located into `drivers/controller_client/{config_fetcher.rs, miss_notifier.rs, mod.rs}` - `stores/promsketch_store/` deleted (was commented out of the public API; not referenced) - `SimpleEngine` struct renamed to `ASAPQueryEngine` (was a type alias on main); compat alias removed Test counts after the reorg match origin/main: - data_plane lib: 804 passed / 2 pre-existing failures (schema_timeline_dispatch_tests, documented in controller_todo §5) / 4 ignored - controller lib: 710/710; bins: 27/27 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

…map_store → sketch_store The `legacy/` submodule under `simple_map_store/` was only referenced by `benches/simple_store_bench.rs`, which profiles the legacy lock-strategy implementations (`LegacySimpleMapStoreGlobal` / `LegacySimpleMapStorePerKey`). No lib / bin / test path consumes either; remove both as a unit: - Delete `data_plane/src/stores/sketch_db/simple_map_store/legacy/` - Delete `data_plane/benches/simple_store_bench.rs` - Drop the matching `[[bench]]` entry from `data_plane/Cargo.toml` - Drop `pub mod legacy;` from `simple_map_store/mod.rs` Rename the surviving physical store to a clearer name: - Folder: `stores/sketch_db/simple_map_store/` → `stores/sketch_db/sketch_store/` - Types: `SimpleMapStore` → `SketchStore`, `SimpleMapStoreGlobal` → `SketchStoreGlobal`, `SimpleMapStorePerKey` → `SketchStorePerKey`, `SimpleMapStorePersistenceConfig` → `SketchStorePersistenceConfig` Test counts unchanged from origin/main: - data_plane lib: 804 passed / 2 pre-existing failures / 4 ignored - controller lib: 710/710; bins: 27/27 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

…db, rename stores/schema → stores/types Three independent cleanups on top of the prior reorg: 1) Delete `query_engines/prometheus_query_engine/` (the Phase-ε.2 HTTP-forwarder to Prometheus's `/api/v1/query`). No production call sites consume it; the http-handler's own `--forward-unsupported-queries` fallback is unaffected. The `StorageBackend::PrometheusRemote` routing identifier stays in `crates/asap_types` so controller-emitted configs targeting it surface a clean 503 NoEngineRegistered (already the current behaviour when the env var was unset). 2) Group `sketch_db/` files by concern into 4 subdirs: sketch_db/ accuracy.rs single-file concern, stays flat metrics.rs Prometheus exposition, stays flat schema/ per-`agg_id` lifecycle mod.rs (was schema.rs) eviction.rs (was schema_eviction.rs) index/ two-level sid index mod.rs (was sketch_index.rs) epoch_columnar.rs (storage layer used by SketchIndex) backfill/ job lifecycle + worker pool + readers mod.rs (was backfill.rs) processor.rs service.rs window_builder.rs worker.rs prometheus_reader.rs (BackfillSource::Prometheus impl) raw_sample_reader.rs (RawSampleReader trait + mock) store/ physical Store impl mod.rs (SketchStore enum, was sketch_store/) common.rs global.rs per_key.rs persistence/ tightly cohesive: cache + flusher + manifest + part format + recovery + source 3) Rename `stores/schema/` → `stores/types/` to remove the name clash with the new `sketch_db/schema/`. The two are unrelated concerns: `stores/types/` holds wire-format / config / data-model types used cross-module (drivers, precompute_engine, query_engines, AND sketch_db); `sketch_db/schema/` holds per-`agg_id` lifecycle (AggSchema, AggStatus, SchemaRegistry). Also delete the 6 tiny `pub use asap_types::X` shim files (aggregation_config.rs, aggregation_reference.rs, inference_config.rs, promql_schema.rs, query_config.rs, streaming_config.rs) — the equivalent re-exports are now consolidated in `stores/types/mod.rs`. Test counts: - data_plane lib: 792 passed / 2 pre-existing failures / 4 ignored (down from 804 due to deletion of 12 unit tests inside the removed prometheus_query_engine/forward.rs; documented failures unchanged) - controller lib: 710/710; bins: 27/27 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

…nsolidate MutableEpoch dedup, write phase 5 plan doc Three follow-ups: 1) Move `query_engines/warm_tier/` back into `query_engines/asap_query_engine/warm_tier/`. The earlier reorg promoted it on the assumption it was shared infra; audit shows only `asap_query_engine::engine` consumes it. PR #137's original nesting was correct. 2) Consolidate the duplicate `MutableEpoch` / `SealedEpoch` / `InternTable` implementations. Before: ~290 LOC of identical storage logic in `sketch_db/store/common.rs` (non-generic, payload = `Arc<dyn AggregateCore>`) and `sketch_db/index/epoch_columnar.rs` (generic over `P`). After: `store/common.rs` is a 50-LOC alias shim (`pub type MutableEpoch = epoch_columnar::MutableEpoch<Arc<dyn AggregateCore>>`, etc.); the generic version is the single implementation. Required adapter methods added on the generic side, gated on `P: Clone` where the legacy semantics needed owning copies: - `MutableEpoch::range_query_into_grouped` — emits the `HashMap<LabelValuesId, Vec<(TimestampRange, P)>>` shape `SketchStore` callers consume. - `MutableEpoch::exact_query_owned` — `Option<Vec<(LabelValuesId, P)>>` for cross-lock-boundary handoff. - `MutableEpoch::remove_windows` — ReadBased / CircularBuffer cleanup primitive. - `MutableEpoch::time_bounds` / `seal` — convenience wrappers. - Same triplet on `SealedEpoch` plus `distinct_window_count` and `unique_windows`. Fixed a latent UB in `SealedEpoch::from_mutable`: previously used `MaybeUninit::zeroed().assume_init()` + `mem::forget` to drain the payload column, which is undefined behavior for any `P` with non-trivial Drop. Replaced with safe `into_iter().zip(...)` — same O(M) cost, works for arbitrary `P` (including `Arc<dyn AggregateCore>`). Made `SealedEpoch.entries` public to match the legacy access pattern (a handful of call sites in `store/{global,per_key}.rs` read it directly for diagnostics + persistence flush). 3) Wrote `data_plane/docs/phase5-unification-plan.md` covering the three in-flight Phase-5 migrations: - M1: `AggSchema` → `SketchInstanceMetadata` (lifecycle fold) - M2: `aggregation_id` → `sid` (data-path identifier) - M3: legacy `Arc<dyn AggregateCore>` payload type retires File-by-file phases A–E, ordering, dependencies on the analyzer chain α-ε and Step Z legacy_expr, and a ~5-day estimate. Test counts unchanged: - data_plane lib: 792 passed / 2 pre-existing failures / 4 ignored - controller lib: 710/710; bins: 27/27 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

`CleanupPolicy::ReadBased` was the eviction policy where windows are dropped after their `read_count` reaches a threshold. The mechanism was outdated and not used in any production deployment — every production call site passes `CleanupPolicy::CircularBuffer` or `NoCleanup`. The remaining surface was test-only + the example YAML. Removed surface: **asap_types crate:** - `CleanupPolicy::ReadBased` variant + its Display / FromStr branches - `AggregationConfig::read_count_threshold` field (also dropped from `AggregationConfig::new` + `from_yaml_data` signatures and the JSON serialize / deserialize paths — wire-format field `readCountThreshold` no longer emitted) - `AggregationReference::read_count_threshold` field + `with_read_count_threshold` constructor - `read_based` parsing in `InferenceConfig::parse_cleanup_policy` **data_plane crate:** - `read_counts: HashMap<...>` from `SketchStoreGlobal`'s `StoreData` and from `SketchStorePerKey`'s `StoreKeyData` - `read_count_threshold` parameter on `cleanup_old_aggregates` and `BatchConfig` - `ReadBased` match-arm in both store backends - `cleanup_read_based` helper on `StoreKeyData` - Per-query read-count update logic in range/exact query paths - Read-count purge logic in epoch eviction - `read_counts_len` field on `AggregationDiagnostic`; matching `[MEMORY_DIAG]` log line in `main.rs` **Tests:** - Two contract tests: `test_cleanup_read_based_evicts_after_threshold_reads`, `test_cleanup_read_based_unread_window_is_retained` - 23 `AggregationConfig::new` call sites updated (the 15th positional `read_count_threshold` arg removed across data_plane + crates + tests + integration tests) **Docs / example:** - `data_plane/examples/promql/inference_config.yaml` switched from `read_based` to `circular_buffer`, `read_count_threshold: 1` lines stripped - `sketch_db/store/INDEX_DESIGN.md` updated to drop the read_counts field, the ReadBased complexity row, and the ReadBased policy section - Stale doc-comment references cleaned in store/global.rs, store/per_key.rs, index/epoch_columnar.rs Tests still match the post-PR baseline: - data_plane lib: 792 passed / 2 pre-existing failures / 4 ignored - controller lib: 710/710; bins: 27/27 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

…eaming-engine path **precomputed_output.rs cleanup (~470 LOC removed):** Deleted commented-out and otherwise-dead code: - 7 commented-out serialization / deserialization methods (serialize_to_json_with_precompute, deserialize_from_bytes, deserialize_from_json, deserialize_from_json_with_precompute, deserialize_from_bytes_with_precompute_and_type, create_precompute_from_json) - The entire commented-out test module (references long-removed signatures like `PrecomputedOutput::new(_, _, key, config)` from before AggregationConfig was lifted out of the struct) - `// pub config: AggregationConfig` field comments + stale TODO Deleted live but unused surface: - `deserialize_from_bytes_with_precompute` — `Not implemented` stub; the only call site was a commented-out branch in kafka.rs that no longer exists - `impl SerializableToSink for PrecomputedOutput` — the trait is consumed only for accumulators in the workspace; this impl was never invoked Kept (live): - `start_timestamp`, `end_timestamp`, `key`, `aggregation_id` - `new()` + `get_freshness_debug_string()` Kept and rewrote doc-comment for honesty: - `origin: Origin` + `Origin { Native, Backfilled { job_id } }` + `new_backfilled()`. Backfill is a real subsystem under `sketch_db/backfill/`; the field is the provenance hook for it. Read-side consumers (HTTP listing, coverage UI, audit logs) belong to backfill scope and don't exist yet — doc-comment now states that honestly rather than promising "Phase 5f-b consults this tag" in present tense. **Arroyo streaming-engine path deleted (workspace-wide):** Background: `StreamingEngine::Arroyo` was the alternate ingest mode where a Kafka consumer parsed Arroyo-format JSON (gzip-compressed sketch bytes + JSON metadata). It was never selected in any production deployment — `--streaming-engine=precompute` is the only selection, and grep confirmed zero `StreamingEngine::Arroyo` use sites outside the enum definition itself. Removed: - `data_plane/src/drivers/ingest/kafka.rs` — the Kafka consumer (its only purpose was Arroyo JSON parsing). ~400 LOC. - `KafkaConsumer*` re-exports in `drivers/ingest/mod.rs`, `drivers/mod.rs`, and `lib.rs` - `precomputed_output.rs::deserialize_from_json_arroyo` + `create_precompute_from_bytes` factory (only called from kafka.rs and from the three deleted worker.rs Arroyo tests below) - Three `test_arroyosketch_*` tests in `precompute_engine/worker.rs` - `StreamingEngine` enum (only variants were Arroyo, Precompute — with Arroyo gone the enum was a one-variant tautology) and `InputFormat` enum (only consumed by the Kafka consumer) - `--streaming-engine`, `--kafka-topic`, `--kafka-broker`, `--input-format`, `--decompress-json` CLI flags from `main.rs` - The `kafka_handle` setup + shutdown in `main.rs`; the `if enable_precompute { ... } else { ... }` branch collapsed to the precompute-always-enabled body - `rdkafka = "0.34"` dep from `data_plane/Cargo.toml` Preserved (these are not Arroyo-the-streaming-engine): - `*_arroyo` suffixed methods on accumulators (`SumAccumulator::deserialize_from_bytes_arroyo` etc.) — the "Arroyo" name there refers to the historical sketch byte format, which is the production sketch serialization used by warm_tier query path + the persistence read-back. Renaming those is a separate concern. **Tests:** - data_plane lib: 789 passed / 2 pre-existing failures / 4 ignored (down from 792 due to the 3 deleted `test_arroyosketch_*` tests) - controller lib: 710/710; bins: 27/27 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

ASAPCollector emits sketches in the canonical prost-encoded `SketchEnvelope` proto from `asap_sketchlib`. The data-plane consumes it via `SketchEnvelopeAccumulator::from_proto_bytes` (drivers/ingest/otel.rs) and `edge_runtime_adapter::reconstruct_via_runtime` (for DDSketch / KLL via the shared `asap-precompute-rs` runtime, which is what asap-precompute-rs itself uses internally). The `*_arroyo` accumulator methods (`SumAccumulator::deserialize_from_bytes_arroyo` and friends) decoded a different format — MessagePack via rmp_serde, inherited from the deleted Arroyo streaming engine path. Their only remaining callers were their own files' round-trip unit tests; the production read-back path through `accumulator_serde` was removed in PR #123 and `snapshot_sealed_epoch` is now a TODO stub returning `Ok(None)` pending the SketchIndex-backed refactor. Removed: - 13 `*_arroyo` method blocks across 9 accumulator files (deserialize_from_bytes_arroyo / serialize_to_bytes_arroyo on: sum, multiple_sum, count_min_sketch, count_min_sketch_with_heap, set_aggregator, delta_set_aggregator, hydra_kll, datasketches_kll, multiple_increase) - 5 round-trip unit tests that exercised those methods - `data_plane/src/utils/precompute_dumper.rs` — only consumer was the deleted KafkaConsumer; CLI flag `--dump-precomputes` (which fed `dump_output_dir` into the now-deleted KafkaConsumerConfig) also removed - `pub mod precompute_dumper;` + glob re-export from utils/mod.rs - `rmp-serde = "1.1"` dep from data_plane/Cargo.toml (no remaining consumers) Doc-comments updated to be honest about the persistence layer's current state (sketch_bytes are opaque; the legacy `accumulator_serde::deserialize_accumulator` callers are gone; `snapshot_sealed_epoch` returns `Ok(None)` until the SketchIndex refactor lands). Tests: data_plane lib 784 → 782 passed (-5 round-trip + -2 PrecomputeDumper tests = -7; new total 782). 2 pre-existing failures unchanged. controller lib 710/710; bins 27/27. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

zzylol force-pushed the refactor/data-plane-reorg branch from e6cd836 to 763c4db Compare May 12, 2026 15:45

zzylol and others added 6 commits May 12, 2026 09:51

zzylol merged commit 43570dc into main May 12, 2026

zzylol deleted the refactor/data-plane-reorg branch May 12, 2026 21:35

Navigation Menu

refactor(data_plane): rename engine crate + flatten module layout + rename SimpleEngine→ASAPQueryEngine - #147

refactor(data_plane): rename engine crate + flatten module layout + rename SimpleEngine→ASAPQueryEngine#147
zzylol merged 7 commits into
mainfrom
refactor/data-plane-reorg

zzylol commented May 12, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

zzylol commented May 12, 2026

Summary

Folder map

Test plan

Notes for reviewers

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant