feat(query-engine): unify sliding-window execution onto the range-query merge walk - #568
Open
milindsrivastava1997 wants to merge 3 commits into
Open
Conversation
…helper execute_range_query_pipeline's per-step "walk by window_size_ms, exact- timestamp lookup, merge found buckets, tolerate misses" logic is pulled into merge_window_at_timestamp, called once per step in place of the duplicated inline code. Pure extraction, verified as a no-op via the existing end-to-end range-query arithmetic tests (handle_range_query_promql -> execute_range_query_pipeline) plus 4 new direct unit tests, including one asserting the walk ignores denser intermediate buckets rather than merging everything in range. This is checkpoint 1 of PR B (design doc's #557 stack): the helper is not wired to anything new yet, so Tumbling behavior is unchanged and Sliding is still unreachable in production. Next: wire a Sliding branch into create_store_query_plan/execute_and_merge_store_queries so instant queries reuse this same helper (single-step case), plus the alignment fix. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…e walk create_store_query_plan and execute_and_merge_store_queries now branch on WindowType instead of the (now-always-false) is_exact_query flag. Sliding does a plain range fetch over [end - range_ms, end) -- same primitive Tumbling already uses -- with the end timestamp floor-aligned to the aggregation's slide_interval_ms grid first (align_to_slide_interval), then merges via merge_window_at_timestamp (checkpoint 1's extracted helper), called once instead of per-step. query_precomputed_output_exact / is_exact_query: true are now unreachable (tracked in #559 for removal). Tumbling's existing merge_precomputed_outputs path is untouched. 9 new end-to-end tests (tests/sliding_window_execution_tests.rs) drive a hand-wired Sliding AggregationConfig through the real handle_query_promql path (query_configs bypasses capability matching, which still has the strict pre-#557-PR-D Sliding rule) -- k=1,2,3,6 merge correctness (each asserting the stride-selected sum, not the sum of every S-spaced bucket in range), the alignment fix, and 4 negative cases (empty store, no data near the window, partial data tolerance, and all-strided-positions-missing resolving to zero series rather than None). Checkpoint 2 of PR B (design doc's #557 stack). Instant queries only -- the range-query side (aligning start once in promql.rs's range-context builders) is still to come. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…shifting reported timestamps promql.rs's two range-context builders now floor-align a Sliding query's lookup anchor (RangeQueryExecutionContext::aligned_start_ms) instead of the client-requested start_ms, once per query rather than per step -- PR A's step % slide_interval_ms validation is what guarantees the offset between the two stays constant across every step. execute_range_query_pipeline computes that fixed offset once and subtracts it only when calling merge_window_at_timestamp; the timestamp reported back via element.add_sample stays exactly what the client asked for. Misalignment still logs via align_to_slide_interval's existing warn! (no new logging needed -- it already fires whenever a real shift happens). An earlier version of this change shadowed start_ms directly, which also shifted every reported sample timestamp to the aligned grid -- caught before landing and reworked to decouple "what we tell the client" from "what we used internally to look up data". 2 new end-to-end range-query tests: strided-merge correctness across 4 output steps, and a misaligned-start case proving the response's timestamps match the request even though the underlying lookups are grid-aligned a few seconds earlier. Completes PR B (design doc's #557 stack) -- both the instant-query (previous commit) and range-query fetch paths now serve Sliding correctly. Still to push/open as an actual PR (base=557-sliding-window-query-engine). Co-Authored-By: Claude Sonnet 5 <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
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.
Part of #557. PR B of a 4-PR stack (design doc:
.design_docs/sliding-window-query-engine-design.md). Stacked on #562 (PR A) — base branch is557-sliding-window-query-engine, notmain.What
Sliding-window instant and range queries are now served correctly across multiple stored buckets, reusing the exact mechanism that already made Tumbling range queries correct instead of adding a parallel one.
The core idea (§2 of the design doc):
execute_range_query_pipeline's per-step logic — walk positionswindow_size_msapart, exact-timestamp lookup, merge whatever's found, tolerate misses — already does the right thing for Sliding for free: since a valid sliding config always hasslide_interval_ms | window_size_ms, walking byWagainst the store's denserS-spaced grid only ever touches the non-overlappingW-strided subset. No double-counting, no new overlap-accounting logic needed. This PR extracts that logic intomerge_window_at_timestampand reuses it from a new Sliding branch inexecute_and_merge_store_queries(instant queries, called once) as well as the existing range-query loop (called per step) — the old sliding-specific "skip merge, expect exactly 1 precompute" branch is deleted, not extended.Alignment (§4): worker buckets sit on the
slide_interval_msgrid, epoch-anchored; store lookups are exact-timestamp keyed, so misalignment doesn't error, it silently looks like missing data. Instant queries floor-align once per call. Range queries floor-align the lookup anchor once per query (not per step — PR A'sstep % slide_interval_ms == 0check is what guarantees that stays correct across every step) while keeping the reported sample timestamps exactly what the client requested — decoupled viaRangeQueryExecutionContext::aligned_start_msand a fixed per-query offset, so the response's time axis never silently shifts even though the data underneath can be a few seconds stale. Misalignment still logs (reusesalign_to_slide_interval's existingwarn!, no new logging needed).Also:
query_precomputed_output_exact/is_exact_query: trueare now unreachable (tracked in #559 for later removal — out of scope here, touches all 4Storebackend impls).Why this is safe to merge on its own
Same as PR A:
should_use_sliding_window()is still hardcodedfalse, so no live query can reach any of this. Tumbling's existing paths (merge_precomputed_outputs, the range-query loop's control flow) are untouched — only extracted-and-reused, never rewritten.Testing
merge_window_at_timestamp, including the actual regression catcher: 15 buckets on a dense 60,000ms grid, asserts the walk sums only the 3 that are 300,000ms apart, not all 15.tests/sliding_window_execution_tests.rs) driving a hand-wiredSlidingconfig through the realhandle_query_promqlpath (query_configsbypasses capability matching, which still has the strict pre-PR-D Sliding rule):k=1,2,3,6merge correctness, the alignment fix, and 4 negative cases (empty store, no data near the window, partial-data tolerance, all-strided-positions-missing resolving to zero series rather thanNone).cargo test,cargo clippy -D warnings,cargo fmt --checkall pass throughout (verified after every commit, not just at the end).Stack
PR A (#562) → this PR → PR C (
cleanup.rs, needs this PR's fetch shape finalized) → PR D (capability_matching.rsrelaxation — the actual activation PR).