fix(query-engine): Sliding range queries no longer sum overlapping windows - #621
Merged
milindsrivastava1997 merged 2 commits intoAug 26, 2026
Merged
Conversation
…ndows execute_range_query_pipeline's per-step composition (scan_window) walked every grid position in a step's lookback span and summed whatever it found there. That's correct for Tumbling, where each stored bucket is a genuinely disjoint slice -- but wrong for Sliding, where each stored bucket is already a complete, pre-merged window (worker.rs pre-merges before storing). Summing several of those together double/triple-counted overlapping data. Both value-side and keys-side per-step composition now branch on the aggregation's WindowType: Sliding takes a single lookup at the step's exact window position (the data was already fetched by the existing wide fetch, so no extra store round-trips); Tumbling is unchanged. Fixes #608. Co-Authored-By: Claude Sonnet 5 <[email protected]>
milindsrivastava1997
marked this pull request as ready for review
August 26, 2026 02:40
…ck invariant Code review on #621 flagged two follow-ups: - The Sliding-vs-Tumbling branch (single_window vs scan_window) was duplicated near-verbatim between the value-side and keys-side loops. Extracted into one window_buckets_for_step helper, used by both. - single_window's correctness for Sliding depends on lookback_ms == window_size_ms, previously documented but not checked anywhere. Threaded window_size_ms/keys_window_size_ms through RangeQueryExecutionContext and added active asserts (not debug_assert!) guarding the equality, matching scan_window's existing precedent of asserting its own precondition -- a broken invariant here means silently wrong data, the same failure mode #608 fixed. Co-Authored-By: Claude Sonnet 5 <[email protected]>
milindsrivastava1997
deleted the
worktree-608-sliding-range-exact-fetch
branch
August 26, 2026 02:44
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.
What was wrong
A PromQL range query over a Sliding-window aggregation (e.g.
sum_over_time(metric[5s])stepped out over time) could silently return the wrong numbers.Each stored Sliding-window bucket is already a complete, correct answer for its own window — the precompute worker pre-merges everything before storing it. The range query's per-step logic didn't know that: at each output timestamp, it walked back across every bucket in the lookback window and added them all together. That's the right thing to do for Tumbling windows (genuinely separate, non-overlapping chunks that are meant to be summed), but for Sliding windows it meant adding several overlapping windows on top of each other — double- or triple-counting the same underlying data.
Example: with a 3-second Sliding window, one step's correct answer was
111, but the buggy code returned12321(111 + 1110 + 11100— three overlapping windows summed instead of one).The fix
The per-step logic now checks whether the aggregation is Sliding or Tumbling:
This applies to both the main data being queried and (for metrics that track keys/labels separately) the keys side too, since keys can also be configured as Sliding.
No extra work needed to fetch this data — it was already being pulled from the store in one batch beforehand; the fix is entirely about how that already-fetched data gets combined per step.
Testing
Fixes #608.