feat(sketch-db): Phase 3b — cross-schema result combiner + engine registry wiring - #22
Merged
Merged
Conversation
…istry wiring
Lands the §7 combiner primitive used by the query path to stitch
per-segment scalars across a schema change, plus the minimal
SimpleEngine plumbing needed to reach a live SchemaRegistry. Does
**not** yet re-route the existing dispatch through the combiner —
that is Phase 3b-2, a pure-plumbing follow-up PR.
## New: `src/engines/timeline_dispatch.rs`
* `CombinedResult { Full(f64) | Partial { covered, missing } }` —
the result type that surfaces the schema-change failure mode
explicitly instead of silently returning "the wrong answer" when
a statistic can't span a schema boundary.
* `SegmentValue { segment, value }` — per-segment scalar input.
* `combine_statistic(stat, &segments, &unresolved) -> CombinedResult`
implementing §7.3 combinability:
- Count / Sum: additive.
- Min / Max: pointwise.
- Cardinality / Increase / Rate / Quantile / Topk: non-combinable
at the scalar level — caller must merge underlying sketches or
fall back to the exact DB.
* 12 unit tests covering each statistic, empty-input, single-segment,
and unresolved-with-covered scenarios.
## SimpleEngine wiring
* Adds `schema_registry: Arc<SchemaRegistry>` field, defaulted to
`SchemaRegistry::empty()` so the ~35 existing `SimpleEngine::new*`
call-sites keep compiling unchanged.
* `with_schema_registry(Arc<SchemaRegistry>) -> Self` builder
alongside `with_controller_client`.
* `timeline_for_query(metric, t1_ms, t2_ms) -> Vec<TimelineSegment>`
thin delegate so the engine's own query path (and Phase 3b-2 tests)
don't need to reach into the store module to build a timeline.
## main.rs plumbing
Defers the `Arc::new(engine)` wrap until after the precompute engine
is constructed, so the engine can share `precompute_ingest_state.schemas`
— the same `Arc<SchemaRegistry>` the ingest path is already
reconciling and the HTTP swap handler is driving event-driven (Phase
2b). When precompute isn't enabled the engine keeps its default
empty registry and `timeline_for_query` returns no segments,
matching pre-Phase-3 behaviour.
559 lib tests pass (up from 547: +12 combiner tests); clippy/fmt
clean. No changes to existing engine tests or call-sites.
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>
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
Lands the §7 combiner primitive plus the minimal plumbing the query path needs to reach a live
SchemaRegistry. Does not yet re-route the existing dispatch through the combiner — that is Phase 3b-2, a pure-plumbing follow-up PR.src/engines/timeline_dispatch.rswithcombine_statistic()+CombinedResult { Full | Partial }+SegmentValue. 12 unit tests.schema_registry: Arc<SchemaRegistry>(default empty),with_schema_registry()builder,timeline_for_query()thin delegate. All ~35 existing call-sites ofSimpleEngine::new*keep compiling unchanged.Arc::new(engine)until after the precompute engine is built, then plumbsprecompute_ingest_state.schemas.clone()into the engine so both components share the sameArc<SchemaRegistry>the HTTP swap handler (Phase 2b) is driving.Why split 3b into two PRs
The engine's query dispatch (
build_query_execution_context_promql+ the range pipeline) is ~5k LoC with many interacting tests. Wiring the combiner into it in the same PR as landing the combiner itself makes the PR review surface too large. Splitting keeps each PR local and reviewable.§7.3 combinability table
Count,SumMin,MaxCardinalityIncrease,RateQuantile,TopkNon-combinable results come back as
Partial { covered, missing }so the user (and Phase 3b-2's caller) can see exactly which segments failed.Test plan
cargo clippy --workspace --all-targets --tests -- -D warningsclean.cargo fmt -- --checkclean.🤖 Generated with Claude Code