From 6de42cdee691e79a5aeef06a6f709ad0758d6127 Mon Sep 17 00:00:00 2001 From: zz_y Date: Sun, 13 Sep 2026 18:28:26 -0600 Subject: [PATCH] test(planning): pin count() to series-count, not distinct cardinality PromQL `count(v)` counts series. It is not a distinct-item count, and it must not be answered by merging HLL registers: for two series holding {a,b,c} and {d,e,f} that returns 6 -- the union's distinct cardinality -- where PromQL's answer is 2. That is not hypothetical. ASAPPlanner `ca7546de`, the revision main pins today, lowers a bare `count by (d) (...)` to `AggIntent::Cardinality`, which `capability_for` binds to `Capability::CardinalityApprox` (HLL). Upstream narrowed the mapping in `b8b5d705` -- bare `count` now lowers to `AggIntent::Count`, and `Cardinality` is reserved for the distinct-count idiom (`distinct_over_time`, `COUNT(DISTINCT ...)`, or the inner node of `count(distinct_over_time(...))`). This branch already carries that bump. Nothing here changes behavior. These are the regression tests for an invariant that has flip-flopped across Planner revisions, asserted at both ends so a future bump cannot quietly reintroduce the conflation: - `query_parser`: bare `count(v)` / `count by (l) (v)` carry a Count intent and no Cardinality intent, while the distinct-count idiom keeps its Cardinality intent. - `runtime_capability`: approximate Count binds `FrequencyEstimate` and Cardinality binds `CardinalityApprox` -- never the same capability. This restates in an executable assertion what the module's own comment on `capability_for_count_approximate_returns_frequency_estimate` already argues in prose. Verified against both revisions: the parser test fails on `ca7546de` with its intended diagnostic and passes on `b8b5d705`. Co-Authored-By: Claude Opus 5 (1M context) --- .../src/physical/runtime_capability.rs | 25 +++++++ control_plane/src/query_parser/mod.rs | 65 +++++++++++++++++++ 2 files changed, 90 insertions(+) diff --git a/control_plane/src/physical/runtime_capability.rs b/control_plane/src/physical/runtime_capability.rs index ecbb1153..4074be1b 100644 --- a/control_plane/src/physical/runtime_capability.rs +++ b/control_plane/src/physical/runtime_capability.rs @@ -502,6 +502,31 @@ mod tests { ); } + /// The two intents must never collapse into one capability: an approximate + /// `Count` is a `COUNT(*)` point query (frequency sketch), while + /// `Cardinality` is `COUNT(DISTINCT ...)` (HLL). Routing `Count` to + /// `CardinalityApprox` makes a by-less `count(v)` merge HLL registers and + /// answer the union's distinct count instead of the number of series -- + /// see `query_parser::tests::bare_count_is_series_count_not_distinct_cardinality` + /// for the lowering half of this invariant. + #[test] + fn count_and_cardinality_bind_different_sketch_capabilities() { + let approximate = AccuracyTarget::Epsilon(0.01); + let count = capability_for(&AggIntent::Count { + accuracy: approximate.clone(), + }); + let cardinality = capability_for(&AggIntent::Cardinality { + col: None, + accuracy: approximate, + }); + assert_eq!(count, Some(Capability::FrequencyEstimate(None))); + assert_eq!(cardinality, Some(Capability::CardinalityApprox)); + assert_ne!( + count, cardinality, + "count() must not bind the HLL capability that answers COUNT(DISTINCT)" + ); + } + #[test] fn capability_for_count_exact_routes_to_archive() { // `count_over_time` lowers to `Count{accuracy:Exact}`. The diff --git a/control_plane/src/query_parser/mod.rs b/control_plane/src/query_parser/mod.rs index d7837d0b..9853800a 100644 --- a/control_plane/src/query_parser/mod.rs +++ b/control_plane/src/query_parser/mod.rs @@ -490,6 +490,71 @@ mod tests { other => panic!("expected canonical Aggregate, got {other:?}"), } } + /// PromQL `count(v)` counts SERIES; it is not a distinct-item count. + /// + /// This mapping has flip-flopped across ASAPPlanner revisions: `ca7546de` + /// (and several around it) lowered a bare `count by (d) (...)` to + /// `AggIntent::Cardinality`, which binds `Capability::CardinalityApprox` + /// and is served by merging HLL registers. For two series holding + /// {a,b,c} and {d,e,f} that answers 6 -- the union's distinct cardinality -- + /// where PromQL's answer is 2, the number of series. + /// + /// `Cardinality` belongs to the distinct-count idiom only: `distinct_over_time`, + /// `COUNT(DISTINCT ...)`, or an explicit `count(distinct_over_time(...))` + /// whose INNER node carries the intent. Keeping the two apart is what lets + /// `capability_for` route `Count` to a frequency sketch and `Cardinality` + /// to HLL, as `runtime_capability`'s own contract states. + #[test] + fn bare_count_is_series_count_not_distinct_cardinality() { + fn intents(query: &str) -> Vec { + fn walk(expr: &QueryExpr, out: &mut Vec) { + match expr { + QueryExpr::Aggregate { + measures, child, .. + } => { + out.extend(measures.iter().cloned()); + walk(child, out); + } + QueryExpr::TimeRange { child, .. } => walk(child, out), + _ => {} + } + } + let mut out = vec![]; + walk(&parse_query_expr_canonical(query, ACC).unwrap(), &mut out); + out + } + + for query in ["count(unique_users)", "count by (svc) (unique_users)"] { + let found = intents(query); + assert!( + found + .iter() + .all(|intent| !matches!(intent, AggIntent::Cardinality { .. })), + "{query} must not carry a Cardinality intent -- that binds HLL and \ + answers the union's distinct count instead of the series count; got {found:?}" + ); + assert!( + found + .iter() + .any(|intent| matches!(intent, AggIntent::Count { .. })), + "{query} must lower to a Count intent, got {found:?}" + ); + } + + // The distinct-count idiom keeps its cardinality intent, on the inner node. + for query in [ + "distinct_over_time(unique_users[5m])", + "count(distinct_over_time(unique_users[5m]))", + ] { + let found = intents(query); + assert!( + found + .iter() + .any(|intent| matches!(intent, AggIntent::Cardinality { .. })), + "{query} is the distinct-count idiom and must keep Cardinality, got {found:?}" + ); + } + } } #[cfg(test)]