feat(query-engine): computed top-k in range queries + stage-E equivalence tests (#581 stage E prep) - #629
Merged
Conversation
…ence tests (#581 stage E prep) Adds step-major topk ranking/truncation to execute_range_query_pipeline (previously range had no top-k support at all), wires it through PromQL's range call sites, and adds an instant/range equivalence test matrix across Tumbling/Sliding window shapes and SetAgg/DeltaSetAgg keys configs, ahead of stage E's full pipeline collapse. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…23) kept_timestamps_by_key's entries are only ever inserted alongside a timestamp drawn from that same element's own samples, so the per-sample filter can never leave a surviving key's samples empty -- the trailing retain was unreachable dead code. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
milindsrivastava1997
left a comment
milindsrivastava1997
left a comment
Contributor
Author
There was a problem hiding this comment.
Automated review findings.
…Finding 1 topk(...) as one arm of a binary expression is broken two different ways, neither of which is what Finding 1's review comment described nor fixable as part of #629 -- both are pre-existing/orthogonal and tracked in #631 instead. One test pins the current (surprising) None-return behavior; the other reproduces the join-corruption bug Finding 1 actually describes, and is #[ignore]d since it isn't fixed here. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ones Addresses PR #629 review findings 2-4 (mod.rs::apply_range_topk): - Finding 2: candidates.sort_by was value-only with no tiebreak, so groups tied at the k-th value boundary kept a different survivor run to run (HashMap iteration order is randomized per-process). Confirmed via a flaky RED test (4/5 pass rate) before adding a label-values tiebreak; stable across 20+ runs after. - Finding 3: folded into the same restructure -- index each group once instead of cloning its label vector per (group, timestamp) sample (G clones instead of G*T). - Finding 4: documented why range has no observable (false, true) case for enable_topk_limiting/enable_topk_formatting, unlike instant's always-sort-when-Topk behavior. Finding 1 is not addressed here -- tracked separately in #631. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
milindsrivastava1997
marked this pull request as ready for review
August 26, 2026 13:14
This was referenced Aug 26, 2026
milindsrivastava1997
added a commit
that referenced
this pull request
Aug 26, 2026
… step-major loop (#581 stage E.1-E.3) (#635) * refactor(query-engine): thread output_timestamps through range pipeline (#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 <noreply@anthropic.com> * refactor(query-engine): restructure range pipeline's per-step loop to 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 <noreply@anthropic.com> * docs(query-engine): fix stale apply_range_topk comment, note step-major 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 <noreply@anthropic.com> * refactor(query-engine): fold apply_range_topk's ranking into the step-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 <noreply@anthropic.com> * refactor(query-engine): collapse execute_query_pipeline into a thin wrapper 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 <noreply@anthropic.com> * fix(query-engine): address stage-E.4 review findings DeltaSetAgg fast-path assert, shared topk comparator, alias reuse, comment fixes. 536/536 passing. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- 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
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
execute_range_query_pipelinehad no limiting/formatting params). Adds step-major top-k ranking/truncation (apply_range_topk) — ranks and truncates independently at each output timestamp, since a range query's surviving key set can differ step to step, unlike instant's single-value-per-group case. Wired through all three PromQL range call sites.lookback_msmust equalwindow_size_ms(activeassert!inexecute_range_query_pipeline, tracked separately as Sliding window execution: support multi-window merge/subtract in query engine #554), so "Sliding at ratio 2/3" isn't constructible; andSetAggregator/DeltaSetAggregatorare keys-side types whose window_type is independent of the paired value aggregation's, so no cells needed excluding.mod.rs/promql.rs).Note: "computed top-k" over an arbitrary non-self-keyed expression (e.g.
topk(5, rate(foo[5m]))) turns out to be unsupported everywhere in this codebase today, not just in range (SumAccumulator::queryhard-errors onStatistic::Topk). Out of scope here, left as-is.Test plan
cargo test -p query_engine_rust --lib: 520/520 passingcargo clippy -p query_engine_rust --lib --tests -- -D warnings: cleanretain), 1 Low finding (tie-break nondeterminism) left as-is per reviewer's own note — pre-existing instant-side behavior, not introduced here🤖 Generated with Claude Code