refactor: fold heap-CMS count_events into aggregation_sub_type - #719
Open
milindsrivastava1997 wants to merge 4 commits into
Open
refactor: fold heap-CMS count_events into aggregation_sub_type#719milindsrivastava1997 wants to merge 4 commits into
milindsrivastava1997 wants to merge 4 commits into
Conversation
…lation aggregation types (#670) Both variants were only ever constructed in #[cfg(test)] code; the real statistic-to-aggregation-type mapping never produces them. Removing them also deletes their legacy factory arm that reused aggregation_sub_type as an inner-accumulator-kind string and hardcoded SUM semantics for nested CMS (issue #670, Finding 856), since the dead code is gone rather than preserved. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K2SLcSk9UnZi5iAWKt26kH
…_type (#670) CountMinSketchWithHeap previously split its SUM/COUNT weighting across two places: aggregation_sub_type (fixed to the now-redundant "topk") and a separate parameters["count_events"] boolean, duplicating and diverging from how plain CountMinSketch/MultipleSum already encode the same axis via sub_type alone. Hard-cutover to aggregation_sub_type: "sum"|"count" for all three CMS-family types, removing count_events entirely. Introduces asap_types::aggregation_mode (AggregationMode/CountMode/ MinMaxMode) as the single typed seam AggregationConfig::mode() exposes; capability_matching's three separate weighting-compatibility functions and accumulator_factory's duplicate sub_type parsers now route through it instead of each re-deriving the same semantics. The planner (promql.rs, sql.rs) patches a topk candidate's sub_type to the detected weighting right after building it, since map_statistic_to_ precompute_operator's placeholder "topk" sub_type is only a stand-in until the real weighting is known; candidate_gen.rs/atomic_costs.rs updated to match. Backward-compat: none needed, AggregationConfig is regenerated by the planner per query rather than persisted. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K2SLcSk9UnZi5iAWKt26kH
…b_type change
The Arroyo pipeline generator derives its UDF name as
f"{aggregationType}_{aggregationSubType}", and the topk-vs-non-topk column
selection patch keyed off aggregationSubType == "topk". Both broke once
CountMinSketchWithHeap's sub_type moved to "sum"/"count" (2e950f5, #670):
UDF lookup would 404 on the missing countminsketchwithheap_sum/_count
templates, and the column-selection patch would silently stop firing.
Renames the existing (COUNT-semantics) UDF template/function to
countminsketchwithheap_count, and switches the column-selection check to
aggregationType, which uniquely identifies a topk config regardless of its
subtype. A value-weighted (SUM) Arroyo UDF was never implemented -- the old
single template always used count semantics regardless of weighting -- so
countminsketchwithheap_sum intentionally has no template yet; that case now
fails loudly with a clear "template not found" error instead of silently
returning wrong (count-weighted) results, which is what actually happened
before this change.
Roborev: closes review 205.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Completes ae9c593 (the file rename alone accidentally landed as its own commit): renames the UDF fn inside the template to match, switches the topk column-selection patch from aggregationSubType == "topk" to aggregationType == "countminsketchwithheap", and adds a regression test covering both new sub_type values. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
milindsrivastava1997
changed the title
milindsrivastava1997
marked this pull request as ready for review
September 5, 2026 03:12
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.
What
CountMinSketchWithHeapused to split its SUM-vs-COUNT weighting across two places:aggregation_sub_type(always the redundant literal"topk") and a separateparameters["count_events"]boolean. PlainCountMinSketch/MultipleSumalready encode the same SUM/COUNT choice withaggregation_sub_typealone, so heap-CMS now does the same —aggregation_sub_type: "sum" | "count", no morecount_eventsparameter.Along the way:
AggregationType::SingleSubpopulation/MultipleSubpopulation— turned out to be dead code, only ever constructed in tests.asap_types::aggregation_mode(AggregationMode/CountMode/MinMaxMode) as one typed place to interpretaggregation_sub_type, used by capability matching, the accumulator factory, and the planner instead of each re-parsing the string themselves.No backward-compat shim needed —
AggregationConfigis generated fresh by the planner per query, not persisted, so there's nothing old-format to keep reading.Closes #670
Test plan
cargo testgreen (asap_types, asap-planner-rs, asap-query-engine)cargo clippy --workspace --all-targetsclean🤖 Generated with Claude Code