From 7051dae3b7bef53ef3f2b5686e6d42c92ccebcf5 Mon Sep 17 00:00:00 2001 From: zz_y Date: Fri, 15 May 2026 18:29:38 -0600 Subject: [PATCH] fix(query): dispatch Vector vs Matrix in asap_tier_result_to_query_result MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes the response-serialization gap that #255's PR-body flagged as the last blocker for Test 5 (HLL roundtrip). With #255's analyzer + reducer fixes in place, the ASAP-tier engine path went the full distance — `idx.sids_for_policy(fp)` resolved correctly, `SketchReducer::evaluate` returned `Ok(...)` — but the HTTP response came back empty (`reqwest::Error: EOF while parsing a value`). Root cause: `asap_tier_result_to_query_result` unconditionally wrapped the result in `QueryResult::matrix(...)`. The Prometheus adapter's `format_success_response` requires `resultType: vector` for instant queries (those the analyzer marked `range_seconds == 0`, i.e. no `[range]` selector — `count(metric)`, `quantile(...)`, etc.) and `resultType: matrix` for range queries (`*_over_time(...)[range]`). A Matrix response for an instant request gets rejected with HTTP 500 and an empty body. Fix: thread the analyzer's `range_seconds` through to the result builder. If any candidate is range-shaped, build a Matrix; otherwise project the latest sample per series into an `InstantVectorElement` and wrap as `Vector` (with `now_ms` as the wire timestamp). `InstantVectorElement` doesn't carry a per-element `label_keys_override` today (only `RangeVectorElement` does, for the topk-`item`-key case) — labels render against the query-scoped `KeyByLabelNames` the serializer holds. Correct for cardinality (the only instant-vector consumer today); a future per-element override on `InstantVectorElement` is plumbable when needed. ## Test 5 (HLL e2e roundtrip) now passes strict success The full chain — controller plan → POST /api/v1/streaming-config → OTLP HLL DPs → window close → GET /api/v1/query?query=count(metric) → status: success — works end-to-end. All 5 e2e tests now pass: * Test 1: streaming-config round-trip (DDSketch) * Test 2: grouping plumb (zone label) * Test 3: full roundtrip DDSketch quantile → strict success * Test 4: full roundtrip KLL quantile → strict success * Test 5: full roundtrip HLL cardinality → strict success ## Tests - `cargo test --test e2e_controller_plans_and_backend_serves`: **5 passed; 0 failed; 0 ignored**. - Full sweep: lib 691 + bins 27 + integration tests all green. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../query_engines/asap_query_engine/engine.rs | 55 ++++++++++++++++++- ...e2e_controller_plans_and_backend_serves.rs | 40 ++++---------- 2 files changed, 62 insertions(+), 33 deletions(-) diff --git a/data_plane/src/query_engines/asap_query_engine/engine.rs b/data_plane/src/query_engines/asap_query_engine/engine.rs index b4ca7d14..4d02101f 100644 --- a/data_plane/src/query_engines/asap_query_engine/engine.rs +++ b/data_plane/src/query_engines/asap_query_engine/engine.rs @@ -3424,10 +3424,42 @@ fn stitch_warm_and_archive( fn asap_tier_result_to_query_result( result: crate::storage_engines::sketch_db::query::ASAPTierResult, - _now_ms: u64, + now_ms: u64, + is_range_query: bool, ) -> crate::query_engines::query_result::QueryResult { use crate::storage_engines::types::KeyByLabelValues; - use crate::query_engines::query_result::{QueryResult, RangeVectorElement}; + use crate::query_engines::query_result::{ + InstantVectorElement, QueryResult, RangeVectorElement, + }; + + // Instant-query result-shape: the Prometheus adapter's + // `format_success_response` rejects `Matrix` for queries the + // analyzer marked as instant (`range_seconds == 0`) — produces a + // 500 ”shape mismatch”. Project the per-series last sample into + // an `InstantVectorElement` and wrap as `Vector` so the wire + // response carries `resultType: vector` matching the request. + if !is_range_query { + let mut elements: Vec = Vec::with_capacity(result.series.len()); + for (label_values, samples) in result.series { + let (_keys, values): (Vec, Vec) = label_values.into_iter().unzip(); + let labels = KeyByLabelValues::new_with_labels(values); + // Take the latest sample (the reducer returns one per + // window_end; for instant readout we want the most recent). + // `InstantVectorElement` doesn't carry a per-element + // `label_keys_override` today (only `RangeVectorElement` + // does, for the topk-`item`-key case) — labels render + // with whatever query-scoped `KeyByLabelNames` the + // serializer holds. That's correct for the cardinality + // shape that's the only instant-vector consumer at the + // moment; if a future instant-vector readout needs + // per-element key remapping, add the override field on + // `InstantVectorElement` then plumb `keys` here. + if let Some((_, value)) = samples.into_iter().last() { + elements.push(InstantVectorElement::new(labels, value)); + } + } + return QueryResult::vector(elements, now_ms); + } let mut elements: Vec = Vec::with_capacity(result.series.len()); for (label_values, samples) in result.series { @@ -3553,6 +3585,16 @@ impl crate::query_engines::routing::query_engine_routing::QueryEngine for ASAPQu let mut combined_result: Option = None; let mut combined_t0: u64 = u64::MAX; + // Track whether ANY candidate is range-vector-shaped + // (`range_seconds > 0`). Drives the Vector-vs-Matrix + // result-shape choice in `asap_tier_result_to_query_result` + // below — instant queries (`count(metric)`, + // `quantile(...)` without `_over_time` etc.) need + // `QueryResult::Vector` so the Prometheus adapter's + // `format_success_response` wraps them as `resultType: + // vector`. Returning `Matrix` for an instant query + // produces a 500 (adapter rejects the shape mismatch). + let mut any_range_candidate = false; // Snapshot the streaming config once for this query's // policy lookups. Hot-reload swaps the underlying Arc; the @@ -3561,6 +3603,9 @@ impl crate::query_engines::routing::query_engine_routing::QueryEngine for ASAPQu let policy_registry = streaming_snap.policy_registry(); for candidate in &analysis.candidates { + if candidate.range_seconds > 0 { + any_range_candidate = true; + } // Content-addressed sid lookup: find matching policies // in the registry → resolve each policy_fp → {sids} // via the reverse index. Both hops are O(1)-amortized. @@ -3722,7 +3767,11 @@ impl crate::query_engines::routing::query_engine_routing::QueryEngine for ASAPQu // QueryResult and run the hybrid-stitch path if archive // is wired and warm coverage is narrower than request. if let Some(result) = combined_result { - let warm_qr = asap_tier_result_to_query_result(result.clone(), now_ms); + let warm_qr = asap_tier_result_to_query_result( + result.clone(), + now_ms, + any_range_candidate, + ); if let (Some((cov_lo, cov_hi)), Some(archive)) = (result.coverage, self.archive_engine.as_ref()) { 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 466c75a1..07e3da1a 100644 --- a/data_plane/tests/e2e_controller_plans_and_backend_serves.rs +++ b/data_plane/tests/e2e_controller_plans_and_backend_serves.rs @@ -968,37 +968,17 @@ async fn controller_plan_to_query_full_roundtrip_kll() { // // HLL backs the cardinality readout. The workload pins HLL via // `sketch_type_override: Some(SketchType::HLL)`. The OTLP DP carries -// a `HllSketchDataPoint` with `HyperLogLogState`. +// a `HllSketchDataPoint` with `HyperLogLogState`. PromQL's +// `count(metric)` is the spec's distinct-counting idiom — returns +// the number of distinct label sets in the result vector — which +// the analyzer routes to `Capability::CardinalityApprox` and the +// reducer dispatches to the HLL cardinality readout. // -// PromQL's `count(metric)` is the spec's distinct-counting idiom — -// it returns the number of distinct label sets in the result vector. -// Three engine-side fixes were needed (alongside this PR): -// -// 1. **Analyzer (`walk_qe::Expr::VectorSelector`)** — gated the -// implicit `Aggregate(Sum)` wrapper on `!ctx.outer_count` so a -// bare selector under `count(...)` doesn't synthesize a spurious -// `ExactAgg(Sum)` candidate that fails the engine's -// "all candidates must succeed" loop. -// 2. **Reducer (`function_to_family`)** — added `"count"` as an -// alias for `QueryFamily::Cardinality`. -// 3. **Test setup** — OTLP DP precision must match what the -// controller plans (`HLLDefaults`); start_time must be near -// end_time so the stored window falls within the query's -// lookback range. -// -// **Currently `#[ignore]`'d.** With all three fixes in place the -// engine path now goes the distance: streaming-config registers, -// OTLP DP lands in `SketchStore`, sids share the right `policy_fp`, -// reducer.evaluate returns `Ok(...)`. But the HTTP response body -// comes back empty / fails JSON decode (`reqwest::Error: EOF while -// parsing a value`) — the response-serialization path for -// instant-vector cardinality results has a separate bug worth its -// own follow-up. Tracked via the diagnostic comments above and the -// engine-debug prints kept in the engine path's git history. - -#[ignore = "HLL roundtrip — analyzer, policy match, reducer all succeed; \ - HTTP response body is empty. Separate serialization bug \ - in the cardinality response path."] +// Closed by a chain of fixes: +// * `count(metric)` analyzer fix (PR #255) +// * `count` reducer alias (PR #255) +// * Vector-vs-Matrix instant-query response shape fix (this PR) + #[tokio::test(flavor = "multi_thread", worker_threads = 4)] async fn controller_plan_to_query_full_roundtrip_hll() { let stack = start_full_stack(19_565, 19_566).await;