refactor(query-engine): unify range pipeline onto output_timestamps + step-major loop (#581 stage E.1-E.3) - #635
Merged
Conversation
…ne (#581 stage E.1) Replaces RangeQueryExecutionContext's start/end/step RangeQueryParams with an explicit output_timestamps: Vec<u64>, computed once upstream in finish_range_context instead of re-expanded via a manual while loop inside execute_range_query_pipeline. Zero behavior change -- same timestamp sequence, just threaded as a list instead of three fields re-walked with a mutable loop variable. Stage E prep: this is the shape a future unified instant/range engine takes directly (instant becomes the one-element case). 520/520 tests passing, no new tests needed (pure refactor, existing suite is the regression guard). Co-Authored-By: Claude Sonnet 5 <[email protected]>
… step-major (#581 stage E.2) execute_range_query_pipeline's fetch/merge loop was group-major (all timestamps for group A, then all timestamps for group B, ...). Restructures it to step-major (all groups for t1, then all groups for t2, ...) -- required for topk correctness (ranking a timestamp's candidates needs every group's value at that timestamp, which a group-major loop can't provide), and the one loop shape topk and non-topk queries now share. Per-group setup (bucket_map, keys_source) still happens exactly once per group, precomputed into a Vec before the step-major loop, not repeated per timestamp. No per-group state carries across timestamps in the existing loop body (every step re-derives its window from bucket_map fresh), so this is a pure reordering: same (group, timestamp, value) triples produced, same per-group sample ordering (chronological, since each group is still visited in ascending timestamp order) -- verified via the full existing suite, including the #629 instant/range equivalence matrix and topk step-major tests, which are exactly what would catch a reordering regression here. 520/520 tests passing, no new tests needed (behavior-preserving refactor). Co-Authored-By: Claude Sonnet 5 <[email protected]>
…or memory tradeoff (roborev #32) apply_range_topk's doc comment still said the fetch/merge loop was group-major and that step-major restructuring was "not done here, deliberately" -- both false as of the previous commit (stage E.2). Updates it to describe the loop as it is now, and to explain apply_range_topk still runs as a separate post-pass rather than being folded into that loop (that's stage E.3, not done yet). Also documents the Low finding: building every group's bucket_map eagerly into the `groups` Vec (instead of one group at a time, dropped between groups) raises peak memory for high-cardinality range queries. Inherent to enabling step-major ranking -- no code change, just made the tradeoff explicit where it wasn't before. 520/520 tests passing. Co-Authored-By: Claude Sonnet 5 <[email protected]>
…-major loop (#581 stage E.3) apply_range_topk ran as a separate pass after the step-major loop finished, re-deriving each timestamp's candidate set (by_timestamp) from the fully assembled results just to rank/truncate for topk. The step-major loop (stage E.2) already visits every group at every output timestamp, so it already has what that re-derivation was reconstructing. Folds the ranking/truncation directly into the loop: each timestamp's (key, value) pairs are collected into step_results, sorted + truncated to k right there when it's a topk query, then inserted into the final result map. apply_range_topk is deleted; formatting (metric-name label prefix) becomes a small tail pass over the final results, since it's a once-per-group operation, not once-per-timestep, and doesn't fit naturally inside the step-major loop the way ranking does. Also fixes tie-break nondeterminism while rewriting this logic: the sort comparator now breaks ties on label value, not just descending value. step_results' order traces back to a HashMap iteration (groups, built from all_data/keys_raw_data) and would otherwise keep a different group at the k-th boundary on every process run. (Same fix separately queued for PR for it there once #629 rebases past this.) 520/520 tests passing (28/28 topk-specific), clippy clean. Co-Authored-By: Claude Sonnet 5 <[email protected]>
…rapper around execute_range_query_pipeline (#581 stage E.4) The actual pipeline collapse: execute_query_pipeline now builds a single-timestamp RangeQueryExecutionContext (build_instant_range_context, mirroring finish_range_context but skipping its start<end range-query validation) and delegates to execute_range_query_pipeline, unwrapping the one resulting sample per group back into an InstantVectorElement. Public signature/return type unchanged, per #581's own D2 decision -- SQL/Elastic callers need no changes. widen_query_window moved from promql.rs to mod.rs (it was never PromQL-specific, just needed by both finish_range_context and this new builder, and mod.rs can't call a private child-module item). The pre-E.4 instant implementation is preserved verbatim under execute_query_pipeline_pre_e4 (#[cfg(test)] only) as the reference side of a new 13-case old-vs-new comparison suite (stage_e4_instant_wrapper_equivalence_tests), covering window type x population x statistic combinations plus edge cases (no data, keys-without- value #597, multi-group, binary-expr composition). This is deliberately different from stage_e_instant_range_equivalence_tests.rs, which compares instant vs range as independent implementations and stops being a meaningful check for this exact change once instant routes through range internally. Cleanup (deleting the pre_e4 path, its now-dead callees, and this comparison suite) is left for a follow-up commit; the dead functions are #[allow(dead_code)]'d in the meantime. Three real bugs found via this process, all fixed: 1. Topk sort-order regression: execute_range_query_pipeline's final `results.into_values().collect()` comes from a HashMap, which does not preserve the ranking step_results established. Instant's contract (unconditional sort-by-value-descending whenever the statistic is Topk, inherited from the old format_final_results) silently broke. Fixed by re-sorting in the wrapper, tie-broken by label for determinism. 2. DeltaSetAggregator performance regression, pre-existing for range queries too: sum_window (renamed from scan_window) walks every grid position in a step's nominal window, which is fine for Tumbling/Sliding but catastrophic for DeltaSetAggregator's [0, current_time) "replay from the beginning" keys window -- ~1e8 positions for a real timestamp. fetch_window_grid_via_exact_lookups (renamed from scan_windows_via_exact) already special-cased this aggregation type at the fetch layer; added the equivalent at the merge layer (collect_bucket_map_entries_before), bypassed via key_accumulator_type at the one call site that needs it. Confirmed via a new RED test (range_query_delta_set_keys_wide_range_from_zero_completes_quickly_and_correctly) that this was never instant-specific -- range queries already hit it, just untested. 3. Ordering bug in the fix for #2, caught by the full suite (not the targeted tests): DeltaSetAggregatorAccumulator::merge_with is order-sensitive (#586), and collect_bucket_map_entries_before's initial version iterated a HashMap in arbitrary order instead of chronologically. Fixed with an explicit sort by timestamp before merging. Both scan_windows_via_exact and scan_window renamed (fetch_window_grid_via_exact_lookups, sum_window) -- the near-identical names for two functions at completely different layers (store fetch vs. in-memory merge composition) directly caused the confusion that let bug #2 exist unnoticed. All stale comment references to the old names updated across mod.rs, promql.rs, and three test files. 536/536 tests passing, clippy clean. Co-Authored-By: Claude Sonnet 5 <[email protected]>
milindsrivastava1997
marked this pull request as ready for review
August 26, 2026 19:43
DeltaSetAgg fast-path assert, shared topk comparator, alias reuse, comment fixes. 536/536 passing. Co-Authored-By: Claude Sonnet 5 <[email protected]>
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
Stages E.1-E.3 of #581's pipeline collapse (E.4 -- making
execute_query_pipelinea thin wrapper around this -- not done here, WIP on top of this branch):RangeQueryExecutionContextthreads an explicitoutput_timestamps: Vec<u64>instead ofstart/end/stepfields re-expanded via a manualwhileloop insideexecute_range_query_pipeline. Zero behavior change -- same timestamp sequence, computed once upstream infinish_range_contextinstead.execute_range_query_pipeline's fetch/merge loop restructured from group-major (all timestamps for group A, then group B, ...) to step-major (all groups for t1, then t2, ...) -- required for topk correctness (ranking a timestamp's candidates needs every group's value at that timestamp, which group-major can't provide). Per-group setup (bucket_map, keys_source) still happens exactly once per group, precomputed before the loop. Pure reordering, no per-group state carries across timestamps in the original loop body, so this is behavior-preserving by construction -- verified via the full suite including the instant/range equivalence matrix and topk step-major tests.apply_range_topk(a separate post-pass that re-derived per-timestamp candidate grouping from the already-assembled results) is folded directly into the step-major loop -- each timestamp's candidates are ranked/truncated inline, right where the loop already visits every group at that timestamp. Formatting (metric-name label prefix) stays a small tail pass, since it's per-group not per-timestep.Two roborev findings addressed along the way (job #32: stale doc comment + documented a real peak-memory tradeoff inherent to step-major, filed as #633 rather than fixed inline since there's no cardinality signal yet that it matters; job #36: no code change needed, an already-present test from PR #629's rebase covers it).
Branched off
581-unify-instant-range-stage-c(PR #629) while it was still open, then rebased cleanly ontomainafter #629 squash-merged -- see commit history for the mechanics (skipped replaying the now-redundant pre-squash commits viagit rebase --onto).Test plan
cargo test -p query_engine_rust --lib: 522/522 passing (1 pre-existing#[ignore]d, unrelated to this branch -- topk(...) as a binary-expr arm: __name__ label mismatch (pre-existing) and metric-name-in-join corruption (#629 Finding 1) #631's pinned bug)cargo clippy -p query_engine_rust --lib --tests -- -D warnings: clean🤖 Generated with Claude Code