fix(query): route handle_query misses through modern execute() trait path - #253
Merged
Merged
Conversation
…path Closes the gap pinned in #252: the legacy ASAPQueryEngine::handle_query path can't read sketch-backed sids (`query_precomputes_by_agg` only matches `AggKind::ExactAgg`), but the modern `execute(&str)` trait path (engine.rs:3430) DOES — it dispatches via `find_matching_policies` → `idx.sids_for_policy(fp)` → `SketchReducer::evaluate` and handles sketches natively. `process_via_simple_engine` (the HTTP handler for SketchStore-routed queries) now falls back to `execute(&str)` when `handle_query` returns None. Trait dispatch loses `KeyByLabelNames`; we surface `KeyByLabelNames::default()` (mirroring `process_via_router`'s identical handling on line 1171) — the Prometheus adapter renders the empty `metric: {}` shape, valid PromQL response. ## Setup-side fix (the actual bug Test 3 surfaced) The diagnostic also caught a missing `.with_sketch_index(idx)` call in the test harness — without it, the engine's `sketch_index` is `None` and EVERY fast path (`execute_store_query`, `execute(&str)`, `query_range`, …) is silently skipped because they all guard with `let Some(idx) = self.sketch_index.as_ref() else { return … };`. This is a load-bearing wiring step that production `data_plane/main.rs` gets right but `start_test_server` callers were missing. Both `start_backend_http_server` and `start_full_stack` now thread `with_sketch_index(sketch_index.clone())` so the engine's reads see the same store the OTLP receiver / precompute engine writes to. ## Test 3 strict assertion now active `controller_plan_to_query_full_roundtrip_ddsketch` previously soft-checked the response had a `status` field. After this PR it asserts `status == "success"` — the FULL e2e path (controller plan → POST /api/v1/streaming-config → OTLP DDSketch DPs → window close → GET /api/v1/query) now works end-to-end. This is the first time we have a green-bar e2e test for the gateway-less data path. ## Tests - `cargo test --test e2e_controller_plans_and_backend_serves`: 3 passed, 0 failed (was 2/3 with Test 3 soft-checked). - Full sweep: lib 690 + bins 27 + integration tests across the workspace all green. ## Out of scope The legacy `query_precomputes_by_agg` filter still doesn't include `AggKind::Sketch` (per #252's diagnostic comment). Closing that fully — so `handle_query` itself works for sketches — would let us delete this fallback. Not blocking; the fallback is a stable bridge. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 task
zzylol
added a commit
that referenced
this pull request
May 15, 2026
…ith-gap (#254) Extends the e2e test suite to cover the second and third sketch families (DDSketch is already strict-success-asserted in Test 3). ## Test 4 — controller_plan_to_query_full_roundtrip_kll (passes) Mirrors Test 3 with `sketch_type_override: Some(SketchType::KLL)`. Builds a `KllState` via `asap_sketchlib::proto::sketchlib::KllState`, wraps in `KllSketchDataPoint`, OTLP-POSTs, watermark-advances, queries `quantile_over_time(0.5, request_size_bytes[10s])`, asserts `status == "success"`. Also asserts the controller emits `aggregationType: DatasketchesKLL` for the override. This validates the second of the five sketch families on the gateway-less data path. The `execute(&str)` trait dispatcher (#253's fallback) handles KLL identically to DDSketch — both route through `SketchReducer::evaluate` with quantile readout dispatch. ## Test 5 — controller_plan_to_query_full_roundtrip_hll (#[ignore]'d) Workload pins HLL with `AggType::Cardinality`. The streaming-config registration succeeds (`aggregationType: HLL` confirmed) and the sketch state lands in `SketchStore` — but the PromQL query path for HLL needs a query shape the analyzer recognises as `Capability::CardinalityApprox`. `count(metric)` doesn't map today: `resolve_sketch_metric_alias` only rewrites `count(metric)` → `count(metric_hll)` when the bare metric is ABSENT (a deploy-time aliasing tactic). With bare registered, alias is a no-op and the analyzer doesn't bind `count(...)` to the cardinality capability. `#[ignore]`'d with a precise note pointing at the gap. The OTLP ingest path for HLL works (no `#[ignore]` on ingest); the readout side needs follow-up analyzer / alias work. ## Helper changes - Splits `build_workload` into `build_workload` (no override) and `build_workload_with_override` (with `sketch_type_override`). - Adds `build_kll_state` / `build_kll_export` / `build_hll_state` / `build_hll_export` mirrors of the existing DDSketch helpers. ## Tests - `cargo test --test e2e_controller_plans_and_backend_serves`: **4 passed; 0 failed; 1 ignored** (HLL). ## What's left for full 5-sketch coverage CountSketch (topk) and CountMinSketch (frequency / count) need their own tests + likely similar analyzer routing work as HLL. Two paths to ship those: 1. Wait for the analyzer-side fix on HLL — same fix likely unblocks all three. 2. Use the demo-contract metric names (`top_endpoint_qps`, `endpoint_request_freq`) which `classify_demo_metric` routes directly, bypassing the analyzer's bare-name handling. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
3 tasks
zzylol
added a commit
that referenced
this pull request
May 16, 2026
Closes the range-query gap I documented in PR #260's session follow-up. Per-user direction (2026-05-16): count_over_time is warm-tier-answerable by sampling; topk_over_time isn't standard PromQL anyway and falls through to the cold tier (Thanos parse- error is acceptable). Sibling of PR #253's instant-query trait-dispatch fallback. New `ASAPQueryEngine::execute_range_promql_modern(query, start_ms, end_ms, step_ms)` mirrors the analyzer + reducer dispatch from the `QueryEngine::execute(&str)` trait surface but returns Matrix per the `/api/v1/query_range` wire-format spec. Wire-up: `process_range_query_request` in `http.rs` tries `handle_range_query_promql` first (legacy path, unchanged), and on `None` calls the new modern method. If the modern path also errors, it falls through to `format_unsupported_query_response` (which the EngineRouter can route to a cold-tier fallback). Prometheus semantics note: the spec says evaluate at each `t = start, start+step, …, end`. The warm tier returns samples at native window-close granularity instead — finer than the user's step when window_size < step (more data, not less). Step-precise evaluation is a future refinement; clients that need exact step timestamps can downsample, or route step- precise queries to the cold tier. Test 10 (`controller_plan_to_range_query_count_over_time_cms`) ingests heap-less CMS DPs via the same setup as Test 7, then queries `count_over_time(endpoint_request_freq[10s])` via `/api/v1/query_range`. Asserts `status=success`, `resultType=matrix`, non-empty series. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes the gap pinned in #252. The legacy
ASAPQueryEngine::handle_querypath can't read sketch-backed sids (query_precomputes_by_aggonly matchesAggKind::ExactAgg), but the modernexecute(&str)trait path (engine.rs:3430) DOES — it dispatches viafind_matching_policies→idx.sids_for_policy(fp)→SketchReducer::evaluateand handles sketches natively.process_via_simple_engine(HTTP handler for SketchStore-routed queries) now falls back toexecute(&str)whenhandle_queryreturns None. Trait dispatch losesKeyByLabelNames; we surfaceKeyByLabelNames::default()(mirroringprocess_via_router's identical handling) — the Prometheus adapter renders the emptymetric: {}shape, valid PromQL response.Setup-side bug that this also catches
Diagnostic also surfaced a missing
.with_sketch_index(idx)call in the test harness. Without it, the engine'ssketch_indexisNoneand every fast path (execute_store_query,execute(&str),query_range, …) is silently skipped — all guard withlet Some(idx) = self.sketch_index.as_ref() else { return … };. Productiondata_plane/main.rsgets this right;start_test_servercallers were missing it.Both test harness helpers now thread
with_sketch_index(sketch_index.clone())so the engine sees the same store OTLP ingest writes to.Test 3 strict assertion now active
controller_plan_to_query_full_roundtrip_ddsketchwas soft-checkingstatusfield exists. After this PR:status == "success"— the FULL e2e path (controller plan → POST/api/v1/streaming-config→ OTLP DDSketch DPs → window close → GET/api/v1/query) works end-to-end.This is the first green-bar e2e test for the gateway-less data path.
Test plan
cargo test --test e2e_controller_plans_and_backend_serves: 3 passed; 0 failed (was 2/3 with Test 3 soft-checked)Out of scope
The legacy
query_precomputes_by_aggfilter still doesn't includeAggKind::Sketch(per #252's diagnostic comment). Closing that fully would let us delete this fallback. Not blocking; the fallback is a stable bridge.🤖 Generated with Claude Code