fix(query): saturating_sub for time=0 + clean up dangling resolve_sketch_metric_alias callers (B6) - #277
Merged
Conversation
…tch_metric_alias callers (B6) Two fixes in `data_plane/src/query_engines/asap_query_engine/engine.rs`: 1. **`calculate_start_timestamp_promql` underflow.** The `OnlySpatial` branch did `end_timestamp - (scrape_interval * 1000)` with `end_timestamp: u64`. When the PromQL request has `time=0` (capability-miss feedback-loop probe shape, see `tests::capability_miss_http_e2e_tests`), `end_timestamp` is 0 and the bare subtraction underflows with `attempt to subtract with overflow`. Switch to `saturating_sub` — clamps to 0, which the downstream store query treats as a [0, 0]-width range. Empty result is the right answer for a probe looking for the capability-miss signal, not data. Currently dormant under the current call ordering (`process_via_simple_engine` calls legacy first, which short-circuits at the empty-streaming-config check before reaching line 611), but surfaces the moment the dispatch is reordered to modern-first. Fix removes the latent blocker for step 4 of #272. 2. **Delete dangling `resolve_sketch_metric_alias` callers at engine.rs:3024 and engine.rs:3525.** PR #275 retired the `resolve_sketch_metric_alias` method itself (the runtime refactor preserves metric names through the sketch processors — the suffix rewrite hadn't done anything in production for months). The PR removed the method definition + the legacy handle_query_promql call site but missed these two call sites inside `execute_range_promql_modern` and `QueryEngine::execute()` (both added by PR #274, which #275 was supposed to revert). Result: main fails to compile with two E0599s. This PR removes them inline as a fix-forward. Both call sites had the same shape: let query_owned = self .resolve_sketch_metric_alias(query) .unwrap_or_else(|| query.to_string()); let query = query_owned.as_str(); Removing the snippet leaves the original `query: &str` parameter in scope, which is what every downstream user wants — the analyzer accepts a `&str` directly. No semantic change beyond "don't try to rewrite a metric name that doesn't get rewritten by anyone." Test plan: * `cargo test -p data_plane --lib`: 749/749 pass (was 748; +1 for the new `calculate_start_timestamp_promql_handles_time_zero_without_underflow` regression test that pins the saturating_sub semantics). * Existing `capability_miss_http_e2e` tests still pass — the legacy path that previously triggered the underflow is still reached the same way (modern is still called second); the fix is dormant until the reorder lands but is now safe. Unblocks the next step of #272 — `process_via_simple_engine` modern-first reorder. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
zzylol
added a commit
that referenced
this pull request
May 18, 2026
…g crate (B7.5, schema-retirement #5 step 4) (#278) After #273 (modern execute() handles sketches via instances_matching) and #277 (saturating_sub for time=0 + dangling-callers cleanup), the legacy `handle_query` / `handle_query_promql` family no longer has features the modern `QueryEngine::execute()` trait can't handle. This PR deletes the whole legacy tree and the orphan `crates/promql_utilities/src/ast_matching/` module that only the legacy path consumed. Deleted: * `ASAPQueryEngine::handle_query`, `ASAPQueryEngine::handle_query_promql`, `ASAPQueryEngine::try_handle_query_promql_via_timeline` (instant-query entry points) * `ASAPQueryEngine::handle_range_query_promql`, `build_range_query_execution_context_promql`, `execute_range_query_pipeline`, `handle_binary_expr_range_promql`, `build_arm_range_context`, `apply_range_binary_op` (range-query entry point + helpers) * `execute_context`, `execute_query_pipeline`, `execute_store_query`, `execute_and_merge_store_queries` (legacy dispatcher chain) * `build_query_execution_context_promql*`, `build_promql_execution_context_tail`, `parse_and_match_promql`, `resolve_agg_info_promql`, `agg_info_from_forced_id`, `find_compatible_aggregation_with_miss_notify`, `resolve_metric_labels`, `calculate_query_timestamps_promql`, `calculate_start_timestamp_promql`, `validate_and_align_end_timestamp`, `extract_quantile_param_promql`, `extract_topk_param`, `build_query_kwargs_promql`, `create_keys_query_params`, `create_store_query_plan`, `collect_all_results`, `merge_precomputed_outputs`, `merge_accumulators`, `collect_results_separate_keys`, `collect_results_same_aggregation`, `limit_keys_for_topk`, `validate_range_query_params`, `format_final_results`, `build_query_requirements_promql`, `query_precompute_for_statistic` (legacy helpers) * `QueryExecutionContext`, `QueryMetadata`, `QueryTimestamps`, `StoreQueryParams`, `StoreQueryPlan`, `RangeQueryParams`, `RangeQueryExecutionContext` (legacy types) * `control_plane_patterns` field, the `PromQLPatternBuilder` setup in `new_with_hot_reload`, `QueryPatternType` enum * `crates/promql_utilities/src/ast_matching/` (4 files; only consumer was the legacy path) * `crates/promql_utilities/src/query_logics/parsing.rs` helpers (`get_metric_and_spatial_filter`, `get_statistics_to_compute`, `get_spatial_aggregation_output_labels`) * Test modules tied to the deleted surface: `range_query_tests`, `sketch_query_tests`, `e2e_feedback_loop_tests`, `forced_agg_id_tests`, `hll_count_query_tests`, `kll_quantile_query_tests`, `cms_rate_capability_tests`, `analyzer_parity_tests`, `calculate_start_timestamp_promql_tests`, `aux_pushdown_tests`, the whole `capability_matching_tests.rs` file and `tests/test_utilities/comparison.rs` Updated: * `data_plane/src/drivers/query/servers/http.rs::process_via_simple_engine` now calls modern `execute()` only. The capability-miss notify side-effect that used to live in `find_compatible_aggregation_with_miss_notify` is moved to the modern path's sid-resolution error branches AND to the "no-sketch-index attached" branch (HttpServer attaches its own `SketchStore` but the `ASAPQueryEngine` builder it hands off does not `.with_sketch_index(...)` — the e2e test `http_capability_miss_feedback_loop_closes_over_http` pins exactly that wiring). * `handle_range_query` now calls modern `execute_range_promql_modern` only — no legacy fallback. * `handle_precompute_job` routes through modern `execute()`. * `tests/schema_timeline_dispatch_tests.rs::single_schema_query_falls_through_to_default_path` moved to `#[ignore]` — its premise no longer has a callsite. Modern-path coverage lives in `asap_tier_classify_tests` and `e2e_modified_otlp_sketch_path`. Test plan: * `cargo test -p data_plane --lib`: 707 pass, 0 failed, 5 ignored (down from 754 — 47 tests deleted with the legacy code they exercised). * `cargo test -p data_plane --lib capability_miss_http_e2e`: 1 pass, 1 ignored — the feedback-loop test that pins the notify side-effect still passes. * `cargo test -p control_plane --lib`: 699 pass, 0 failed. The `try_handle_query_promql_via_timeline` cross-reconfigure dispatch path is gone too. Its functionality (per-segment dispatch across schema boundaries) was scheduled for a sid-level rewrite in the schema-retirement #5 follow-up; deferred to a separate PR since no current test exercises a multi-segment reconfigure boundary (the two `#[ignore]`d tests in `tests/schema_timeline_dispatch_tests.rs` documented as needing a sid-level rewrite anyway). Closes step 4 of #272. 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
Two fixes bundled because the first one couldn't be tested without the second.
Fix 1 —
saturating_subfor time=0calculate_start_timestamp_promqlin theOnlySpatialbranch did bare u64 subtraction:```rust
end_timestamp - (self.prometheus_scrape_interval * 1000)
```
When the PromQL request has `time=0` (capability-miss feedback-loop probe shape, see `tests::capability_miss_http_e2e_tests`), `end_timestamp` is 0 and the bare subtraction underflows u64 with `attempt to subtract with overflow`. Switching to `saturating_sub` clamps to 0 — exactly the right semantic for a probe looking for the miss signal, not data.
Currently dormant under the current call ordering (legacy runs first and short-circuits at the empty-streaming-config check before reaching this line), but surfaces the moment `process_via_simple_engine` is reordered to call modern first. Removes the latent blocker for step 4 of #272.
Fix 2 — dangling `resolve_sketch_metric_alias` callers
PR #275 retired the `resolve_sketch_metric_alias` method but missed two call sites at `engine.rs:3024` and `engine.rs:3525` (both added by PR #274, which #275 was supposed to revert). Main currently fails to compile with two E0599s. This PR removes them inline. The removed snippets had the shape:
```rust
let query_owned = self.resolve_sketch_metric_alias(query).unwrap_or_else(|| query.to_string());
let query = query_owned.as_str();
```
Removing leaves the original `query: &str` parameter in scope, which is what every downstream caller wants. No semantic change.
Test plan
Why this matters
Unblocks step 4 of #272 (schema-retirement #5) — reorder `process_via_simple_engine` to call modern `execute()` first, so the legacy `handle_query` family can be retired.
🤖 Generated with Claude Code