Repository navigation
fix(query-engine): judge a Sliding range step's cover by distinct windows - #847
Conversation
…dows Two workers can each emit a bucket for the same (key, window), so counting buckets let a duplicate stand in for a missing cover window and return a partial value. Count distinct window starts instead, on both the value and the key side. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| /// Number of distinct stored windows in a step's Sliding cover. A window | ||
| /// can hold several buckets (one per worker that saw the key), so the | ||
| /// bucket count can't tell a complete cover from a partial one. | ||
| fn sliding_cover_window_count( |
There was a problem hiding this comment.
Second walk over the same grid. This helper walks (window_start..window_end).step_by(W) and does a bucket_map.get at each step. sum_window (through window_buckets_for_step) then walks the same positions and does the same lookups. So each Sliding step does two grid walks per group, on both the key and value sides. Having sum_window return the number of non-empty positions it visited along with the buckets would answer the completeness question in one pass, and the cover logic would live in one place. Also: sum_window explicitly asserts step_increment > 0, but this helper's step_by(0) panics with only a generic message.
| keys_window_end, | ||
| *keys_stored_window_size_ms, | ||
| ); | ||
| if covered < expected { |
There was a problem hiding this comment.
Key-side change has no test. This changes the Sliding key cover check (keys_bucket_map, distinct windows instead of bucket count), but the new regression test only covers a value aggregation with KeysSource::Fixed. If someone reverts this branch to keys_window_buckets.len() < expected, every test still passes, and a dual-population query with a duplicated key bucket and a missing key window would again resolve keys from a partial cover. Please add a key-side twin of sliding_range_step_with_duplicate_bucket_and_missing_window_is_skipped.
| }); | ||
| for (start, value) in [ | ||
| (3_000, 10.0), | ||
| (3_000, 10.0), |
There was a problem hiding this comment.
Are these partials or duplicates? The test inserts two identical (3_000, 10.0) buckets for the same (key, window) and calls one of them a duplicate, but the engine merges every bucket at a start as a per-worker partial. If a step with a complete cover included [3s,6s) (e.g. a step at 9s with [6s,9s) present), it would sum to 20 for that window. That is right if these are two workers' partials and double-counting if this is a real duplicate (re-emission or retry). The PR fixes completeness only. Please state the invariant (one bucket per worker per window, never a re-emitted copy), or rename the test and use different partial values so it doesn't model a true duplicate.
| if window_type == WindowType::Sliding { | ||
| let expected = (lookback_ms / context.window_size_ms) as usize; | ||
| if window_buckets.len() < expected { | ||
| let covered = Self::sliding_cover_window_count( |
There was a problem hiding this comment.
Reuse the exact-cover planner. The instant path already judges Sliding completeness as set equality between plan_exact_cover's required windows and the found (start, end) ranges (found != required). This range-step check re-derives the requirement as lookback / W plus a distinct-start count. The two can drift apart: expected uses floor division while the walk takes ceil(lookback/W) positions. Computing each step's required windows with plan_exact_cover and checking membership would give range and instant one definition of a complete cover.
| window_end, | ||
| context.window_size_ms, | ||
| ); | ||
| if covered < expected { |
There was a problem hiding this comment.
Wasted work on skipped steps. window_buckets_for_step (line 3066) clones every bucket in the cover (clone_boxed_core, which deep-copies sketches) before this completeness check runs. The check no longer reads window_buckets, so a step whose cover is incomplete pays for the full clone and then throws it away. On a range where a series appears partway through, every early step does this for each group. The key side already runs its check before composing. Move this check above the window_buckets_for_step call to match.
1 participant
Part 1/5 of #839 (roll-ups across subpopulations). Stack: #1 (this) → engine roll-up → compliance suite → MILP sketches → MILP exact types.
What
A range query's per-step Sliding completeness check counted buckets against the number of cover windows. Two workers can each emit a bucket for the same
(key, window), so a duplicate could stand in for a missing window and return a partial value. The check now counts distinct window starts (sliding_cover_window_count) on both the value and the key side.This matters more once roll-ups land, because a roll-up appends every subpopulation's buckets into one
BucketMapslot. It's also a latent bug on its own.Test
sliding_range_step_with_duplicate_bucket_and_missing_window_is_skipped:[3s,6s)is stored twice and[0s,3s)is missing. Before the fix, the step at 6s returned the partial20.0; it is now skipped. The step at 7s still returns1100.🤖 Generated with Claude Code