Skip to content

feat: SimpleEngine re-snapshots StreamingConfig per query (PR E phase 2) - #12

Merged
zzylol merged 1 commit into
mainfrom
feat/pr-e-phase2-per-query-snapshot
Apr 15, 2026
Merged

zzylol merged 1 commit into
mainfrom
feat/pr-e-phase2-per-query-snapshot

Conversation

@zzylol

@zzylol zzylol commented Apr 15, 2026

Copy link
Copy Markdown
Contributor

PR E phase 2 — makes PR #10's POST /api/v1/streaming-config endpoint actually useful for query-time behavior.

The problem this solves

Phase 1 (merged in PR #10) landed the swap machinery and the HTTP endpoint, but SimpleEngine still took a one-time snapshot of Arc<StreamingConfig> at construction. A controller push via the endpoint would update the shared ArcSwap, but SimpleEngine held its own cached Arc and never re-read — so the swap was observable only through the GET debug endpoint, never through an actual query.

This PR threads the shared HotReloadStreamingConfig handle all the way into SimpleEngine and re-snapshots per query:

main.rs builds one HotReloadStreamingConfig
  ├─> HttpServer::with_hot_reload_config(handle.clone())
  │       ↑ POST /api/v1/streaming-config swaps via this handle
  │
  └─> SimpleEngine::new_with_hot_reload(handle.clone())
          ↑ each query calls streaming_config_snapshot() which
            load_full()s from the same underlying ArcSwap

HotReloadStreamingConfig clones share the same inner Arc<ArcSwap<StreamingConfig>>, so a swap on one clone is immediately visible through the other. That's the entire mechanism.

What this PR closes

Paired with PR #10 (endpoint) and PR #11 (capability-miss notify), phase 2 closes the query-side of the control loop end-to-end:

query arrives → SimpleEngine miss
    ↓
PR #11 fires fire-and-forget POST to controller /api/v1/plan
    ↓
§5.2 fallback serves the current query
    ↓
controller generates a new sketch plan
    ↓
controller POSTs to backend /api/v1/streaming-config (PR #10)
    ↓
swap lands in the shared HotReloadStreamingConfig
    ↓
next query re-snapshots via PR E phase 2 and finds a match   ← THIS PR

Before phase 2 the last step was broken — the swap landed but queries never saw it.

Changes

SimpleEngine

  • Field renamed: streaming_config: Arc<StreamingConfig>streaming_config_source: HotReloadStreamingConfig
  • New constructor: SimpleEngine::new_with_hot_reload(...) takes the shared handle. main.rs uses this path.
  • Existing constructor preserved: SimpleEngine::new(...) signature unchanged for tests, binaries, and legacy callers. Internally wraps the provided Arc<StreamingConfig> in a fresh HotReloadStreamingConfig so external unrelated handles don't leak in.
  • New accessor: streaming_config_snapshot(&self) -> Arc<StreamingConfig>. Each call observes whatever was most recently pushed via PR feat: StreamingConfig hot-reload — phase 1 API + endpoint (PR E) #10.

13 internal call sites in simple_engine.rs

Each site that reads from streaming_config is converted to use one of two patterns:

  • Inlined snapshot when the chain's result copies out of the borrowed AggregationConfig (e.g. window_size * 1000, .grouping_labels.clone()):

    self.streaming_config_snapshot()
        .get_aggregation_config(id)
        .map(|c| c.window_size * 1000)?
  • Per-helper binding when a borrowed &AggregationConfig outlives the initial expression (e.g. create_store_query_plan reads multiple fields):

    let streaming_config = self.streaming_config_snapshot();
    let cfg = streaming_config.get_aggregation_config(id)?;
    // ... use cfg ...

main.rs

Switches from SimpleEngine::new(streaming_config.clone(), ...) to SimpleEngine::new_with_hot_reload(hot_reload_config.clone(), ...).

What's NOT hot-reloaded yet (phase 3)

  • Per-query top-level snapshot binding — today each helper re-snapshots independently, so two helpers in the same query could theoretically see different configs under a concurrent swap. Both views remain internally consistent and the resulting answer is correct under EITHER old or new config; swaps are rare and the torn window is microseconds. Binding a single per-query snapshot and plumbing it through helper signatures is a larger refactor deferred to phase 3.
  • Ingest router / OTLP receiver — still routes by startup AggregationConfig clones. Adding a new agg_id via POST does not yet cause incoming metrics to be routed into that agg_id on the ingest side.
  • In-flight precompute worker GroupState — unchanged from phase 1. Existing groups complete with their construction-time config; new groups created after the swap pick up the new config. Intended semantics.

Tests

Three new unit tests in a new hot_reload_phase2_tests module:

  1. streaming_config_snapshot_starts_at_initial_config — baseline: initial config observable through the new accessor.
  2. streaming_config_snapshot_observes_post_construction_swapthe core guarantee: build engine with a shared handle → swap from outside → next accessor call sees the new config. Also asserts the previous snapshot remains internally consistent (per-query stability under ArcSwap).
  3. legacy_new_constructor_is_independent_of_external_handle — verifies the backwards-compat SimpleEngine::new path pins the initial config in a fresh internal wrapper; a swap on an unrelated external handle does NOT leak into test/bin-only constructions.

Validation

  • cargo check --all-targets: clean
  • cargo clippy --all-targets -- -D warnings: clean
  • cargo fmt --check: clean
  • cargo test -p query_engine_rust --lib: 495 passed (up 3 from main)

Stack

🤖 Generated with Claude Code

Makes `POST /api/v1/streaming-config` actually useful for query-time
behavior. Before this PR, SimpleEngine snapshotted the startup config
into `Arc<StreamingConfig>` once at construction and ignored every
subsequent swap — meaning a controller push via PR #10's endpoint
was observable only by the GET debug endpoint, not by queries.

Phase 2 threads the shared `HotReloadStreamingConfig` handle all the
way into SimpleEngine and re-snapshots per query:

  main.rs builds one HotReloadStreamingConfig
    ├─> HttpServer::with_hot_reload_config(handle.clone())
    │       ↑ POST /api/v1/streaming-config swaps via this handle
    │
    └─> SimpleEngine::new_with_hot_reload(handle.clone())
            ↑ each query calls streaming_config_snapshot() which
              load_full()s from the same underlying ArcSwap

Because `HotReloadStreamingConfig` clones share the same inner
`Arc<ArcSwap<StreamingConfig>>`, a swap on one clone is immediately
visible through the other. That's the entire mechanism.

## Changes

### `SimpleEngine`

  * Field `streaming_config: Arc<StreamingConfig>` → `streaming_config_source: HotReloadStreamingConfig`
  * New constructor `SimpleEngine::new_with_hot_reload(...)` takes the
    shared handle. `main.rs` uses this path.
  * Existing `SimpleEngine::new(...)` signature preserved for tests,
    binaries, and legacy callers. Internally wraps the provided
    `Arc<StreamingConfig>` in a FRESH `HotReloadStreamingConfig` so
    external unrelated handles don't leak in — the `legacy_new_constructor_is_independent_of_external_handle`
    test pins this behavior.
  * New accessor `streaming_config_snapshot(&self) -> Arc<StreamingConfig>`.
    Each call observes whatever was most recently pushed via PR #10.
    Doc comment explains the per-query-binding discipline internal
    callers must follow.

### 13 internal call sites in `simple_engine.rs`

Each site that reads from `streaming_config` is converted to use
either:

  * Inlined `self.streaming_config_snapshot().get_aggregation_config(...).map(|c| c.X.clone())...`
    when the access is a single-expression chain whose result copies
    out of the borrowed `AggregationConfig` (e.g. `window_size * 1000`
    or a `.clone()` on an owned field).
  * A per-helper `let streaming_config = self.streaming_config_snapshot();`
    binding when a borrowed `&AggregationConfig` outlives the initial
    expression (e.g. `create_store_query_plan`, which binds the
    config once and reads multiple fields from it). The binding
    ensures a consistent view for that helper's entire execution
    and keeps the borrowed reference valid through the local Arc's
    lifetime.

Per-query entry points do NOT bind a single top-level snapshot today.
Instead each helper re-snapshots independently. Under a concurrent
swap that races with a running query, two helpers in the same query
could see different configs — but both views are internally
consistent, and the resulting query answer remains correct under
EITHER view (old or new). Swaps are rare; the torn-state window
is microseconds. Binding a single per-query snapshot and plumbing
it through helper signatures would be a much larger refactor and
is tracked as phase 3.

### `main.rs`

Switches from `SimpleEngine::new(streaming_config.clone(), ...)` to
`SimpleEngine::new_with_hot_reload(hot_reload_config.clone(), ...)`.
The same `HotReloadStreamingConfig` handle now flows to both the HTTP
server (which handles POST) and the engine (which reads), so a POST
is observable by the next query.

## Tests

Three new unit tests in a `hot_reload_phase2_tests` module at the
end of `simple_engine.rs`:

1. `streaming_config_snapshot_starts_at_initial_config` — baseline
   sanity check that the initial config is observable through the
   new accessor.

2. `streaming_config_snapshot_observes_post_construction_swap` —
   **the core PR E phase 2 guarantee**: build the engine with a
   shared handle, swap the handle from outside, call the accessor
   again, and verify the new config is visible. Also asserts the
   previous snapshot remains internally consistent (per-query
   stability).

3. `legacy_new_constructor_is_independent_of_external_handle` —
   verifies that the backwards-compat `SimpleEngine::new` path
   snapshots into a FRESH internal `HotReloadStreamingConfig`, so
   a swap on an unrelated external handle does NOT leak into
   test/bin-only constructions.

## Validation

  * cargo check --all-targets: clean
  * cargo clippy --all-targets -- -D warnings: clean
  * cargo fmt --check: clean
  * cargo test -p query_engine_rust --lib: 495 passed (up 3 from
    main — the phase 2 unit tests)

## What closes now

Paired with PR #10 (endpoint) and PR #11 (capability-miss notify),
phase 2 closes the query-side of the control loop end-to-end:

  query hits SimpleEngine miss
    → PR #11 fires fire-and-forget POST to controller /api/v1/plan
    → §5.2 fallback serves the current query
    → controller generates new plan
    → controller POSTs to backend /api/v1/streaming-config (PR #10)
    → swap lands in the shared HotReloadStreamingConfig
    → next query re-snapshots via PR E phase 2 and finds a match

## What's still NOT hot-reloaded

  * **Ingest router / OTLP receiver** — still routes by startup
    `AggregationConfig` clones. Adding a new agg_id via POST does
    not yet cause incoming metrics to be routed into that agg_id
    on the ingest side. Tracked as phase 3.
  * **In-flight precompute worker GroupState** — unchanged from
    phase 1. Existing groups complete with construction-time config;
    new groups pick up the new config. Intended semantics.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@zzylol
zzylol merged commit a017687 into main Apr 15, 2026
5 of 6 checks passed
@zzylol
zzylol deleted the feat/pr-e-phase2-per-query-snapshot branch April 15, 2026 19:50
zzylol added a commit that referenced this pull request May 11, 2026
… + typed Predicate (#136)

Additive lift of all 10 A-classified legacy variants into the canonical
`intent_algebra::query_expr::QueryExpr`, plus the minimal typed
`Predicate` IR for `Filter.pred` covering the 4 used-and-cleanly-shaped
ScalarExpr variants.

## What landed

### Canonical `QueryExpr` grew from 5 → 15 variants

Pre-existing (PR #130 / canonical): `Scan`, `Window`, `Aggregate`,
`LetBinding`, `Ref`.

New (this PR): `Filter`, `Project`, `Partition`, `Distinct`, `Merge`,
`Join`, `SetOp`, `Sort`, `Limit`, `BinaryOp`.

Naming convention: single-input variants use `child:` (matches existing
canonical `Window`/`Aggregate`/`LetBinding`). Multi-input variants
follow design.md §6 shapes exactly: `Merge { children }`,
`Join { kind, pred, left, right }`, `SetOp { kind, all, left, right }`,
`BinaryOp { op, lhs, rhs, vector_match }`.

### Supporting types lifted alongside

`ColumnRef`, `PartitionKeys`, `BinaryOpKind`, `JoinKind`, `SetOpKind`,
`SortKey`, `VectorMatch`/`VectorMatchKind`/`VectorGrouping`/`GroupSide`,
`LiteralValue`, `ProjectItem`. All re-exported from
`intent_algebra::mod.rs`.

### New typed `Predicate`

```rust
pub enum Predicate {
    Column(ColumnRef),
    Literal(LiteralValue),
    BinaryOp { op: BinaryOpKind, lhs: Box<Predicate>, rhs: Box<Predicate> },
    IsNull { expr: Box<Predicate>, negated: bool },
}
```

Plus `Predicate::from_legacy_scalar(&legacy::ScalarExpr) ->
Result<Predicate, QueryExprError>`. Translates the 4 supported
variants (Column / Literal / BinaryOp / IsNull); returns
`QueryExprError::UnsupportedLegacyScalar(name)` for the 4 E-deferred
(FunctionCall / ScalarSubquery / InList / Between). Per user decision,
those stay in legacy until a real consumer demands the typed shape.

Translation handles `LiteralValue::Duration → Int(nanos)` fold (canonical
LiteralValue is deliberately narrower).

### Consumer migration deferred to subsequent batches

**Zero consumer-site redirects in this PR** — and that's the right
call. Every legacy-A-variant consumer (`query_parser/`, `physical/
{stage_split,planner,allocator}.rs`, `optimizer/engine.rs`,
`legacy_lower.rs`) simultaneously matches A-variants AND C-variants
(`Source`, `Aggregate`, `Window`, `SketchAgg`, `WindowedAgg`, `TopK`,
`HistogramQuantile`, `PromQLSubquery`) AND constructs `ScalarExpr`
predicates with E-deferred variants. Migrating them mid-pipeline would
require lifting C-variants and E-variants in the same PR — explicit
out-of-scope per the batch plan.

The consumers migrate one C-batch at a time:
- Batch 3a (PR #11): `AggFunc → AggIntent`
- Batch 3b (PR #10): `TopK` split
- Batch 3c (PR #8): `WindowedAgg` un-fusion
- Batch 3d (PR #9): `SketchAgg → Aggregate@L3`
- Batch 3e (PR #12): PromQL parser `histogram_quantile → Quantile`
- Batch 13: retire `legacy_expr.rs` + `legacy_lower.rs`

### Catch-all arms added in 6 canonical-side consumers

Files: `optimizer/cost/mod.rs`, `physical/colored_dag/{allocator,emitter}.rs`,
`sketch_algebra/lower.rs`, `warm_tier_analysis.rs`. These previously
matched canonical `QueryExpr` exhaustively over the 5 pre-existing
variants; now they need conservative handling for the 10 new ones
(Edge stage / Logical wrap / child-walk / zero-cost) with TODO
markers for the follow-up reshape batches.

### Serde tag note

Initially gave `ColumnRef`/`LiteralValue`/`PartitionKeys`/`Predicate`
internally-tagged `#[serde(tag = "kind", rename_all = "snake_case")]`
attrs, but serde rejects internally-tagged newtype variants containing
primitives (e.g. `Named(String)`). Switched to externally-tagged
`#[serde(rename_all = "snake_case")]`. Caught by the round-trip test.

## Build + test

- `cargo build --release -p controller` — clean
- `cargo build --release -p query_engine_rust` — clean
- `cargo test -p controller --lib` — **666 passed** (was 655 after
  Batch 1; +11 from new typed-Predicate + schema tests)
- `cargo test -p query_engine_rust --lib -- engines::warm_tier` — 13/13

## Diff: 8 files, +794 / -8

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