You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
refactor(asap-types): model aggregation kinds and subtypes with typed enums #670
AggregationConfig represents an aggregation with an AggregationType plus an unconstrained aggregation_sub_type: String. The string is overloaded across unrelated aggregation kinds: plain CMS uses sum/count, MinMax uses min/max, heap CMS uses topk while its weighting is stored separately in parameters, Increase uses values such as rate, and aggregations without a subtype use an empty string.
This makes invalid combinations representable and forces the factory, planner, capability matcher, deserializers, and tests to interpret the same strings independently. Empty-string sentinels also make it unclear whether a subtype is intentionally absent or simply missing.
Goal
Replace the stringly-typed aggregation-kind/subtype representation with a typed, discriminated model. At minimum, plain CountMinSketch must have an explicit SUM-versus-COUNT mode, and other aggregation-specific modes must not be representable under the wrong aggregation kind.
Requirements
Define typed variants for aggregation-specific modes, including plain CMS SUM/COUNT and MinMax min/max.
Decide how heap CMS topk and count_events should be represented rather than splitting semantics between subtype and an untyped parameter map.
Preserve or explicitly migrate the existing serialized configuration format. Existing valid configs must remain readable during the migration.
Move validation to the configuration seam so factories, planners, matchers, and workers consume the same typed value.
Make absence of a subtype explicit for aggregation kinds that do not use one; do not use empty strings as implicit sentinels in new code.
Preserve the intentional legacy MultipleSubpopulation behavior unless a separate compatibility decision is documented.
Add behavioral tests for parsing, serialization compatibility, factory selection, and capability matching.
Non-goals
This is a representation and validation refactor; it should not change the intended SUM/COUNT behavior delivered by #505 or the heap follow-up tracked by #666.
Clarification: plain CountMinSketch uses aggregation_sub_type with sum/count. CountMinSketchWithHeap uses aggregation_sub_type: topk to identify its top-k structure; its SUM-versus-COUNT weighting is currently controlled separately by parameters["count_events"] (true or omitted for COUNT, false for SUM). The interim checks should preserve this distinction. A future typed model should represent both dimensions explicitly rather than forcing heap CMS to use sum/count as its subtype.
Finding 856: The legacy MultipleSubpopulation + CountMinSketch factory arm hardcodes SUM semantics (count_events: false) and does not validate a nested subtype. This path uses aggregation_sub_type as the inner accumulator kind (CountMinSketch/cms), unlike canonical CountMinSketch, where it means sum/count. Changing it is therefore a compatibility/design decision; defer it to this typed aggregation model refactor.
Finding 744: Plain CMS subtype interpretation is duplicated between query-engine factory construction and asap-types capability matching. The helpers have different directions, and asap-types cannot depend on query-engine. The durable fix is to introduce a shared typed aggregation/subtype model or shared parser in this refactor, then make both callers consume it.
Both findings are intentionally out of scope for the current CMS behavior fix.
Activity
milindsrivastava1997 commentedon Sep 2, 2026
Clarification: plain
CountMinSketchusesaggregation_sub_typewithsum/count.CountMinSketchWithHeapusesaggregation_sub_type: topkto identify its top-k structure; its SUM-versus-COUNT weighting is currently controlled separately byparameters["count_events"](trueor omitted for COUNT,falsefor SUM). The interim checks should preserve this distinction. A future typed model should represent both dimensions explicitly rather than forcing heap CMS to usesum/countas its subtype.milindsrivastava1997 commentedon Sep 2, 2026
Triage follow-ups from the CMS review:
MultipleSubpopulation + CountMinSketchfactory arm hardcodes SUM semantics (count_events: false) and does not validate a nested subtype. This path usesaggregation_sub_typeas the inner accumulator kind (CountMinSketch/cms), unlike canonicalCountMinSketch, where it meanssum/count. Changing it is therefore a compatibility/design decision; defer it to this typed aggregation model refactor.Both findings are intentionally out of scope for the current CMS behavior fix.
fix(asap-summary-ingest): update Arroyo UDF selection for heap-CMS su…
refactor(asap-types): delete dead SingleSubpopulation/MultipleSubpopu…
refactor(asap-types): fold heap-CMS count_events into aggregation_sub…
fix(asap-summary-ingest): update Arroyo UDF selection for heap-CMS su…
refactor: fold heap-CMS count_events into aggregation_sub_type (#719)