refactor(sketch-db): Phase 3b-2-a — extract forced-agg-id context builder - #37
Merged
Merged
Conversation
…lder
Pure refactor. Splits `SimpleEngine::build_query_execution_context_promql`
into composable pieces so Phase 3b-2-b's timeline dispatch can build
per-segment execution contexts against specific `agg_id`s without
re-running the auto-resolver. Zero behavioural change to existing
code paths.
## What's extracted
* `parse_and_match_promql(query) -> Option<(QueryPatternType, PromQLMatchResult)>`
— shared phase-1 (PromQL parse + pattern match). Previously
inlined inside `build_query_execution_context_promql`.
* `resolve_agg_info_promql(query, match_result, pattern_type) ->
Option<AggregationIdInfo>` — phase-2 auto-resolver (try
`QueryConfig` exact match, fall back to capability matching
with miss-notify). Previously inlined inside
`build_query_execution_context_promql`.
* `agg_info_from_forced_id(agg_id) -> Option<AggregationIdInfo>`
— the new building block that powers per-segment dispatch.
Looks up the agg type in the current `StreamingConfig` and
returns the single-aggregation `AggregationIdInfo` shape
(key and value share the same id), matching the fallback
branch in `get_aggregation_id_info`. Returns `None` if the
agg_id has been removed from the config.
## What's new
* `build_query_execution_context_promql_for_agg_id(query, time,
forced_agg_id) -> Option<QueryExecutionContext>` — parallel
entry point that does phase-1 + `agg_info_from_forced_id` +
phase-3 (`build_promql_execution_context_tail`), skipping the
auto-resolver entirely. This is the hook Phase 3b-2-b calls
per `TimelineSegment`.
## What's NOT changed
* `build_query_execution_context_promql` — now delegates to
the extracted helpers but body-by-body identical in effect.
Caller-observable output unchanged.
* `build_promql_execution_context_tail` — untouched (already
agg-info-agnostic).
* `execute_context` / `execute_range_query_pipeline` — untouched.
* All existing query paths (`handle_query_promql`,
`handle_range_query_promql`, etc.) — continue through the
auto-resolver entry point with zero change.
## Test plan
- [x] 4 new unit tests in `forced_agg_id_tests`:
* Auto-resolve still works after refactor (regression gate).
* Forced entry point produces a context for a known agg_id.
* Unknown agg_id returns `None` without panic.
* Forced and auto-resolved produce observably identical
`AggregationIdInfo` for the common single-agg case.
- [x] 696 lib tests pass (up from 692); existing
`dispatch_arithmetic_tests::test_handle_query_promql_single_metric_still_works`
and `plan_execution_temporal_tests::test_temporal_sum_over_time_*`
all still green.
- [x] `cargo clippy --workspace --all-targets --tests -- -D warnings` clean.
- [x] `cargo fmt -- --check` clean.
## Next
Phase 3b-2-b wires the per-segment dispatch:
1. At query entry, call `SchemaRegistry::timeline_for_metric(metric, start, end)`.
2. If exactly 1 segment, delegate to the existing auto-resolver
(zero behaviour change for single-schema queries).
3. If > 1 segment: for each segment, call
`build_query_execution_context_promql_for_agg_id` with the
segment's `agg_id` and clipped range, execute, collect the
scalar, and combine via `combine_statistic`.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
3 tasks
zzylol
added a commit
that referenced
this pull request
Apr 20, 2026
…able stats (#48) Task #34 gap #1 of 3. The §7 schema-timeline primitive (`SchemaRegistry::timeline_for_metric`) and the cross-schema combiner (`engines::timeline_dispatch::combine_statistic`) landed in PRs #20 / #22 / #25, but `SimpleEngine::handle_query_promql` was still resolving a single `agg_id` via `resolve_agg_info_promql` and running the full query against it. A query whose time range spans a reconfigure boundary (old `agg_id` retired, new `agg_id` created) saw a data cliff for the pre-boundary slice. This PR wires the dispatcher: * New `SimpleEngine::try_handle_query_promql_via_timeline`: 1. Parse + pattern-match the query, extract metric name. 2. Build a probe `QueryExecutionContext` to read the resolved `[t1, t2]` + `Statistic`. 3. Call `timeline_for_query(metric, t1, t2)`. Bail out with `None` (fall-through to default single-agg path) if fewer than two segments, or if the statistic is non-combinable (quantile / topk / cardinality / rate / increase — those follow in PR B2 with a Partial HTTP response surface). 4. Per segment: reuse `build_query_execution_context_promql_for_agg_id` from PR #37 (the extracted forced-agg-id entry point), clip the store plan's `[start, end]` to the segment's bounds, execute, collect results. 5. Group by label-tuple and fold per-group per-segment scalars through `combine_statistic`. Emit the combined scalar as an `InstantVectorElement`. Purged segments or segments whose `agg_id` is no longer in the config go into `unresolved` so the combiner sees them. * `handle_query_promql` now tries the timeline path first; returns immediately on `Some`, falls through to the existing single-agg path on `None`. Zero behavior change when the timeline has 0–1 segments for the query's metric (the common case today). ## Scope Combinable stats only: Count / Sum / Min / Max. Non-combinable stats still take the single-agg path — PR B2 will surface `CombinedResult::Partial` on the HTTP response so users see `{covered, missing: [segments]}` explicitly instead of a silent data cliff. ## Validation - `cargo test -p query_engine_rust --lib` — 728 pass (baseline unchanged; the dispatcher stays dormant when tests only register one schema per metric). - `cargo clippy --all-targets -- -D warnings` — clean - `cargo fmt --all -- --check` — clean ## Follow-ups (explicit non-scope here) - **Integration test** seeding two agg_ids + cross-boundary Sum query. Requires the full `PrecomputeEngine` setup harness the existing e2e tests use; deferred as a dedicated PR so this one stays a focused dispatcher patch. - **PR B2**: Partial response surface for non-combinable stats on the HTTP adapter. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
3 tasks
zzylol
added a commit
that referenced
this pull request
May 2, 2026
* docs(todo): sync after 2026-05-01 query-path-closes-the-loop work (#72) Capture this session's merged backend work — #70 (tonic OTLP gRPC max_decoding_message_size bumped to 64 MiB) and #71 (store range_query_into overlap filter + engine closest-pane selection + Prometheus-adapter precompute_window annotation) — at the top of TODO.md so the runtime-warm-tier-actually-works claim is testable from the doc. Cited the matching collector-side PRs (ASAPCollector#210 + #211) in the companion-changes note so future readers can see both halves of the wire fix. Added one new entry under "Known reconciliation gap": `IngestState.sketch_snapshots` is RAM-only, so backend restarts break delta ingest until the agent restarts too. Same item is mirrored in the collector's PROGRESS.md follow-up list — fix on either side closes the gap. `_Last updated_` set to 2026-05-01. Docs only; no code changes. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor: retire sketch-core mirror (#73) * refactor: retire sketch-core mirror * refactor: switch consumer imports to asap_sketchlib::sketches::* Update PR #73 against the reorganized asap_sketchlib (PR #36): the runtime sketches no longer live under a dedicated `asap::` module — they were merged into the existing `src/sketches/` layout (single home per sketch concept, ASAP-runtime types appended to the file that already holds the high-throughput in-process variant). Mechanical path swaps in asap-query-engine: - `asap_sketchlib::asap::dd_sketch::*` → `::sketches::ddsketch::*` - `asap_sketchlib::asap::count_min::*` → `::sketches::countmin::*` - `asap_sketchlib::asap::count_sketch::*` → `::sketches::count::*` - `asap_sketchlib::asap::hll_sketch::*` → `::sketches::hll::*` - `asap_sketchlib::asap::kll::*` → `::sketches::kll::*` - `asap_sketchlib::asap::count_min_with_heap::*` → `::sketches::cms_heap::*` - `asap_sketchlib::asap::hydra_kll::*` → `::sketches::hydra_kll::*` - `asap_sketchlib::asap::set_aggregator::*` → `::sketches::set_aggregator::*` - `asap_sketchlib::asap::delta_set_aggregator::*`→ `::sketches::delta_set_aggregator::*` - `asap_sketchlib::asap::config::*` → `::asap_runtime::*` Naming-conflict renames carried through to the consumers: - `HllDelta` → `HllSketchDelta` (octo_delta::HllDelta still wins the short name) - `HeapItem` → `CmsHeapItem` (common::input::HeapItem still wins the short name) main.rs aliases `asap_sketchlib::asap_runtime as config` so the existing clap derive references (`config::DEFAULT_CMS_IMPL`, `config::configure(...)`) still work without touching the rest of the bin. Tests: - `cargo build --workspace` → clean - `cargo test -p query_engine_rust --lib precompute_operators` → 141 passed, 0 failed Depends on ProjectASAP/asap_sketchlib#36 (force-pushed `e473ccc`). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor: align CountSketchDelta consumer with sketchlib-go wire format Track the additive `hh_keys` field on `asap_sketchlib::CountSketchDelta` so the proto delta path constructs the type with all fields filled in. Sends an empty `hh_keys` for now: the vendored Rust proto bindings in `asap_otel_proto::sketchlib::v1` haven't been regenerated against the latest `.proto` (which carries `hh_keys` on the Go side). The TopK rebuild on the proto-delta path will fire once those bindings sync; the sketchlib-go-aligned semantics are already in place underneath. Bumps the asap_sketchlib git dep to `refactor/wire-format-align-go` (see asap_sketchlib PR #37). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
zzylol
added a commit
that referenced
this pull request
May 5, 2026
* refactor: retire sketch-core mirror * refactor: switch consumer imports to asap_sketchlib::sketches::* Update PR #73 against the reorganized asap_sketchlib (PR #36): the runtime sketches no longer live under a dedicated `asap::` module — they were merged into the existing `src/sketches/` layout (single home per sketch concept, ASAP-runtime types appended to the file that already holds the high-throughput in-process variant). Mechanical path swaps in asap-query-engine: - `asap_sketchlib::asap::dd_sketch::*` → `::sketches::ddsketch::*` - `asap_sketchlib::asap::count_min::*` → `::sketches::countmin::*` - `asap_sketchlib::asap::count_sketch::*` → `::sketches::count::*` - `asap_sketchlib::asap::hll_sketch::*` → `::sketches::hll::*` - `asap_sketchlib::asap::kll::*` → `::sketches::kll::*` - `asap_sketchlib::asap::count_min_with_heap::*` → `::sketches::cms_heap::*` - `asap_sketchlib::asap::hydra_kll::*` → `::sketches::hydra_kll::*` - `asap_sketchlib::asap::set_aggregator::*` → `::sketches::set_aggregator::*` - `asap_sketchlib::asap::delta_set_aggregator::*`→ `::sketches::delta_set_aggregator::*` - `asap_sketchlib::asap::config::*` → `::asap_runtime::*` Naming-conflict renames carried through to the consumers: - `HllDelta` → `HllSketchDelta` (octo_delta::HllDelta still wins the short name) - `HeapItem` → `CmsHeapItem` (common::input::HeapItem still wins the short name) main.rs aliases `asap_sketchlib::asap_runtime as config` so the existing clap derive references (`config::DEFAULT_CMS_IMPL`, `config::configure(...)`) still work without touching the rest of the bin. Tests: - `cargo build --workspace` → clean - `cargo test -p query_engine_rust --lib precompute_operators` → 141 passed, 0 failed Depends on ProjectASAP/asap_sketchlib#36 (force-pushed `e473ccc`). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor: align CountSketchDelta consumer with sketchlib-go wire format Track the additive `hh_keys` field on `asap_sketchlib::CountSketchDelta` so the proto delta path constructs the type with all fields filled in. Sends an empty `hh_keys` for now: the vendored Rust proto bindings in `asap_otel_proto::sketchlib::v1` haven't been regenerated against the latest `.proto` (which carries `hh_keys` on the Go side). The TopK rebuild on the proto-delta path will fire once those bindings sync; the sketchlib-go-aligned semantics are already in place underneath. Bumps the asap_sketchlib git dep to `refactor/wire-format-align-go` (see asap_sketchlib PR #37). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore(asap_sketchlib): bump pin past PR #39 module renames Three asap_sketchlib modules were renamed in upstream PR #39: - sketches::countmin → sketches::countminsketch - sketches::count → sketches::countsketch - sketches::cms_heap → sketches::countminsketch_topk Backend consumers updated. Cargo.toml pin moved from refactor/wire-format-align-go branch to main (which now also has hh_keys restoration via PR #42 and DDSketch + KLL byte parity via #40 + #41). Unblocks ASAPCollector Phase 3 step 3 (backend consumes asap-precompute-rs). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- 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
Pure refactor. Splits
build_query_execution_context_promqlinto composable pieces so Phase 3b-2-b can build per-segment execution contexts against specificagg_ids. Zero behavioural change to existing code paths.parse_and_match_promql— shared phase-1 (parse + pattern match).resolve_agg_info_promql— existing phase-2 auto-resolver, now named + extracted.agg_info_from_forced_id(agg_id)— new: look up agg type inStreamingConfig, return single-aggregationAggregationIdInfo,Noneif id missing.build_query_execution_context_promql_for_agg_id(query, time, forced_agg_id)— skips auto-resolver, uses the forced id.What's NOT changed
build_query_execution_context_promqlnow delegates to helpers but is body-by-body identical in effect.build_promql_execution_context_tail,execute_context,execute_range_query_pipeline— untouched.Test plan
None, forced and auto agree for single-agg case.dispatch_arithmetic_tests+plan_execution_temporal_testsgreen.Next
Phase 3b-2-b wires per-segment dispatch:
combine_statistic.🤖 Generated with Claude Code