Skip to content

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

Open
milindsrivastava1997 wants to merge 3 commits into
mainfrom
worktree-issue-580-range-query-fixes
Open

fix(query-engine): range queries expand keys_query and merge same-timestamp buckets#582
milindsrivastava1997 wants to merge 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).
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