Repository navigation
feat(query-engine): answer RQE expression trees on POST /api/v1/rqe_query - #889
Conversation
Add `asap_types::rqe_expr`: a serde type for RQE expression trees (rqe leaves, agg, binary, scalar nodes), a `validate` check run at load, and `leaves` / `output_labels` helpers. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011gKTCAEz3sJ1Hzde16xxU1
An e2e query's optional `expr` (an RQE expression tree, validated at load) now names the plan RQEs it reads. Each tree leaf becomes one more optimizer item and Raqe, served by its plan pick like a text leaf, and gets an entry in a new `rqe_configs` section of the inference config: RQE id -> the aggregations it reads and, for an exact roll-up, the query-time stage that regroups to the RQE's grouping. `query_configs` are unchanged; a table without trees converts as before and writes no `rqe_configs`. A query may have only a tree: its language is that of the metrics its leaves read (records -> SQL). The engine's `InferenceConfig` loads `rqe_configs` (empty when absent); nothing reads it yet. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011gKTCAEz3sJ1Hzde16xxU1
…uery `SimpleEngine::handle_rqe_query` evaluates a validated RQE tree at a query time without parsing PromQL/SQL. A leaf reads the aggregations its `rqe_configs` entry names, over the lookback ending at the time aligned as the PromQL instant path aligns it, through the shared context tail and executor; a roll-up config reads at the aggregation's grouping and its query-time stages regroup to the leaf's. `agg` nodes regroup with the query-time aggregation pipeline, `binary` nodes reuse PromQL's vector and scalar combiners. Output labels are the root's labels, never `__name__`. The route answers Prometheus instant-vector JSON whatever the adapter's language and never forwards: invalid body/tree, unknown RQE id or bad time is a 400 `bad_data`, an execution failure a 500, an empty window an empty result. Top-k leaves rank globally (a leaf has no bucketing labels). The SQL context tail moves to `build_execution_context_tail` (now returning the planning error) and `get_aggregation_id_info` takes aggregation refs, so both the SQL and RQE builders share them. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011gKTCAEz3sJ1Hzde16xxU1
An RQE leaf may name `topk_by` labels: like PromQL `topk by (...)`, a top-k leaf then ranks its input independently within each value combination of those labels, while `grouping` stays its output labels. Absent, the whole input is one bucket. Validation allows `topk_by` only on `topk_by_value` leaves, non-empty and within `grouping`. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011gKTCAEz3sJ1Hzde16xxU1
…tion' into 882-featplanner-planner-saved-reads-rqe-trees-and-writes-rqe_configs
A top-k leaf's optimizer item now carries its `topk_by` labels as `topk_by_labels`, as a PromQL `topk by (...)` query's does, so its Raqe is grouped by the buckets (the plan's top-k RQE grouping) and its heap is one per bucket, keyed by the remaining labels. Without `topk_by` the Raqe is a single bucket and can't share the deployment of a `topk by (...)` query. Tests run the pilot random workload (seed-862/workload-01-pilot) and its saved plans: `topk by (region, service)` leaves read the same heaps as their PromQL queries. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011gKTCAEz3sJ1Hzde16xxU1
…s-rqe_configs' into 883-featquery-engine-rqe-tree-query-route
A top-k leaf now ranks per bucket the way the PromQL path ranks `topk by (...)`: its aggregation keeps one heap per bucket (the aggregation's grouping) and the instant plan's top-k limit ranks within each. The leaf's `topk_by` (none: one bucket) must equal that grouping; otherwise the leaf fails as an execution error instead of answering a different ranking. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011gKTCAEz3sJ1Hzde16xxU1
milindsrivastava1997
left a comment
There was a problem hiding this comment.
Code review: 9 findings.
| return Err(failed(format!( | ||
| "read labels {:?}, not the leaf's grouping {:?}", | ||
| labels.labels, leaf_labels.labels | ||
| )) |
There was a problem hiding this comment.
For a non-roll-up config the final labels != leaf_labels check can never fail: read_labels is set to leaf_labels, and execute_context_result returns context.metadata.query_output_labels unchanged, so a leaf grouping that differs from the aggregation's grouping_labels/aggregated_labels is never detected.
Failure scenario: Aggregation grouped by [job], leaf grouping [instance]. The single-population fallback emits store keys (job values) labelled as instance with a 200. With self-keyed accumulators, merge_grouped_and_self_keyed drops every key with a warning, giving an empty 200. Compare leaf.grouping against value_config.grouping_labels (+ aggregated_labels) before executing.
There was a problem hiding this comment.
Fixed in a791ac5: before any read, a leaf's grouping must equal the aggregation's grouping plus aggregated labels, or what its roll-up stages produce. Otherwise 400 (test: refuses_a_grouping_its_aggregation_does_not_serve, including the keyed-sum case). The old post-execution check is now a debug_assert.
| end_timestamp, | ||
| }, | ||
| metadata, | ||
| agg_info, |
There was a problem hiding this comment.
Only leaf.id is used to look up the config. The leaf's metric, capability and lookback_ms are never checked against the aggregation rqe_configs[id] points at; value_config.metric is always read.
Failure scenario: id "q0" planned as sum_over_time on http_requests, sent with metric "other" or capability quantile or a different lookback. The engine reads http_requests anyway; a quantile against a Sum accumulator fails per key and is skipped with a warning, so the client gets an empty 200 or wrong values instead of a 400.
There was a problem hiding this comment.
Fixed in a791ac5: the metric (table.value_column for SQL) and capability (aggregation type, value-weighted for SUM/COUNT modes) are checked against the plan's aggregation; a mismatch returns 400.
| } | ||
|
|
||
| let end_timestamp = self.align_end_timestamp_promql(query_time); | ||
| let start_timestamp = end_timestamp.checked_sub(leaf.lookback_ms).ok_or_else(|| { |
There was a problem hiding this comment.
lookback_ms is not validated as > 0 or as a multiple of the aggregation's window/slide.
Failure scenario: lookback_ms: 0 gives start == end; no buckets are found, NoLocalData maps to Ok(None), and the leaf returns an empty vector with HTTP 200, indistinguishable from 'no data yet'.
| return Err(RqeQueryError::BadRequest( | ||
| "an RQE expression must read at least one RQE".to_string(), | ||
| )); | ||
| } |
There was a problem hiding this comment.
The comment says "One snapshot of the configs, so every leaf reads the same plan", but only inference_config is snapshotted. streaming_config is re-read separately per leaf in get_aggregation_id_info, evaluate_rqe_leaf, build_execution_context_tail and create_store_query_plan.
Failure scenario: update_streaming_config lands mid-evaluation of a multi-leaf tree; leaves (or agg_info vs value_config of one leaf) resolve against different configs, giving a spurious 500 ('no streaming config for aggregation_id') or the two sides of a binary op from different plans. Read streaming_config once in handle_rqe_query and pass it down.
There was a problem hiding this comment.
Fixed in a791ac5, comment only: configs don't change while a tree is answered, so it no longer claims a snapshot.
| /// query-time stages regroup to the leaf's. A top-k leaf ranks within | ||
| /// each bucket of its aggregation's grouping, as PromQL `topk by` does, | ||
| /// so that grouping must be the leaf's `topk_by`. | ||
| fn evaluate_rqe_leaf( |
There was a problem hiding this comment.
Request/config mismatch errors (topk_by vs heap grouping, leaf grouping mismatch, labels the pipeline can't regroup) are wrapped with failed → QueryExecutionError::Native → HTTP 500 instead of 400.
Failure scenario: A top-k leaf with topk_by [region] against a global heap config gets 500 'internal'. Clients/the E2E harness treating 5xx as engine bugs misclassify it; contradicts the PR's contract that an invalid tree gets 400 bad_data.
There was a problem hiding this comment.
Fixed in a791ac5: topk_by, grouping and unregroupable-label mismatches return 400 bad_data. Engine faults (missing streaming config, invalid stage config) stay 500.
| ) -> Result<RqeValue, RqeQueryError> { | ||
| match expr { | ||
| RqeExpr::Rqe { rqe } => { | ||
| let (labels, values) = |
There was a problem hiding this comment.
The agg node calls apply_instant_pipeline, which rejects non-finite values, so a legal tree such as avg(a / b) with any b == 0 fails the whole query with 500 where Prometheus returns Inf/NaN.
Failure scenario: {"agg":"avg","by":[],"of":{"op":"/","lhs":leafA,"rhs":leafB}} with one series leafB == 0 → Err('query-time aggregations cannot process non-finite values') → HTTP 500. Listed as known in the PR, but still diverges from PromQL for valid input.
There was a problem hiding this comment.
Deferring. No current workload aggregates over a division, so non-finite values can't occur. Recorded on #886 as a known divergence from PromQL.
| values: lhs_values, | ||
| }, | ||
| RqeValue::Vector { | ||
| labels: rhs_labels, |
There was a problem hiding this comment.
validate() compares binary-op label sets with BTreeSet equality (ignores duplicates), while combine_vector_vector compares sorted label Vecs positionally.
Failure scenario: Leaf grouping ["job","job"] combined with agg by ["job"] passes validate, then combine_vector_vector returns None at runtime → 400 'different labels' after store reads already ran. Validation (#887) should reject duplicate labels.
| let missing = | ||
| |field: &str| RqeQueryError::BadRequest(format!("RQE {:?}: missing {field}", leaf.id)); | ||
| Ok(match leaf.capability { | ||
| RqeCapability::Sum => (Statistic::Sum, HashMap::new()), |
There was a problem hiding this comment.
The missing("phi")/missing("k") paths in leaf_statistic, and the root RqeValue::Scalar arm in handle_rqe_query, are unreachable: validate() already requires phi/k and rejects scalar-only trees. Dead error paths blur which layer owns validation; use expect on the invariant or remove them.
There was a problem hiding this comment.
Fixed in a791ac5: expect/unreachable! with the invariant stated, since validation guarantees them.
| /// A no-op in the common case (already-aligned timestamps), a safety net | ||
| /// otherwise, for every PromQL query shape, not just a subset. | ||
| fn align_end_timestamp_promql(&self, end_timestamp: u64) -> u64 { | ||
| pub(super) fn align_end_timestamp_promql(&self, end_timestamp: u64) -> u64 { |
There was a problem hiding this comment.
RQE leaves reuse align_end_timestamp_promql, whose warning says 'PromQL query end timestamp ... not aligned', so every unaligned RQE request logs a misleading PromQL warning. Make the message language-neutral or move the aligner to mod.rs.
There was a problem hiding this comment.
Fixed in a791ac5: the warning is now language-neutral ("Query end timestamp …").
Require every subtree that feeds the result (root, agg operand, one side of each binary op) to read at least one RQE leaf; allow a repeated leaf id only for identical leaves; reject duplicate labels in grouping, by and topk_by; reject an empty id or metric and a zero lookback. Document that label lists are sets matched by name. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011gKTCAEz3sJ1Hzde16xxU1
…tion' into 882-featplanner-planner-saved-reads-rqe-trees-and-writes-rqe_configs
- Compare a query's `rqes` with its tree's leaves as sets. - Allow one RQE leaf at several repeat intervals: each interval is a Raqe, and the interval-independent rqe_configs entry is written once. Different leaves under one id are still refused. - Name a query in errors by its text, else its tree's RQE ids. - Compute and check a query's RQE ids once. - Check a tree's leaves read one kind of metric without dedup(). - Describe the e2e-tables fixture instead of citing its PR. - --expr-only (with --planner saved) plans only the e2e queries that have an RQE tree; the others (e.g. rate) get no deployment. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011gKTCAEz3sJ1Hzde16xxU1
…s-rqe_configs' into 883-featquery-engine-rqe-tree-query-route
Before reading a leaf, check it against the aggregation its rqe_configs entry names: same metric (table.value_column for SQL), a type that serves its capability with value weighting, a lookback its windows tile, top-k buckets equal to the heap grouping, and a grouping the aggregation serves (its own labels, a mergeable subset of its groups, or what its query-time stages regroup to). A mismatch is the request's fault: 400 bad_data, not 500. Also: drop error paths validation already rules out (missing phi/k, scalar root), make the end-timestamp alignment warning language-neutral, and stop claiming a config snapshot. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011gKTCAEz3sJ1Hzde16xxU1
milindsrivastava1997
left a comment
There was a problem hiding this comment.
Automated code review: 5 finding(s), unverified candidates.
| ), | ||
| )); | ||
| } | ||
| if !window_compatible(aggregation, leaf.lookback_ms) { |
There was a problem hiding this comment.
A leaf's lookback_ms is only checked for window compatibility, never against the num_aggregates_to_retain its rqe_configs entry was planned with.
Failure scenario: Planner sizes a sum RQE for a 5m lookback over 1m-sliding 5m windows (retains 1 closed window). A client POSTs the same id with lookback_ms=900000 (15m): passes window_compatible, but the buffer holds only the newest window(s), so the endpoint returns a silently under-counted sum with 200 instead of a 400.
(from automated /code-review; unverified)
There was a problem hiding this comment.
Fixed in c2064d2: a leaf needing more windows than its rqe_configs entry retains gets a 400. The window count follows the planner's closed_instance_count; unbounded retention skips the check.
| /// `POST /api/v1/rqe_query`: answers with Prometheus instant-vector JSON. Never | ||
| /// forwards to a fallback backend: an invalid request is a 400, a failed | ||
| /// execution a 500, and a window without data yet an empty result. | ||
| async fn handle_rqe_query(State(state): State<AppState>, body: Bytes) -> Response { |
There was a problem hiding this comment.
handle_rqe_query skips the state.config.handle_http_requests check that process_query_request applies to every other query route.
Failure scenario: With handle_http_requests=false, POST /api/v1/rqe_query still evaluates RQE trees against the local store, bypassing the disable switch for this route only.
(from automated /code-review; unverified)
There was a problem hiding this comment.
Fixed in c2064d2: with handle_http_requests off, the route returns an internal error ('RQE query handling is disabled'), as the instant route does when it has no fallback.
| let agg_info = self | ||
| .get_aggregation_id_info(&config.aggregations) | ||
| .map_err(failed)?; | ||
| let streaming_config = self.streaming_config.read().unwrap().clone(); |
There was a problem hiding this comment.
evaluate_rqe_leaf clones the whole StreamingConfig per leaf, and build_execution_context_tail / create_store_query_plan each clone it again: 3 full clones per leaf per request on the query hot path.
Failure scenario: With hundreds of aggregation configs, a 4-leaf tree deep-copies the full config 12x per request, adding latency to the path RQE benchmarks measure. Holding the read guard or cloning only the needed AggregationConfig avoids it.
(from automated /code-review; unverified)
There was a problem hiding this comment.
The premise doesn't hold: streaming_config is RwLock<Arc<StreamingConfig>>, so .clone() copies a pointer, not the config. Even so, c2064d2 takes one snapshot per request and passes it to every leaf.
| /// `POST /api/v1/rqe_query`: answers with Prometheus instant-vector JSON. Never | ||
| /// forwards to a fallback backend: an invalid request is a 400, a failed | ||
| /// execution a 500, and a window without data yet an empty result. | ||
| async fn handle_rqe_query(State(state): State<AppState>, body: Bytes) -> Response { |
There was a problem hiding this comment.
handle_rqe_query never calls state.query_tracker.record_instant, unlike the instant/range handlers.
Failure scenario: With the query tracker enabled for passive auto-discovery, rqe-frontend workloads are invisible to it, so auto-planning never sees them.
(from automated /code-review; unverified)
There was a problem hiding this comment.
Declining. The tracker discovers PromQL strings for passive auto-planning, and RQE trees can't be planned that way; trees come from saved plans.
| /// never `__name__`: every leaf reads a range like `sum_over_time`, whose | ||
| /// result Prometheus strips of its name. A leaf whose window holds no | ||
| /// data yet contributes an empty vector. | ||
| pub fn handle_rqe_query( |
There was a problem hiding this comment.
A leaf's k (top-k) and phi are never compared with what its RQE's plan was sized for; only metric, aggregation type, windows and topk buckets are checked.
Failure scenario: Plan sizes a CMS-heap for k=5; a request with the same id and k=500 passes every check and returns a top-500 from a too-small heap: silently wrong ranks/values, no 400.
(from automated /code-review; unverified)
There was a problem hiding this comment.
Fixed for k in c2064d2: a top-k leaf whose k exceeds its aggregation's heapsize gets a 400. The planner's own rule is stricter (heap ≥ merged windows × k), so this catches the gross case. phi: declining, since quantile sketches answer any φ.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011gKTCAEz3sJ1Hzde16xxU1
…tion' into 882-featplanner-planner-saved-reads-rqe-trees-and-writes-rqe_configs
- A query with `promql`/`sql` must name its `rqes`: text leaves map to plan RQEs by `rqes`, never by the tree's leaf order. Only a tree-only query may omit them. - `--expr-only` no longer plans query text: it writes only the tree leaves' deployments and `rqe_configs`, and no `query_configs`. - `leaf_item` drops the lookback check `RqeExpr::validate` owns, and shares one metric lookup (`leaf_metric`) with `is_sql`. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011gKTCAEz3sJ1Hzde16xxU1
…s-rqe_configs' into 883-featquery-engine-rqe-tree-query-route
…isabled handling - A leaf whose lookback reads more closed windows than its `rqe_configs` entry retains of the value aggregation is a 400. Windows read are `(lookback - window) / slide + 1`, as the planner sizes retention. - A top-k leaf whose `k` exceeds its CountMinSketchWithHeap's `heapsize` is a 400 (`cms_heap_params` is now shared with the accumulator factory). - `POST /api/v1/rqe_query` answers 500 `internal` when request handling is disabled: it has no fallback, like the instant route without one. - The tree reads one streaming-config snapshot for all its leaves. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011gKTCAEz3sJ1Hzde16xxU1
…qe-expression-tree-type-and-validation
…on-tree-type-and-validation' into 882-featplanner-planner-saved-reads-rqe-trees-and-writes-rqe_configs # Conflicts: # asap-planner-rs/src/optimizer/milp_output.rs
…ads-rqe-trees-and-writes-rqe_configs' into 883-featquery-engine-rqe-tree-query-route # Conflicts: # asap-query-engine/src/engines/simple_engine/sql.rs # asap-query-engine/src/tests/mod.rs
…rqe-tree-query-route
Closes #883. Part of #886. Stacked on #888 (→ #887).
POST /api/v1/rqe_querywith{"expr": <RQE tree>, "time": <unix secs>}answers a tree without parsing PromQL/SQL, returning the Prometheus adapter's instant-vector JSON.rqe_configs[id]→AggregationIdInfo; end = PromQL-alignedtime, start = end −lookback_ms; then the shared context tail (build_sql_execution_context_tailmoved tomod.rsasbuild_execution_context_tail) andexecute_context_result, as the PromQL instant path does. A roll-up config runs its query-time stages throughapply_instant_pipeline.bylabels, or all) throughapply_instant_pipeline; avg is the unweighted mean.combine_vector_vector/combine_scalar.__name__, as Prometheus drops it after*_over_timeand aggregations.bad_data. An execution failure → 500. A window with no data → empty success. No fallback.handle_http_requestsor the query tracker. Both exist for PromQL text: forwarding to a fallback and passive query discovery.Small refactors:
get_aggregation_id_infotakes&[AggregationReference];combine_*andalign_end_timestamp_promqlarepub(super).Tests: 23 new.
sum_over_time,sum by (job)(sum_over_time),quantile_over_time(DDSketch, as KLL merges are randomised),topk, and the roll-up.Known: an agg over Inf/NaN (e.g.
x / 0) returns 500, because the query-time pipeline rejects non-finite values.🤖 Generated with Claude Code