Skip to content

fix(query-engine): range queries expand keys_query and merge same-timestamp buckets - #582

Merged
milindsrivastava1997 merged 3 commits into
mainfrom
worktree-issue-580-range-query-fixes
Aug 24, 2026
Merged

fix(query-engine): range queries expand keys_query and merge same-timestamp buckets#582
milindsrivastava1997 merged 3 commits into
mainfrom
worktree-issue-580-range-query-fixes

Conversation

@milindsrivastava1997

Copy link
Copy Markdown
Contributor

Summary

Fixes #580.

Test plan

  • Added native_range_query_tests.rs: RED before the fix, GREEN after — dual-population single-step, dual-population multi-step, sliding 2-bucket collision, sliding 3-bucket collision, sliding single-bucket regression guard.
  • cargo test -p query_engine_rust --lib — 552 passed, 0 failed.
  • cargo clippy -p query_engine_rust --lib --tests — clean.

…estamp buckets (#580)

execute_range_query_pipeline never read keys_query, so dual-population
metrics returned nothing over a range. It also collapsed same-start-
timestamp buckets in a HashMap, dropping all but one when a Sliding
aggregation legitimately returns more than one bucket per window
(#567/#570). Mirrors the keys fetch/merge/expand pattern already used
by execute_and_merge_store_queries/collect_results_separate_keys.
@milindsrivastava1997

Copy link
Copy Markdown
Contributor Author

Code review findings on execute_range_query_pipeline (asap-query-engine/src/engines/simple_engine/mod.rs):

  1. mod.rs:1512 — Range key expansion silently skips a value group (continue, no log/error) when get_keys() returns None or the group key is missing from merged_keys, while the mirrored instant-query path returns an Err for the same condition. A dual-population metric using DeltaSetAggregator whose key set ever shrank (get_keys() returns None per delta_set_aggregator_accumulator.rs:251-258) causes this path to silently return an empty-but-Ok result instead of erroring, so handle_range_query_promql's Prometheus fallback (which only triggers on Err) never kicks in — callers get a wrongly-empty successful response.

  2. mod.rs:1463 — The keys aggregation is fetched and merged once up front and reused unchanged for every output timestep in the range loop, even though key membership can legitimately change over the queried interval. For a dual-population range query where labels are added/removed partway through the range, every timestep gets the same key expansion from the final snapshot — producing phantom samples for keys that didn't exist yet early on, or missing series for keys gone by the range's end.

  3. mod.rs:1463 — The new keys-fetch-and-merge block duplicates equivalent logic already in execute_and_merge_store_queries (mod.rs:596-625) instead of sharing a helper. The two copies have already diverged (instant path passes the real do_merge flag and logs latency; range path hardcodes true and drops logging), so a future fix to one merge path is likely to be applied to only one copy.

…fetch/merge (#582 review)

- execute_range_query_pipeline now iterates the merged keys map (like
  collect_results_separate_keys) and fails the whole query on an
  unresolvable key set, instead of silently continue-ing past it.
- Extracted fetch_and_merge_keys, shared by the instant and range
  paths, and pass the real do_merge flag through instead of
  hardcoding true for range.
- merge_precomputed_outputs's spatial (non-merge) branch now warns
  and merges instead of asserting exactly 1 precompute per key, since
  do_merge=false no longer guarantees that under a range query's
  widened fetch (and won't once Sliding needs merging there too).
@milindsrivastava1997

Copy link
Copy Markdown
Contributor Author

Follow-up finding on the latest push (7b9f084), in execute_range_query_pipeline:

keys_query's fetch window is never widened for range queries, only values_query is (asap-query-engine/src/engines/simple_engine/mod.rs:1491, promql.rs:583). finish_range_context widens values_query to [start-lookback, end] but clones keys_query unchanged, and keys_query's window is computed from a single instant (create_keys_query_params, using only end_timestamp):

  • SetAggregator ("latest window only") → keys window is just [end-window_size, end], the last window of the range.
  • DeltaSetAggregator ("all keys since start") → keys window is [0, end], all of history.

This causes two different failure modes:

  1. Silent data loss (SetAggregator): the range loop iterates over the keys present in merged_keys, not over all_data. A label with real values earlier in the range but absent from the final window (e.g. a pod that scaled down) never gets iterated at all — its whole history silently disappears from the output, no error.

  2. Whole-query failure (DeltaSetAggregator): keys_query's [0, end] window picks up keys from arbitrarily far in the past — including ones whose only data point is well before the queried range and outside the (still-bounded) widened values_query window. That key is in merged_keys but missing from all_data, and the new hard-error semantics (ok_or_else(...)? at mod.rs:1541) fail the entire range response over one stale key, even though every other key in the query has good data.

None of the new tests in native_range_query_tests.rs exercise mismatched key/value windows (all use 1:1 matching timestamps), so this path is untested.

@milindsrivastava1997

Copy link
Copy Markdown
Contributor Author

Correction on the DeltaSetAggregator part of the finding above.

`merge_accumulators` in `delta_set_aggregator_accumulator.rs` does correctly reconcile add/remove deltas across `[0, T]` — a key that's added then removed cancels out. So it's not "picks up every key that ever existed" as I said.

The real trigger is simpler: `get_keys()` (delta_set_aggregator_accumulator.rs:251-258) returns `None` for the entire accumulator if `removed` is non-empty post-merge, instead of returning `added.difference(&removed)`. Since `removed` is non-empty as soon as any key has ever been removed across the metric's full history, this fires on ordinary label churn, not just stale/ancient keys. Pre-existing (since #342), not introduced by this PR — but #582's new hard-fail on missing keys (mod.rs:1547) turns this common case into a full range-query failure instead of the previous silent skip.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Range query pipeline drops keys_query and hardcodes is_exact_query=false

1 participant