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 <[email protected]>
…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 <[email protected]>
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 <[email protected]>
…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 <[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
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