fix(query-engine): expand self-keyed accumulators in range queries - #587
fix(query-engine): expand self-keyed accumulators in range queries#587milindsrivastava1997 wants to merge 2 commits into
Conversation
…k) without keys_query execute_range_query_pipeline's single-population branch used each value group's own store-level group_key directly and never called get_keys() on the merged value accumulator, so self-keyed accumulators like CountMinSketchWithHeap (top-k) returned empty over a range instead of expanding into their top-k keys — the instant path already does this via collect_results_same_aggregation. Now every group tries merged.get_keys() first (mirroring the instant path exactly) and only falls back to the precomputed key list — the store's group_key for single-population metrics, or the separate keys aggregation's expansion for dual-population metrics — when get_keys() returns None. This also covers self-keyed accumulators stored under a non-None outer key (real Arroyo/worker.rs ingestion always wraps Some(key), even for empty grouping), not just the None-keyed case from the original report. Fixes #584. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
This is reachable in production: The new tests ( Suggest gating the |
…eys_query in range queries Review on #587 caught a regression: the previous fix called merged.get_keys() unconditionally on every group's merged value accumulator, including dual-population groups (separate keys_query present). collect_all_results (instant path) branches globally on query shape instead — dual-population always goes through collect_results_separate_keys, which never consults the value accumulator's own get_keys() at all. A real, tested capability-matched config (sql.rs) pairs a self-keyed CountMinSketchWithHeap value aggregation with a separate DeltaSetAggregator keys aggregation; the previous fix let the heap's own (window-shifting) top-k keys silently override the keys aggregation's expansion for that config. Range queries now mirror collect_all_results exactly: dual-population groups always use the keys aggregation's expansion, full stop; only single-population groups let the value accumulator's own get_keys() take priority (falling back to the store-level group key), which is what #584 actually needed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1 participant
Summary
execute_range_query_pipeline's single-population branch never calledget_keys()on the merged value accumulator, so self-keyed accumulators likeCountMinSketchWithHeap(top-k) returned empty over a range instead of expanding into their top-k keys — the instant path already does this viacollect_results_same_aggregation.merged.get_keys()first (mirroring the instant path exactly) and only falls back to the precomputed key list when it returnsNone. This also covers self-keyed accumulators stored under a non-Noneouter key (real Arroyo/worker.rs ingestion always wrapsSome(key), even for empty grouping), not just theNone-keyed case from the original report.Fixes #584.
Test plan
cargo test -p query_engine_rust --lib native_range_query_tests— 7 passed, including two new regression tests for Range queries drop self-keyed accumulator expansion (top-k) for single-population metrics #584 (range_query_self_keyed_topk_expands_without_keys_query,range_query_self_keyed_topk_expands_with_non_none_outer_key)cargo test -p query_engine_rust --lib— 554 passed, 0 failed🤖 Generated with Claude Code