fix(query-engine): merge all sliding-window buckets per key instead of taking first - #570
Merged
Merged
Conversation
This was referenced Aug 21, 2026
Closed
…f taking first execute_and_merge_store_queries kept only the first precomputed bucket per key for Sliding-window queries, discarding the rest when the store returned more than expected. DataFusion's SummaryMergeMultipleExec already merges all of them correctly, so binary-expr queries (still on DataFusion) don't hit this. #567 will move binary-expr onto this native path, so this native/DataFusion behavior gap needed closing first. Part of #567 Stage 1. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… and stale latency label - Sliding branch now delegates to merge_precomputed_outputs (do_merge=true) instead of hand-rolling its own extract/merge/insert loop, removing the duplication with the Tumbling branch's merge path. - merge_accumulators now takes ownership of the accumulator Vec so its single-element shortcut can move the value out instead of re-cloning it on top of the clone already done to build the Vec. - The [LATENCY] log's merge/no-merge label was hardcoded off window_type and said "no merge" even when merge_accumulators was in fact called; it now reflects whether merging actually occurs. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…compute buckets merge_precomputed_outputs silently skipped any key whose timestamped_buckets list was empty, with no log signal — unlike the "found N, expected 1" mismatch case a few lines up in the Sliding caller, which does warn. Since this function is shared by Sliding, Tumbling, and the keys-merge path, the warn now covers all three instead of being Sliding-only. Also files #575 to compute EXPECTED_BUCKETS_PER_KEY instead of hardcoding it to 1, since #554 will make >1 legitimate whenever a sliding-window query's range exceeds the window size. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
milindsrivastava1997
force-pushed
the
567-1-sliding-bucket-merge-fix
branch
from
August 23, 2026 02:46
506489e to
8e768fa
Compare
This was referenced Aug 23, 2026
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.
Summary
execute_and_merge_store_queries's Sliding-window branch kept only the first precomputed bucket per key and discarded the rest when the store returned more than expected — DataFusion'sSummaryMergeMultipleExecalready merges all of them correctly, so this was a real native/DataFusion behavior gap.Stage 1 of #567 (converting binary-arithmetic PromQL instant queries from DataFusion to native execution) — this closes the gap before binary-expr queries move onto this native path in a later stage, so the cutover doesn't inherit the bug.
Test plan
native_pipeline_merge_tests.rs: single-bucket baseline, multi-bucket merge (red before the fix, green after), 3-bucket generalization, Tumbling-branch regression guardcargo test --lib(536 tests) greencargo clippy --lib --tests -- -D warningscleanSide finding filed separately as #569 (pre-existing, unrelated Tumbling-path panic surfaced while writing the regression guard).