Skip to content

refactor: SketchIndex on epoch-columnar storage (refactor #2) - #121

Merged
zzylol merged 1 commit into
mainfrom
refactor/sketch-index-epoch-columnar
May 10, 2026
Merged

zzylol merged 1 commit into
mainfrom
refactor/sketch-index-epoch-columnar

Conversation

@zzylol

@zzylol zzylol commented May 10, 2026

Copy link
Copy Markdown
Contributor

Summary

Refactor #2 in the chain after #120 (which scaffolded SketchIndex with naive HashMap<u64, Vec<SketchTimeSeries>> + BTreeMap<TimestampMs, …> per series). That naive shape collapsed all the optimizations the legacy SimpleMapStore had spent six attempts building. This PR lifts those primitives into a generic stores/epoch_columnar module and rebuilds SketchIndex.series on top of it.

No behavior change for upstream callers — runtime ingest/query paths still hit the legacy SimpleMapStore. The new types + tests + storage shape are in place, ready for the wire-in PR (Phase 4 + 5 + 6 finish).

Why

The legacy SimpleMapStore (asap-query-engine/src/stores/sketch_db/simple_map_store/) has an INDEX_DESIGN.md listing six storage optimizations:

Opt What
1 Lazy window_to_ids — built on first exact query, invalidated cheaply on insert
2 Offset-based index — stores u32 column offsets, not payload clones
3 Monotonic ingest fast path — skip HashSet probe for consecutive same-window inserts
4 Batch metadata hoisting — caller groups DPs by key, fingerprints once
5 Columnar storage — three parallel arrays; range scan touches only windows_col
6 Pre-allocated epoch buffers on rotation

The new SketchIndex had none of them. Insert was Vec::push into per-sid Vec<SketchTimeSeries> with a linear scan to find the right series, then BTreeMap insert per sample. Range queries were O(per-sid Vec) × O(log N) per series. This PR carries Opts 1, 2, 3, 5, 6 into the new index; Opt 4 is caller-side and lands in the wire-in PR.

What changed

New: asap-query-engine/src/stores/epoch_columnar.rs (~470 lines)

Generic, parameterized over the payload type:

  • InternTable<K: Eq + Hash + Clone> — interns a key (a per-series group-by VALUES vector for SketchIndex's use case) to LabelValuesId = u32. Hot path carries 4-byte ids instead of BTreeMap<String,String> clones.

  • MutableEpoch<P> — append-only insert (amortized O(1)), three parallel arrays (windows / label-id / payload), monotonic same-window fast path (Opt 3), lazy window_to_ids (Opt 1+2, built on first exact_query, invalidated by single pointer-width write on insert), pre-allocated buffers on rotation (Opt 6).

  • SealedEpoch<P>from_mutable consumes the columns into a flat sorted Vec<(TimestampRange, LabelValuesId, P)> (O(M log M) paid once at rotation, off the insert hot path). Binary-search range scan O(log N + k). The consume avoids cloning payloads — they move out of the column.

  • SidStoreData<K, P> — pairs one active MutableEpoch<P> with a BTreeMap<EpochId, SealedEpoch<P>> rotation ring. Rotation triggered by epoch_capacity distinct-window threshold; oldest sealed evicted at max_epochs.

Rewritten: asap-query-engine/src/stores/sketch_index.rs

type SidStore = Arc<RwLock<SidStoreData<BTreeMap<String, String>, SketchSampleState>>>;

pub struct SketchIndex {
    instances: RwLock<HashMap<u64, SketchInstanceMetadata>>,  // read-mostly
    series: DashMap<u64, SidStore>,                           // per-sid locking
}
  • instances is RwLock<HashMap> because registration happens once per first-seen sid.
  • series is DashMap because per-sid writes happen on every DP — one writer per sid does not block another.
  • query_range(sid, t0, t1) is the new API replacing the missing range-query path: groups results back by interned label-values vector and returns Vec<SketchTimeSeries>.
  • classify(sid) reads the per-sid SidStoreData to distinguish Hit (current_epoch or sealed_epochs nonempty) from Ghost (registered but empty) from Unknown (not registered).

Public surface preserved

  • Capability, SketchKindHandle, SketchConfig, AccuracyBound,
    SketchInstanceMetadata, SketchSampleState, SketchEncoding,
    SidLookup — same shape, same pub use exports in stores/mod.rs.
  • SketchTimeSeries is still returned by the query path; just no longer the storage shape.

Caveats

  • The new SketchIndex is not yet on the runtime ingest or query path. Legacy SimpleMapStore (aggregation_id-keyed) still carries traffic. Wire-in lives in the next PR (Phase 4 + Phase 5 + Phase 6 finish).
  • register and append_sample now take &self (interior mutability via RwLock + DashMap); call-sites that previously took &mut SketchIndex need a small adjustment.

Test plan

  • cargo build --release -p query_engine_rust clean (warnings only, no errors)
  • cargo build --release -p controller clean
  • 10 unit tests pass (4 in epoch_columnar, 6 in sketch_index):
    • epoch_columnar::tests::intern_idempotent
    • epoch_columnar::tests::mutable_epoch_insert_and_query
    • epoch_columnar::tests::sealed_epoch_binary_search_range
    • epoch_columnar::tests::sid_store_rotation
    • sketch_index::tests::ghost_classification
    • sketch_index::tests::hit_after_append
    • sketch_index::tests::range_query_returns_distinct_series
    • sketch_index::tests::range_query_clips_to_window_bounds
    • sketch_index::tests::epoch_rotation_is_visible_to_query
    • sketch_index::tests::ddsketch_accuracy_bound
  • Full cargo test --release -p query_engine_rust --lib stores — 182 passed; 0 failed (no regressions in legacy SimpleMapStore tests)
  • Wire-in PR (Phase 4 + 5 + 6 finish): exercise the new index under live OTLP ingest + PromQL read

🤖 Generated with Claude Code

…tion columnar pipeline

Per-sid storage was naive `HashMap<u64, Vec<SketchTimeSeries>>` with a
`BTreeMap<TimestampMs, …>` per series. That collapsed all the
optimizations the legacy SimpleMapStore had spent six attempts
building: lazy `window_to_ids` index, offset-based index (column
offsets not payload clones), monotonic ingest fast path, columnar
storage, pre-allocated buffers, batch hoisting at the call site.

This change lifts those primitives into a generic
`stores/epoch_columnar` module, parameterized over the payload type,
and rebuilds `SketchIndex.series` on top of it as
`DashMap<u64, Arc<RwLock<SidStoreData<…>>>>`.

What's in `epoch_columnar`:
- `InternTable<K>` — interns a per-series group-by VALUES vector to a
  compact `LabelValuesId = u32` so the hot loop carries 4-byte ids
  instead of `BTreeMap<String,String>` clones.
- `MutableEpoch<P>` — append-only insert (amortized O(1)), three
  parallel arrays (windows / label-id / payload), monotonic same-window
  fast path, lazy `window_to_ids` (built on first exact_query,
  invalidated by single pointer-write on insert), pre-allocated buffers
  on rotation.
- `SealedEpoch<P>` — `from_mutable` consumes the columns to a flat
  sorted `Vec` (O(M log M) once at rotation, off the insert path);
  binary-search range scan O(log N + k).
- `SidStoreData<K,P>` — pairs an active mutable epoch with a
  `BTreeMap<EpochId, SealedEpoch>` ring, rotation on
  `epoch_capacity` distinct-window threshold.

What's in `SketchIndex`:
- `instances: RwLock<HashMap<u64, SketchInstanceMetadata>>` — read-mostly.
- `series: DashMap<u64, Arc<RwLock<SidStoreData<BTreeMap<String,String>, SketchSampleState>>>>`
  — per-sid locking; one writer per sid doesn't block another.
- `query_range(sid, t0, t1)` — replaces the missing range-query API;
  groups results back by interned label-values vector and returns
  `SketchTimeSeries` with `BTreeMap<i64, SketchSampleState>` samples.
- `classify(sid)` recomputed: looks at the SidStoreData's
  current_epoch + sealed_epochs to distinguish Hit vs Ghost.

Types unchanged:
- `Capability`, `SketchKindHandle`, `SketchConfig`, `AccuracyBound`,
  `SketchInstanceMetadata`, `SketchSampleState`, `SketchEncoding`,
  `SidLookup` — all preserved with the same shape, only the storage
  shape under `series` changed.

Build clean (`cargo build --release -p query_engine_rust`); 10 unit
tests pass:
  stores::epoch_columnar::tests::intern_idempotent
  stores::epoch_columnar::tests::mutable_epoch_insert_and_query
  stores::epoch_columnar::tests::sealed_epoch_binary_search_range
  stores::epoch_columnar::tests::sid_store_rotation
  stores::sketch_index::tests::ghost_classification
  stores::sketch_index::tests::hit_after_append
  stores::sketch_index::tests::range_query_returns_distinct_series
  stores::sketch_index::tests::range_query_clips_to_window_bounds
  stores::sketch_index::tests::epoch_rotation_is_visible_to_query
  stores::sketch_index::tests::ddsketch_accuracy_bound

The SketchIndex still isn't carrying live traffic — Phase 4/5/6 finish
PR follows.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@zzylol
zzylol merged commit 47dad01 into main May 10, 2026
@zzylol
zzylol deleted the refactor/sketch-index-epoch-columnar branch May 10, 2026 17:52
zzylol added a commit that referenced this pull request May 10, 2026
…ath (Phase 4 + 5 + 6 finish) (#122)

Lights up the centralized sid resolver and the new SketchIndex on the
live ingest+query path. Companion to refactor #2 (#121) which built the
SketchIndex storage substrate.

## Phase 4 finish

- OTLP receive (`route_modified_otlp_sketches_to_precompute`): per-DP
  sid resolution gate. Three cases:
    (sid=0, attrs present)  → resolver mints fresh sid via
                              compute-or-mint cache
    (sid != 0, attrs present) → trust attrs; backend-resolved value
                                wins. Mismatch → push old sid to
                                `unknown_series_ids` for sender-side
                                eviction.
    (sid != 0, attrs empty) → resolver.is_known(sid) lookup; miss →
                              push to `unknown_series_ids`, drop DP.
- `route_modified_otlp_sketches_to_precompute` now returns
  `Vec<u64>` of unknown sids; both `MetricsServiceImpl::export` and
  `handle_otlp_http` stamp it into `ExportMetricsServiceResponse.
  unknown_series_ids` (the field already existed on the wire from
  Phase 2, was Vec::new() placeholder).
- New `ResolveSeriesIDs` gRPC service in `metrics_service.proto`:
  bulk pre-resolution endpoint for senders that have an attribute-only
  batch ready before the next Export. Reuses the existing
  `SeriesAssignment` shape; idempotent end-to-end.

## Phase 5 finish

- `IngestState` now carries `Arc<SeriesIdResolver>` +
  `Arc<SketchIndex>`. Both allocated once in `main.rs` and threaded
  through `PrecomputeEngine::new` so the OTLP receiver, the precompute
  engine, and `SimpleEngine` all share the same instances.
- After sid resolution, on first-seen sid the OTLP path materializes
  `SketchInstanceMetadata` (capability inferred from sketch kind:
  DDSketch/KLL → QuantileApprox, HLL → CardinalityApprox,
  CountSketch/CountMin → FrequencyTopk; sketch_config from the
  parent-container fields lifted in Phase 2; accuracy via
  `AccuracyBound::from_config`) and registers it. Per-DP:
  `append_sample(sid, label_values, (start_ms, end_ms), state)`.
- `ModifiedOtlpSketchDp` extended with `series_id`,
  `start_time_unix_nano`, and `container_config: SketchConfig`. The
  five typed-DP match arms thread these from the OTLP wire shape.
- New helpers: `sketch_kind_handle_for(&dp)` and
  `encoding_to_handle(i32) -> SketchEncoding`.
- Legacy `WorkerMessage::AccumulatorInput` write site annotated
  `// DEPRECATED:` — kept as fallback during validation, removed
  after warm-tier carries traffic in production.

- `SimpleEngine` got an optional `sketch_index: Option<Arc<SketchIndex>>`
  field + `with_sketch_index` builder. `QueryEngine::execute` now
  pre-classifies the query's sid set:
    1. Parse PromQL → extract metric name + label-matcher KEY set.
    2. `index.instances_matching(metric, required_keys)` → candidate sids.
    3. For each: `index.classify(sid)`:
        - all `Hit` → fall through to handle_query (legacy path; the
          per-Capability sketch reducer over `query_range` is a
          follow-up — see TODO in the engine).
        - any `Ghost` or `Unknown` → return
          `EngineError::CapabilityMiss(SketchWarmTier, ...)`.
        - empty match → likewise CapabilityMiss.
- New `SketchIndex::instances_matching(metric, required_keys)` helper:
  filters instances where metric_name matches and required_keys is a
  subset of the instance's `group_by_keys` (over-approximation;
  archive failover catches the rest).

## Phase 6 finish

- The existing `EngineRouter::execute` already retries on
  `CapabilityMiss` against the next engine in priority order, so the
  Ghost/Unknown branch falls through to the archive engine
  (Thanos forward) automatically. No router change needed —
  `CapabilityMiss(SketchWarmTier, ...)` IS the trigger.

## Deferred (TODOs at call sites)

- Per-Capability sketch reducer over `SketchIndex.query_range` for
  the all-Hit case (today still flows through the legacy
  `SimpleMapStore` path). Follow-up PR.
- Phase 6 hybrid stitch (warm covers `[t0..t1']`, archive covers
  `[t1'..t1]`, concatenate). Requires `QueryResult` to carry
  timestamp coverage metadata, which it doesn't today. Deferred.
- Legacy `WorkerMessage::AccumulatorInput` aggregation_id-keyed write
  path: marked `// DEPRECATED:` only. Removal is a separate cleanup
  PR after warm-tier validation.

## Build + test

- `cargo build --release -p query_engine_rust` clean (warnings only).
- 7 net-new tests pass:
    drivers::ingest::otel::sid_resolution_tests::* (3 tests)
    engines::simple::engine::warm_tier_classify_tests::* (3 tests)
    plus 1 small expansion in series_resolver tests
- 902/902 tests pass for code touched by this PR. The 32 pre-existing
  failures (datafusion / persistence / schema_timeline_dispatch) were
  red on main before this branch existed and are unrelated to
  warm-tier wiring.

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 10, 2026
The datafusion-backed query path (engines/physical, engines/logical, the
datafusion_summary_library crate, the parallel datafusion-LogicalPlan
adapter, all DF-specific tests, and the controller-side query_language
adapter) is redundant: the controller crate already owns logical and
physical plan layers (controller/src/algebra/{lower,physical,plan}.rs +
intent_algebra/lower.rs + language_logical_plan/). Keeping the parallel
DF stack alive cost an extra translation hop, ~5800 lines of code, and
a test surface that papered over the divergence.

Companion structural cleanup: relocate the new sid-keyed SketchIndex
files (added in #121 / #122) into stores/sketch_db/, the canonical
home for storage. They were floating at stores/ top level after
#121.

## What was deleted

- crates/datafusion_summary_library/ (8 files — sketch-aware datafusion
  ExecutionPlan operators)
- asap-query-engine/src/engines/physical/ (7 files — datafusion-LogicalPlan
  → ExecutionPlan adapters)
- asap-query-engine/src/engines/logical/ (2 files — plan_builder for
  the DF logical layer)
- asap-query-engine/src/tests/datafusion/ (11 test files exercising the
  deleted engines)
- controller/src/query_language/datafusion/ (small adapter)
- datafusion = "43" + datafusion_summary_library deps from
  asap-query-engine/Cargo.toml + workspace Cargo.toml.

## What was moved

- asap-query-engine/src/stores/sketch_index.rs
    → asap-query-engine/src/stores/sketch_db/sketch_index.rs
- asap-query-engine/src/stores/epoch_columnar.rs
    → asap-query-engine/src/stores/sketch_db/epoch_columnar.rs
- All cross-file imports rewritten to the new sketch_db:: paths.

## In-source surgery

- engines/simple/engine.rs: deleted the DF-importing methods
  (SessionContext, physical_plan::collect, record_batch_to_result_map,
  engines::logical::plan_builder::build_*). Phase 5 SketchIndex
  warm-tier hook (with_sketch_index + classify branch in execute())
  preserved verbatim, only its imports were rewritten.

- stores/sketch_db/simple_map_store/per_key.rs: query_disk_parts and
  EpochSource::snapshot_sealed_epoch now panic with
  "datafusion-dependent path removed; ingest/persistence still under
  refactor". Both fire only on persistence-enabled runs (agreed-upon
  breakage). Marked // TODO: replace with non-datafusion path.

- 8 controller files: deleted DF-referencing variants/match arms/trait
  impls.

## Verification

- cargo build --release -p query_engine_rust: clean (3 dead-code warnings)
- cargo build --release -p controller: clean (5 unused-import warnings on
  pre-existing items)
- cargo test --release -p query_engine_rust --lib: 792 passed, 2 failed
  (schema_timeline_dispatch_tests::*, both pre-existing flakes; 8
  persistence tests skipped — they fire the TODO panics)
- grep for "datafusion" in the tree: 6 lines remain, all in TODO
  comments and panic messages. No imports, deps, or types.

## Follow-ups

- Replace persistence TODO panics with a non-datafusion warm-tier
  serializer (likely the asap_sketchlib proto codec).
- The per-Capability sketch reducer (warm-tier query evaluator) is in
  flight in a parallel branch — replaces the all-Hit CapabilityMiss
  with real sketch evaluation from SketchIndex.query_range.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant