diff --git a/.design_docs/sliding-window-query-engine-design.md b/.design_docs/sliding-window-query-engine-design.md new file mode 100644 index 0000000..6442dae --- /dev/null +++ b/.design_docs/sliding-window-query-engine-design.md @@ -0,0 +1,322 @@ +# Design: sliding-window query engine execution (#557) + +Written up from a `/grilling` session. Supersedes the "not designed yet" +section of [[sliding-window-multi-window-merge-blocker]] — that doc's +findings still hold; this is the design built on top of them. This version +supersedes an earlier draft that proposed `k` separate exact-match store +calls for instant queries (§2 below) before the range-query pipeline +unification was discovered. + +## Key insight that shrinks scope + +`optimizer/candidate_gen.rs`'s `window_candidates()` already constrains slide +`S` such that `S | W` (window width). That means the `k = range_ms / W` +windows needed to cover a query span, picked at **stride `W`** from the +`S`-spaced storage grid, are non-overlapping by construction — merging them +is exactly as safe as merging tumbling's non-overlapping buckets. No +overlap-aware accounting (subtract, weighted merge, etc.) is needed for +correctness. + +Separately: every `subtractable` sketch type in `sketch_properties.rs` is +also `mergeable`, and the live engine has **no subtract execution path at +all today** (`determine_query_method`'s `Subtract` variant is +cost-model-only, used by the offline optimizer, never wired into +`simple_engine`). So Subtract is out of scope for this issue — merge-only +is sufficient for every case `window_compatible` will allow through. + +## Decisions + +### 1. `capability_matching.rs` + +`window_compatible`'s Sliding arm becomes `data_range_ms.is_multiple_of(window_ms)`, +mirroring Tumbling exactly (as the issue specifies). No change to what it +checks about `S` — a config's `S | W` invariant is guaranteed by whatever +constructed it (`candidate_gen.rs`'s own constraint), not re-verified here, +same as Tumbling doesn't re-verify scrape-interval alignment at match time. + +### 2. Unified fetch mechanism: reuse the range-query pipeline for instant queries too + +There are two independent execution mechanisms in `simple_engine` today: + +- **Instant-query path** (`create_store_query_plan` → + `execute_and_merge_store_queries` → `merge_precomputed_outputs`): one store + call, merges whatever `TimestampedBucketsMap` comes back, trusting the + store call's `(start, end)` bounds were already correct. +- **Range-query path** (`execute_range_query_pipeline`, `mod.rs:1423`): one + big range fetch over the whole query horizon, builds an in-memory + `bucket_map: HashMap`, then **per output + step** walks `t = window_start; t < current_time; t += window_size_ms`, + doing an exact-timestamp lookup and merging whatever's found (missing + entries silently skipped — "partial data is okay"). + +For Tumbling both are safe, because every stored bucket already sits on the +one `W`-spaced grid — a plain range fetch naturally returns exactly the +non-overlapping buckets needed either way. For Sliding this isn't true: the +store holds `S`-spaced buckets, denser than `W`. The instant path's "merge +whatever's in the map" would double-count; the range path's stride-`W` +exact-timestamp walk already does the right thing — it only ever looks up +`W`-strided positions, silently ignoring the intervening `S`-spaced entries. + +**Decision: extract the range pipeline's per-step "walk by `window_size_ms`, +exact-lookup, merge found ones" logic into a shared helper, and have +**Sliding's instant-query path call it too** (as a single-step case — the +same shape as a range query whose horizon is one step wide). Tumbling's +existing instant-query path (`merge_precomputed_outputs`) is left +**completely untouched** — it isn't broken, and touching working code here +is unnecessary risk for zero benefit. + +Net effect: +- Sliding's instant path now does **one range fetch** (`query_precomputed_output`, + not `_exact`) over `[end - k*W, end)`, then the shared stride-`W` walk + helper, called once. +- `query_precomputed_output_exact` / `is_exact_query: true` becomes + **unused once this ships** — nothing sets `is_exact_query: true` anymore. + Not removing the `Store` trait method or the flag in this issue (that's a + 4-backend-impl change, separate scope) — but flagging it in the + legacy/unused-code issue (#559) as a fresh entry once #557 lands, so it + doesn't get forgotten. +- **Code comment requirement:** wherever the shared walk helper is + extracted, and wherever the old sliding-specific "skip merge, expecting 1 + precompute per key" branch is deleted, leave a comment explaining *why* + the instant path now reuses range-query machinery (the double-counting + problem above) — this is the actual non-obvious reasoning a future reader + would otherwise have to re-derive from scratch. + +Rejected (superseded): a loop of `k` separate `query_precomputed_output_exact` +calls for the instant path. Correct, but duplicates stride-selection logic +that the range pipeline already has, instead of reusing it. + +### 3. Merge: delete the sliding special case, don't extend it + +`execute_and_merge_store_queries`'s current sliding branch +("Skipping merge, expecting 1 precompute per key") assumed exactly one +bucket per key. That assumption is gone. Per §2, Sliding's instant queries +now route through the shared range-pipeline walk helper instead of +`merge_precomputed_outputs` — so this branch is **deleted outright**, not +extended to loop. Net: less code than today. + +### 4. Epoch/slide-interval alignment + +Worker buckets sit on the `slide_interval_ms` grid, anchored at Unix epoch 0 +(`precompute_engine/window_manager.rs`). The fix differs between instant and +range queries, because §5 gives range queries a way to guarantee alignment +by construction instead of re-deriving it defensively on every step. + +- **The fetch bounds** (`[end - k*W, end)`), in both cases, don't need to be + grid-aligned — `query_precomputed_output` is a plain numeric range scan, + not a grid lookup, so a generously-bounded span is fine as long as it + covers everything needed. +- **The walk's lookup points** (`window_start`, and each `t` stepped by `W`) + *do* need grid alignment — they're exact-timestamp `HashMap` lookups + against `slide_interval_ms`-grid keys. Misalignment doesn't error, it + silently returns "no data in window" (instant path) or skips that output + sample (range path) — a correctness-shaped bug that looks like a data gap, + not a crash. **This is exactly the kind of thing that needs a code comment + at the alignment call site**, since the failure mode gives no signal that + something's wrong. +- **Instant queries:** floor-align `current_time` to `slide_interval_ms` + once per call, silently + `warn!`, same precedent as the existing + `data_ingestion_interval_ms` alignment in `align_end_timestamp_promql`. + There's no `step` to reason about for a single point, so this bounded + (up to one `S`) staleness is unavoidable here — tracked as a known gap in + #558, not fixed in this issue. +- **Range queries:** floor-align only the **first** `current_time` (the + query's `start`), once, before the loop — not per step. §5's new + `step % slide_interval_ms == 0` validation guarantees every subsequent + `current_time = start + n*step` stays exactly on the grid by construction + (adding `S`-divisible increments to a grid-aligned point can't drift off + the grid), and since `S | W` always holds for a valid config, every + `W`-strided walk point inside each step stays grid-aligned too. **Zero** + staleness for range queries that pass validation — strictly better than + the instant-query floor-align-and-hope approach, and it's why §5 exists + as a real check rather than dead weight. +- **Scope:** only the values used inside the walk change. + `timestamps.end_timestamp` / the query's nominal `start` (what's reported + back as the answer's timestamp(s)) stays untouched. + +### 5. `validate_range_query_params` + +Initially thought to be dead: the check (`step % window_size_ms == 0`) isn't +load-bearing for the live Tumbling algorithm's actual control flow — the +real loop increments `current_time += step_ms` directly (`mod.rs:1567`), and +`buckets_per_step`/`lookback_bucket_count` (the values that would need this +invariant) are computed but only ever used in a debug log line. Fossil +evidence in the test module (`simulate_sliding_window`, index-based, vs. +`simulate_timestamp_based_lookup`, current, explicitly commented *"the new +implementation that handles gaps in data correctly"*) suggests it's a +leftover from an earlier, superseded index-based algorithm. + +**Decision, revised: leave Tumbling's check completely untouched** — "not +load-bearing today" isn't the same as "safe to delete," and this is the one +piece of #557 that could otherwise touch **live** Tumbling query behavior. +Zero risk beats a marginal cleanup; if it's truly dead, that's a separate, +independently-reviewable janitorial change, not bundled into this issue. + +**Add a new, real (not vestigial) Sliding branch instead: `step % +slide_interval_ms == 0`, not `window_size_ms`.** This isn't a mechanical +copy of Tumbling's check with `S` swapped in for `W` — it's doing different +work. Tumbling's check (to the extent it ever mattered) was about bucket +alignment; Sliding's is what makes §4's "align the range-query start once, +not every step" simplification correct — it guarantees the grid-alignment +invariant is preserved across every step by construction, turning a +would-be silent per-step misalignment (bounded staleness, easy to miss) into +a loud rejection at the query boundary instead. `step % window_size_ms == 0` +would be **stricter than necessary** for Sliding: since `S | W` always holds, +`step % S == 0` alone is sufficient to keep every `W`-strided walk point on +the grid, and using `W` as the divisor would wrongly reject queries like +`step == S` (refresh every slide interval) — exactly the natural cadence +you'd want to allow. + +Rename the parameter from `tumbling_window_ms` to `window_size_ms` at the +same time, since Tumbling's arm was never actually tumbling-specific logic, +just named as if it were — but that's the only change on the Tumbling side. + +### 6. `cleanup.rs` + +Runtime semantics confirmed by reading the actual enforcement code +(`simple_map_store/global.rs:374-399`): `read_count_threshold` is a +delete-after-N-reads counter, incremented once per **store call** that +touches a window's time range — regardless of whether the caller's +downstream logic actually uses that window. It must be a safe **upper +bound** on total future reads a bucket could receive; under-provisioning is +a correctness bug (premature deletion), over-provisioning is just wasted +retention. + +This interacts with §2's unification in a way that isn't obvious and +**needs a code comment at the formula site**: because Sliding's fetch now +pulls a whole `[end - k*W, end)` range in one store call (§2), the store-level +read-count accounting (`global.rs:527-531`, which bumps every window +overlapping the scanned range) bumps **every `S`-spaced bucket in that span** +— not just the `k` buckets the in-memory walk actually merges. The number of +buckets touched per query is `data_range_ms / slide_interval_ms`, **not** +`data_range_ms / window_size_ms` — using the latter would under-provision +`read_count_threshold` whenever `S < W` (the normal case), risking deletion +of a bucket before a later query that still needs it. + +- New parameter needed: `slide_interval_ms`, threaded from the caller + (`planner/promql.rs:287`) alongside the existing parameters — for + Tumbling this is irrelevant (`S == W` there, formula unchanged). +- `ReadBased` (the policy that actually matters — confirmed every config in + the repo defaults to it, `CircularBuffer` is never selected anywhere): + `read_count_threshold = (data_range_ms / slide_interval_ms) * num_steps`, + `num_steps` computed the same way Tumbling already does it. +- `CircularBuffer`: mirror Tumbling's shape too — + `ceil((data_range_ms + range_duration_ms) / window_size_ms)` — as a + mechanical translation while already touching this function. Uses + `window_size_ms` (not `slide_interval_ms`) since it's a retain-N-buckets + count, not a read-count — the two policies are counting fundamentally + different things and shouldn't share a divisor by coincidence. **Flagged + as untested by construction**: nothing in the repo exercises + `CircularBuffer` at all today, for either window type. Tracked in #559. +- Fix the dangling gap the blocker doc flagged: Sliding's branch currently + returns before reaching the `NoCleanup` error case, relying on caller + discipline (every current caller happens to guard `NoCleanup` before + calling) rather than the function's own match arm. + +### 7. Multi-population keys — no change needed (confirmed, not designed) + +`create_keys_query_params` already does a plain range query +(`is_exact_query: false`) for both `SetAggregator` (latest window) and +`DeltaSetAggregator` (from-beginning-of-time), regardless of window type. +Both are set-union merges (`mergeable: true, subtractable: false` in +`sketch_properties.rs`) — a key-set union is idempotent, so merging +overlapping `S`-grid buckets on the key side is correct by construction, +unlike value accumulators where overlap causes double-counting. No changes +needed here. + +### 8. Aggregation priority — no change needed (confirmed, not designed) + +`aggregation_priority` prefers larger `window_size_ms` (fewer buckets to +merge). Same reasoning applies unchanged to Sliding (fewer `k`). + +## Test plan + +End-to-end correctness test, using the existing +`test_utilities/engine_factories.rs` harness (`create_engine_single_pop*` — +builds a real `SimpleEngine` + `SimpleMapStore`, no new scaffolding needed). + +`should_use_sliding_window()` is hardcoded `false` (planner can't emit +Sliding configs until #555 unblocks) — so "plan" in this test means +**hand-constructing** a `Sliding` `AggregationConfig` directly, same as +`capability_matching_tests.rs` already does for its configs, not invoking +the live planner. + +Matrix across different `k` (window_size = data_range / {1, 2, 3, 6}) to +cover the stride logic at different divisor counts, e.g.: +- `k=1`: exact-match case, same as today's existing behavior (backward-compat + check) — now served via the unified range-pipeline-with-one-step path + instead of `query_precomputed_output_exact`. +- `k=2, 3, 6`: multi-window merge at different strides, both as instant + queries (single-step walk) and as PromQL range queries (multi-step walk) + to cover both callers of the shared walk helper. + +For each: insert distinct known values (e.g. `Sum` accumulator, values +`1..=N`) at every `slide_interval_ms` across a wide enough span, fire a +query whose `data_range_ms = k * window_size_ms`, and assert the result +equals the sum of exactly the `k` non-overlapping `W`-strided buckets — +**not** the sum of all `S`-spaced buckets in range. That second assertion is +the actual regression catcher: it's what would fail if a future change +reused the range-query path naively instead of the stride-`W` walk. + +Also worth a dedicated test for the read-count accounting in §6: insert a +Sliding config, run a query that fetches a `k*W`-wide range, and assert the +store's read-count for **every** `S`-spaced bucket in that range incremented +by 1 — not just the `k` that were merged — to lock in the §6 semantics +against a future "optimize the fetch to only touch what's merged" change +that would silently break the cleanup-threshold math. + +## Implementation approach: TDD, stacked PRs + +This is correctness-critical query-serving logic (wrong-but-plausible +results, not crashes, are the failure mode — double-counted merges, +silently-dropped alignment). Test-first, and staged as separately-reviewable +stacked PRs rather than one large diff. + +The key fact enabling this: `should_use_sliding_window()` is hardcoded +`false` (planner can't emit a Sliding config until #555 unblocks), so +**every piece of this stack is inert in production** — nothing a real query +can reach changes until the final PR lands. And `engine_factories.rs`'s test +harness registers configs via explicit `query_configs` entries, which +`promql.rs:1034` checks *before* falling back to capability matching — so +the execution-side tests can hand-register a `k>1` Sliding config and +exercise it directly, completely bypassing `window_compatible`. That's what +makes the ordering below safe: execution gets built and proven correct +*before* capability matching is relaxed to let real queries reach it. + +1. **PR A — §5, `validate_range_query_params`.** Add the Sliding-only + `step % slide_interval_ms == 0` branch; Tumbling's existing check is + untouched. Independent of the rest of the stack. +2. **PR B — §2–4, execution unification (base of the stack).** Shared + stride-`W` walk helper; Sliding's instant path reuses it instead of + `merge_precomputed_outputs`; delete the old "skip merge, expecting 1 + precompute per key" branch; alignment fix (instant: floor-align once per + call; range: floor-align only the start, relying on PR A's validation). + Tested via hand-built `k=1,2,3,6` configs registered through + `query_configs`, covering both instant and range-query callers of the + shared walk helper, plus the "sum of `k` strided buckets, not all + `S`-spaced buckets" regression assertion from the Test plan. +3. **PR C — §6, `cleanup.rs` (stacked on B).** Needs B's fetch shape final + for the `slide_interval_ms`-based `read_count_threshold` formula. Own + dedicated read-count accounting test (Test plan, last paragraph). +4. **PR D — §1, `capability_matching.rs` relaxation (stacked last, the + "activation" PR).** Only lands once B/C are proven — this is what + actually lets `find_compatible_aggregation` select a `k>1` Sliding + config, so capability matching never promises more than execution can + already deliver. + +Write each PR's test subset first, confirm it fails against the current code +for the reason the design predicts (e.g. PR B's `k>1` cases should fail with +"expected 1 precompute per key," not some unrelated error), then implement. +Stop after each PR for review/commit rather than landing the stack in one +shot. + +## Follow-up issues (not part of #557) + +- #558 — silent floor-alignment staleness (both the existing + ingestion-interval case and the new slide-interval case). +- #559 — legacy/unused code: `simple_map_store::legacy::{global, per_key}` + (referenced only by a comparative benchmark, never constructed in + production), the untested `CircularBuffer` cleanup policy, and (new, from + this revision) `query_precomputed_output_exact` / `is_exact_query: true` + becoming unused once §2's unification ships — add this as a fresh bullet + to #559 during implementation. diff --git a/asap-query-engine/src/engines/simple_engine/mod.rs b/asap-query-engine/src/engines/simple_engine/mod.rs index 501f721..481a32d 100644 --- a/asap-query-engine/src/engines/simple_engine/mod.rs +++ b/asap-query-engine/src/engines/simple_engine/mod.rs @@ -1281,13 +1281,20 @@ impl SimpleEngine { // Range Query Support // ============================================================ - /// Validate range query parameters + /// Validate range query parameters. + /// + /// Sliding checks `step % slide_interval_ms`, not `window_size_ms`: the + /// per-step walk in `execute_range_query_pipeline` floor-aligns only the + /// first step to the storage grid, and `S | W` always holds for a valid + /// sliding config, so `step % slide_interval_ms == 0` is what keeps + /// every later step grid-aligned. See design doc for #557. fn validate_range_query_params( - &self, start: u64, end: u64, step: u64, - tumbling_window_ms: u64, + window_size_ms: u64, + window_type: WindowType, + slide_interval_ms: u64, ) -> Result<(), String> { if start >= end { return Err("start must be before end".to_string()); @@ -1295,11 +1302,23 @@ impl SimpleEngine { if step == 0 { return Err("step must be positive".to_string()); } - if !step.is_multiple_of(tumbling_window_ms) { - return Err(format!( - "step ({} ms) must be a multiple of tumbling window size ({} ms)", - step, tumbling_window_ms - )); + match window_type { + WindowType::Tumbling => { + if !step.is_multiple_of(window_size_ms) { + return Err(format!( + "step ({} ms) must be a multiple of tumbling window size ({} ms)", + step, window_size_ms + )); + } + } + WindowType::Sliding => { + if !step.is_multiple_of(slide_interval_ms) { + return Err(format!( + "step ({} ms) must be a multiple of slide interval ({} ms)", + step, slide_interval_ms + )); + } + } } Ok(()) } @@ -1584,6 +1603,117 @@ impl SimpleEngine { } } +#[cfg(test)] +mod validate_range_query_params_tests { + use super::*; + + #[test] + fn tumbling_accepts_step_that_is_multiple_of_window_size() { + assert!(SimpleEngine::validate_range_query_params( + 0, + 600_000, + 300_000, + 300_000, + WindowType::Tumbling, + 300_000, + ) + .is_ok()); + } + + #[test] + fn tumbling_rejects_step_that_is_not_multiple_of_window_size() { + let err = SimpleEngine::validate_range_query_params( + 0, + 600_000, + 200_000, + 300_000, + WindowType::Tumbling, + 300_000, + ) + .unwrap_err(); + assert!(err.contains("tumbling window size")); + } + + #[test] + fn sliding_accepts_step_that_is_multiple_of_slide_interval_but_not_window_size() { + // W=300_000 (5m), S=60_000 (1m). step=120_000 (2m) is a + // multiple of S but NOT of W - Sliding must accept it even + // though Tumbling, with the same W, would reject it. + assert!(SimpleEngine::validate_range_query_params( + 0, + 600_000, + 120_000, + 300_000, + WindowType::Sliding, + 60_000, + ) + .is_ok()); + + assert!(SimpleEngine::validate_range_query_params( + 0, + 600_000, + 120_000, + 300_000, + WindowType::Tumbling, + 60_000, + ) + .is_err()); + } + + #[test] + fn sliding_rejects_step_that_is_not_multiple_of_slide_interval() { + let err = SimpleEngine::validate_range_query_params( + 0, + 600_000, + 90_000, + 300_000, + WindowType::Sliding, + 60_000, + ) + .unwrap_err(); + assert!(err.contains("slide interval")); + } + + #[test] + fn sliding_accepts_step_equal_to_slide_interval() { + assert!(SimpleEngine::validate_range_query_params( + 0, + 600_000, + 60_000, + 300_000, + WindowType::Sliding, + 60_000, + ) + .is_ok()); + } + + #[test] + fn start_must_be_before_end() { + assert!(SimpleEngine::validate_range_query_params( + 600_000, + 600_000, + 60_000, + 300_000, + WindowType::Tumbling, + 300_000, + ) + .is_err()); + } + + #[test] + fn step_must_be_positive() { + assert!(SimpleEngine::validate_range_query_params( + 0, + 600_000, + 0, + 300_000, + WindowType::Tumbling, + 300_000, + ) + .is_err()); + } +} + #[cfg(test)] mod range_query_tests { use crate::data_model::{AggregateCore, AggregationType, KeyByLabelValues, SerializableToSink}; diff --git a/asap-query-engine/src/engines/simple_engine/promql.rs b/asap-query-engine/src/engines/simple_engine/promql.rs index a9a2d54..d0212e8 100644 --- a/asap-query-engine/src/engines/simple_engine/promql.rs +++ b/asap-query-engine/src/engines/simple_engine/promql.rs @@ -497,19 +497,26 @@ impl SimpleEngine { let end_ms = Self::convert_query_time_to_data_time(end); let step_ms = (step * 1000.0) as u64; - let tumbling_window_ms = self + let (tumbling_window_ms, window_type, slide_interval_ms) = self .streaming_config .read() .unwrap() .get_aggregation_config(base_context.agg_info.aggregation_id_for_value) - .map(|c| c.window_size_ms)?; + .map(|c| (c.window_size_ms, c.window_type, c.slide_interval_ms))?; - self.validate_range_query_params(start_ms, end_ms, step_ms, tumbling_window_ms) - .map_err(|e| { - warn!("Range arm query validation failed: {}", e); - e - }) - .ok()?; + SimpleEngine::validate_range_query_params( + start_ms, + end_ms, + step_ms, + tumbling_window_ms, + window_type, + slide_interval_ms, + ) + .map_err(|e| { + warn!("Range arm query validation failed: {}", e); + e + }) + .ok()?; let lookback_ms = base_context.store_plan.values_query.end_timestamp - base_context.store_plan.values_query.start_timestamp; @@ -1114,20 +1121,33 @@ impl SimpleEngine { let step_ms = (step * 1000.0) as u64; // Get window size - let tumbling_window_ms = self + let (tumbling_window_ms, window_type, slide_interval_ms) = self .streaming_config .read() .unwrap() .get_aggregation_config(base_context.agg_info.aggregation_id_for_value) - .map(|config| config.window_size_ms)?; + .map(|config| { + ( + config.window_size_ms, + config.window_type, + config.slide_interval_ms, + ) + })?; // Validate parameters - self.validate_range_query_params(start_ms, end_ms, step_ms, tumbling_window_ms) - .map_err(|e| { - warn!("Range query validation failed: {}", e); - e - }) - .ok()?; + SimpleEngine::validate_range_query_params( + start_ms, + end_ms, + step_ms, + tumbling_window_ms, + window_type, + slide_interval_ms, + ) + .map_err(|e| { + warn!("Range query validation failed: {}", e); + e + }) + .ok()?; // Calculate lookback from the base context's store plan let lookback_ms = base_context.store_plan.values_query.end_timestamp