fix(planner): restrict DeltaSetAggregator to tumbling windows - #606
Merged
Conversation
…y window_type (#588) DeltaSetAggregator only tracks added/removed keys since the last window, so it's only correct for non-overlapping (tumbling) windows. build_agg_configs_for_statistics was copying the surrounding query's window_type straight through, which would silently plan a Sliding DeltaSetAggregator once sliding windows are enabled. Always plan the companion config as Tumbling, sized to the sibling value aggregation's slide_interval_ms so its buckets align with the pane-close cadence the value aggregation already emits at. No-op under Tumbling (the only window_type the live planner produces today). Co-Authored-By: Claude Sonnet 5 <[email protected]>
…ration (#588) candidate_gen.rs's enumerate_candidates was enumerating Sliding window candidates for every agg type compatible with Statistic::Cardinality, including DeltaSetAggregator -- which is only correct under non-overlapping (tumbling) windows. This optimizer module isn't wired into the live planner yet, but should already encode the invariant before it is. Skip Sliding entries from window_candidates() when agg_type is DeltaSetAggregator; Tumbling candidates for it, and Sliding candidates for its siblings (SetAggregator, HLL), are unaffected. Co-Authored-By: Claude Sonnet 5 <[email protected]>
…hing (#588) DeltaSetAggregator only tracks added/removed keys since the last window, so it's only correct for non-overlapping (tumbling) windows. The two planner-side producers were fixed to never emit one, but find_compatible_aggregation's paired-key-agg lookup matched candidates by (metric, is_key_agg_type) alone, bypassing window_compatible entirely -- a Sliding DeltaSetAggregator built by any other path could still be selected to serve a query and silently produce incorrect merged add/remove sets. Add key_agg_window_valid() as the single source of truth for this invariant, wired into both window_compatible() (the value-slot matching path) and the key-agg find() filter (the actual pairing path CountMinSketch/HydraKLL rely on). When a key agg exists on the metric but gets filtered out for this reason, log the specific cause instead of the generic "none found" warning. Co-Authored-By: Claude Sonnet 5 <[email protected]>
milindsrivastava1997
marked this pull request as ready for review
August 25, 2026 16:42
Contributor
Author
|
Reviewed — build/tests pass, no correctness regressions found in the DeltaSetAggregator/Sliding fix (#588). Two minor cleanups:
|
milindsrivastava1997
changed the title
- candidate_gen.rs: call the shared key_agg_window_valid predicate instead of duplicating the DeltaSetAggregator+Sliding check inline, so the invariant has one source of truth. - capability_matching.rs: fold the key-agg miss-path diagnostic into the same single pass over configs.values() instead of re-scanning to find the rejected candidate separately. Co-Authored-By: Claude Sonnet 5 <[email protected]>
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
agg_config.rs: force the DeltaSetAggregator companion config to Tumbling regardless of the surrounding query's window_type, sized to the sibling'sslide_interval_ms.candidate_gen.rs: the (not-yet-wired-in) cost optimizer no longer enumerates Sliding candidates for DeltaSetAggregator.capability_matching.rs: newkey_agg_window_valid()gate, wired intowindow_compatible()and the key-agg pairing lookup (which previously bypassed window checking entirely) — the actual belt-and-suspenders fix that stops a bad config from ever being selected to serve a query.Closes #588.
Test plan
cargo test -p asap_planner -p query_engine_rust -p asap_types --lib— 695 tests pass, 0 failures🤖 Generated with Claude Code