From b944839c9efb1d7e670741770d2f02f2064b90c1 Mon Sep 17 00:00:00 2001 From: zz_y Date: Fri, 15 May 2026 23:23:00 -0600 Subject: [PATCH] fix(enums): close AggregationType match gaps for CountSketch[WithHeap] MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Audit follow-up from PR #258 — the new `CountSketchWithHeap` enum variant exposed two pre-existing panic / capability-miss paths that needed parallel arms: - `does_precompute_operator_support_subpopulations` (logics.rs): the catch-all `_ => panic!(...)` would fire for both `AggregationType::CountSketch` and `CountSketchWithHeap` if callers ever routed those through. Added explicit Sum/Count arms for CountSketch (mirrors the CMS arm — signed-counter equivalent) and a Topk arm for the heap-bearing variant (returns false, parallel to `CountMinSketchWithHeap`). - `compatible_agg_types(Statistic::Topk)` (capability_matching.rs): the list previously named `CountMinSketchWithHeap` and bare `CountSketch` but omitted `CountSketchWithHeap` — capability misses on a heap-bearing CountSketch policy would route the Topk query past the warm engine. Added the third entry. Test coverage in `test_does_precompute_operator_support_subpopulations` extended to assert no-panic for both new variants on their canonical statistics. Co-Authored-By: Claude Opus 4.7 (1M context) --- crates/asap_types/src/capability_matching.rs | 11 ++++++-- .../src/query_logics/logics.rs | 28 +++++++++++++++++++ 2 files changed, 37 insertions(+), 2 deletions(-) diff --git a/crates/asap_types/src/capability_matching.rs b/crates/asap_types/src/capability_matching.rs index 3ddd270e..57330399 100644 --- a/crates/asap_types/src/capability_matching.rs +++ b/crates/asap_types/src/capability_matching.rs @@ -235,11 +235,18 @@ pub fn compatible_agg_types(stat: Statistic) -> &'static [AggregationType] { // signed-counter matrix). `CountSketchAccumulator` answers // `Statistic::Topk` directly — see // `precompute_operators/count_sketch_accumulator.rs:284`. - // Without CountSketch listed here, `topk(K, top_endpoint_qps)` - // capability-misses and the warm engine returns `status=error`. + // `CountSketchWithHeap` is the explicit heap-bearing variant + // that also satisfies Topk through the heap directly + // (parallel to `CountMinSketchWithHeap`); the analyzer's + // `topk(...)` candidate returns `FrequencyTopk(Any)` so + // either heap-bearing variant matches. + // Without CountSketch / CountSketchWithHeap listed here, + // `topk(K, top_endpoint_qps)` capability-misses and the + // warm engine returns `status=error`. Statistic::Topk => &[ AggregationType::CountMinSketchWithHeap, AggregationType::CountSketch, + AggregationType::CountSketchWithHeap, ], } } diff --git a/crates/promql_utilities/src/query_logics/logics.rs b/crates/promql_utilities/src/query_logics/logics.rs index 8aa4c4c3..9772c18f 100644 --- a/crates/promql_utilities/src/query_logics/logics.rs +++ b/crates/promql_utilities/src/query_logics/logics.rs @@ -101,6 +101,12 @@ pub fn does_precompute_operator_support_subpopulations( // CountMinSketchWithHeap is only supported for Topk — does not support subpopulations AggregationType::CountMinSketchWithHeap if matches!(statistic, Statistic::Topk) => false, + // CountSketch is the signed-counter equivalent of CMS — same + // subpopulation shape for Sum/Count statistics. The + // heap-bearing variant covers Topk like its CMS counterpart. + AggregationType::CountSketch => matches!(statistic, Statistic::Sum | Statistic::Count), + AggregationType::CountSketchWithHeap if matches!(statistic, Statistic::Topk) => false, + // Default: not supported _ => panic!("Unexpected precompute operator: {}", precompute_operator), } @@ -186,6 +192,28 @@ mod tests { Statistic::Sum, AggregationType::CountMinSketch, )); + + // Sibling CountSketch path — must not panic, matches + // CountMinSketch's Sum/Count subpopulation shape. + assert!(does_precompute_operator_support_subpopulations( + Statistic::Sum, + AggregationType::CountSketch, + )); + assert!(does_precompute_operator_support_subpopulations( + Statistic::Count, + AggregationType::CountSketch, + )); + + // Heap-bearing variants on Topk — both return false (heap + // is per-policy, not subpopulation-keyed) and must not panic. + assert!(!does_precompute_operator_support_subpopulations( + Statistic::Topk, + AggregationType::CountMinSketchWithHeap, + )); + assert!(!does_precompute_operator_support_subpopulations( + Statistic::Topk, + AggregationType::CountSketchWithHeap, + )); } #[test]