PR #629 code review findings

[milindsrivastava1997]

Findings from a code review of PR #629 (computed top-k for range PromQL queries).

1. Already tracked in topk(...) as a binary-expr arm: __name__ label mismatch (pre-existing) and metric-name-in-join corruption (#629 Finding 1) #631; out of scope here. Range binary joins involving topk() (for example, topk(k,a)+b) have two pre-existing metric-name/label-matching failures. topk(...) as a binary-expr arm: __name__ label mismatch (pre-existing) and metric-name-in-join corruption (#629 Finding 1) #631 contains the reproductions and is the correct issue for the functional fix.
2. mod.rs:1904 — A missing/unparseable k kwarg silently skips ranking/truncation but still runs formatting, returning the full unranked result set as if it were valid topk output. Under normal PromQL context construction this should be unreachable because k extraction is fallible upstream, but it remains an internal invariant/defensive-programming gap.
3. mod.rs:1891 — (enable_topk_limiting=false, enable_topk_formatting=true) does not run range ranking/truncation. This is not currently observable: range results have no meaningful outer ordering, and all current call sites pass both flags together. The flags' contract should be clarified or simplified to prevent a future caller from assuming instant-query-like semantics.
4. stage_e_instant_range_equivalence_tests.rs:623 — Stale comment: it claims the topk sort has “no tiebreak,” but mod.rs:1939-1942 already implements the deterministic label-value tiebreak described by the test. Update the comment.
5. mod.rs:1890 — apply_range_topk's bool params are ordered (formatting, limiting), opposite the (limiting, formatting) convention used by sibling functions in the same file.
6. mod.rs:1904 — The k-kwarg parsing expression is duplicated verbatim between apply_range_topk and execute_query_pipeline instead of a shared helper.
7. mod.rs:1972 — The metric-name-prepend rewrite for topk labels is duplicated in intent between apply_range_topk and format_final_results; a shared helper would prevent the two output paths drifting.
8. mod.rs:1939 — Candidate list is fully sorted (O(G log G)) per timestamp before truncating to k; select_nth_unstable_by would suffice in expected O(G).
9. mod.rs:1953 — results.retain and the following values_mut() loop are two separate full passes over the map that could be fused into one.
10. Already covered by existing tests. topk_range_step_major_ranking_differs_per_step uses the self-keyed fixture with three candidates and topk(2), so it exercises self-keyed expansion together with real truncation. The separately named fix(query-engine): range queries expand keys_query per output step #595 regression uses topk(5) and does not truncate, but this is not a coverage gap.

🤖 Generated with Claude Code