refactor(query-engine): unify instant/range key resolution (#581 stage B) - #612
Merged
Merged
Conversation
…solver Adds resolve_and_query_group, one function both collect_results_separate_keys and collect_results_same_aggregation now call to resolve a value group's expansion keys and query a statistic for each. Consolidates three lenient skip-and-warn cases into one place instead of three separately-maintained copies: a group with keys data but no value data (#597), a keys accumulator that can't produce a resolvable key set, and a single key's stat query failing. The latter two used to hard-fail the entire instant query; they now skip just the affected group/key, matching range's existing behavior. TDD: two new RED tests pin the desired behavior (instant_query_dual_population_unresolvable_key_set_is_skipped_not_fatal, instant_query_dual_population_key_missing_from_value_accumulator_is_skipped_not_fatal) and now pass. Full suite (577 tests) green. Range's inline KeysSource logic still needs rewiring onto this same resolver -- follow-up commit. Part of #581.
…olver execute_range_query_pipeline's per-step loop now calls resolve_and_query_group (added in the previous commit for the instant pipeline) instead of its own separate per-key resolution and query loop. KeysSource::Fixed's payload changes from Vec<KeyByLabelValues> (always 0 or 1 elements) to Option<KeyByLabelValues>, matching resolve_and_query_group's fallback_key parameter directly. Behavior-preserving: same early-exit triggers for an empty/unmergeable keys window, same fallback-key semantics, same "drop the fully-unlabeled case" outcome (RangeVectorElement can't represent a None key). One deliberate, already-agreed change: a per-key query failure now logs at warn! via the shared resolver instead of range's previous debug!, matching instant's policy -- the row is still dropped either way, only the log level differs. Full suite (577 tests) green, including every existing range dual-population/ sliding-window/delta-set test -- no behavior regression. B (instant/range key-resolution unification) is now complete: both pipelines call the same resolve_and_query_group. Part of #581.
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.
What this does
Instant and range PromQL queries each had their own, separately-maintained logic for answering the same question: "given a fetched/merged value group, which output label combinations does it produce, and what's each one's value?" That duplication has caused three separate bugs as the two copies drifted apart over time (#570, #582, #587). This PR replaces both copies with one shared function,
resolve_and_query_group, that both pipelines now call.Before / after
Before: instant (
collect_results_separate_keys/collect_results_same_aggregation) and range (execute_range_query_pipeline's inlineKeysSourcelogic) each independently decided which keys to expand a value group into and how to handle edge cases — with different answers to the same edge cases:DeltaSetAggregator): instant hard-failed the entire query; range already skipped just that group.After: both cases are lenient in both pipelines — skip the affected group/key with a
warn!, return everything else. Instant's behavior changed to match range's (already the more lenient, and previously agreed-on, policy — see #597, which did the same thing for a third case last month).Known trade-off surfaced during review, not fixed here: the same leniency change means a genuinely malformed query (e.g.
quantile(1.5, ...), out of[0,1]— validated lazily insidequery_statistic, not at parse time) can no longer be told apart from a per-key data skew, so it's now silently swallowed into an empty/partial result on the instant side instead of a hard error. Range already had this exact gap before this PR; this PR's refactor propagates it onto instant, which previously got it right. Not fixed in this PR — needs either a structured error type distinguishing "no data for this key" from "malformed query," or upfront parameter validation inpromql.rs. Filing as a follow-up issue.Commits
ed788a3— Addedresolve_and_query_groupand rewired instant's two collector functions onto it. Two new RED→GREEN tests pin the two newly-lenient cases; full suite (577 tests) green.82a4d60— Rewired range's per-step loop onto the same function, replacing its separate inline key-resolution logic. Behavior-preserving except one deliberate, already-agreed log-level change (debug→warn on a skipped per-key failure). Full suite (577 tests) green, no regressions in any existing range test.Part of #581 (does not close it — #581 tracks the full instant/range unification; this PR is one stage of it).