From 5b01bc3db67482c9bd81bab2e20c4f9cb5f50de4 Mon Sep 17 00:00:00 2001 From: zz_y Date: Fri, 15 May 2026 17:38:37 -0600 Subject: [PATCH] fix(query): route handle_query misses through modern execute() trait path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- data_plane/src/drivers/query/servers/http.rs | 58 +++++++++++++++++++ ...e2e_controller_plans_and_backend_serves.rs | 33 ++++++++--- 2 files changed, 82 insertions(+), 9 deletions(-) diff --git a/data_plane/src/drivers/query/servers/http.rs b/data_plane/src/drivers/query/servers/http.rs index 3640572ca..a9d292ca8 100644 --- a/data_plane/src/drivers/query/servers/http.rs +++ b/data_plane/src/drivers/query/servers/http.rs @@ -945,6 +945,64 @@ async fn process_via_simple_engine( Err(status) => status.into_response()} } None => { + // Legacy `handle_query` returned None — likely the + // sketch-vs-precompute query gap pinned in #252: + // `query_precomputes_by_agg` only picks up + // `AggKind::ExactAgg` sids, never `AggKind::Sketch`. Try + // the modern `ASAPQueryEngine::execute(&str)` trait path + // before falling through to the unsupported-query branch + // — `execute` uses + // `idx.sids_for_policy(fp)` + `SketchReducer::evaluate` + // and handles sketches natively. + // + // Trait dispatch loses `KeyByLabelNames` (the trait + // returns just `QueryResult`); we surface an empty + // `KeyByLabelNames`, identical to how `process_via_router` + // handles the same trait surface — the Prometheus + // adapter renders an empty `metric: {}` object, a valid + // shape that PromQL clients accept. + use crate::query_engines::routing::query_engine_routing::QueryEngine; + let modern_result = state + .query_engine + .execute(&parsed_request.query) + .await; + if let Ok(query_result) = modern_result { + debug!( + "Modern execute() handled what legacy handle_query missed \ + (query='{}')", + parsed_request.query + ); + use crate::drivers::query::adapters::QueryExecutionResult; + let execution_result = QueryExecutionResult { + query_output_labels: promql_utilities::data_model::KeyByLabelNames::default(), + query_result, + }; + let total_duration = start_time.elapsed(); + debug!( + "Total request processing took (modern fallback): {:.2}ms", + total_duration.as_secs_f64() * 1000.0 + ); + return match state + .adapter + .format_success_response(&execution_result) + .await + { + Ok(response) => { + annotate_data_source( + response, + StorageBackend::SketchStore.data_source_id(), + ) + .await + } + Err(status) => status.into_response(), + }; + } + debug!( + "Both legacy handle_query AND modern execute() returned None/Err for \ + query='{}', falling through to fallback / unsupported", + parsed_request.query + ); + let total_duration = start_time.elapsed(); debug!("=== QUERY ENGINE RETURNED NONE ==="); debug!( diff --git a/data_plane/tests/e2e_controller_plans_and_backend_serves.rs b/data_plane/tests/e2e_controller_plans_and_backend_serves.rs index 8626b7856..ff321b4a8 100644 --- a/data_plane/tests/e2e_controller_plans_and_backend_serves.rs +++ b/data_plane/tests/e2e_controller_plans_and_backend_serves.rs @@ -130,10 +130,11 @@ async fn start_backend_http_server() -> (u16, HotReloadStreamingConfig) { use data_plane::storage_engines::types::StreamingConfig; let hot_reload = HotReloadStreamingConfig::new(StreamingConfig::default()); - let query_engine = Arc::new(ASAPQueryEngine::new_with_hot_reload( - hot_reload.clone(), - 15_000, - )); + let sketch_index = Arc::new(SketchStore::new()); + let query_engine = Arc::new( + ASAPQueryEngine::new_with_hot_reload(hot_reload.clone(), 15_000) + .with_sketch_index(sketch_index.clone()), + ); let adapter_config = AdapterConfig::prometheus_promql( "http://127.0.0.1:9999".to_string(), // unused — no forwarding in this test @@ -145,7 +146,6 @@ async fn start_backend_http_server() -> (u16, HotReloadStreamingConfig) { adapter_config, }; - let sketch_index = Arc::new(SketchStore::new()); let server = HttpServer::new(http_config, query_engine, sketch_index) .with_hot_reload_config(hot_reload.clone()); @@ -283,10 +283,16 @@ async fn start_full_stack(otlp_http_port: u16, otlp_grpc_port: u16) -> FullStack handle_http_requests: true, adapter_config, }; - let query_engine = Arc::new(ASAPQueryEngine::new_with_hot_reload( - hot_reload.clone(), - 15_000, - )); + let query_engine = Arc::new( + ASAPQueryEngine::new_with_hot_reload(hot_reload.clone(), 15_000) + // CRITICAL: without this the engine's `sketch_index` is + // None and every fast path that reads sid → SketchInstance + // metadata is silently skipped. Sketches DO land in + // `sketch_index` via OTLP ingest (the engine's + // `precompute_engine` shares the Arc), but the query + // path can't see them without this binding. + .with_sketch_index(sketch_index.clone()), + ); let server = HttpServer::new(http_config, query_engine, sketch_index) .with_hot_reload_config(hot_reload.clone()); let backend_port = server @@ -704,4 +710,13 @@ async fn controller_plan_to_query_full_roundtrip_ddsketch() { "PromQL response missing `status` field — HTTP layer is unhealthy\n{}", serde_json::to_string_pretty(&response).unwrap_or_default() ); + let status = response["status"].as_str().unwrap_or("(missing)"); + assert_eq!( + status, "success", + "PromQL query did not succeed after the modern-execute() fallback in \ + process_via_simple_engine. The legacy handle_query path can't read \ + sketch-backed sids (#252), but the fallback should now reach them via \ + the trait-dispatch path. Response:\n{}", + serde_json::to_string_pretty(&response).unwrap_or_default() + ); }