Refined comments

Author: akanksha-akkihal

asap-common/dependencies/rs/asap_types/src/capability_matching.rs

@@ -33,10 +33,6 @@ pub fn compatible_agg_types(stat: Statistic) -> &'static [AggregationType] {
         Statistic::Cardinality => &[
             AggregationType::SetAggregator,
             AggregationType::DeltaSetAggregator,
-            // HLL is the single-population probabilistic alternative used by
-            // `COUNT(DISTINCT col) GROUP BY …` queries. It backs the per-bucket
-            // sketch directly (no paired key aggregation required) — see
-            // `is_multi_population_value_type`, which excludes HLL.
             AggregationType::HLL,
         ],
         Statistic::Topk => &[AggregationType::CountMinSketchWithHeap],

asap-common/dependencies/rs/promql_utilities/src/query_logics/enums.rs

@@ -185,8 +185,6 @@ pub enum AggregationOperator {
     Min,
     Max,
     Topk,
-    /// Distinct-value count. SQL-side normalisation maps `COUNT(DISTINCT col)` to
-    /// the aggregation name "CARDINALITY", which lands here.
     Cardinality,
 }
 
@@ -453,8 +451,7 @@ mod tests {
     fn test_aggregation_operator_cardinality_round_trip() {
         // The SQL parser normalises `COUNT(DISTINCT col)` to the aggregation name
         // "CARDINALITY"; `parse_single_statistic` then routes it through
-        // `AggregationOperator::FromStr`. Without a Cardinality variant the lookup
-        // returns Err and the query is rejected as "Unsupported statistic name".
+        // `AggregationOperator::FromStr`. 
         let op: AggregationOperator = "cardinality".parse().expect("cardinality should parse");
         assert_eq!(op, AggregationOperator::Cardinality);
         assert_eq!(op.to_statistics(), vec![Statistic::Cardinality]);

asap-common/dependencies/rs/sql_utilities/src/ast_matching/sqlparser_test.rs

@@ -595,73 +595,6 @@ mod tests {
         );
     }
 
-    /// Regression for three matcher-side gaps that surface together when the
-    /// simple_engine SQL path runs an HLL `COUNT(DISTINCT)` query end-to-end:
-    ///
-    /// 1. The parser correctly normalises `COUNT(DISTINCT col)` to the
-    ///    aggregation name `"CARDINALITY"`, but
-    ///    `SQLPatternMatcher::is_valid_aggregation` never gained `CARDINALITY`
-    ///    in its `legal_aggregations` set, so the validator rejected the query
-    ///    with `IllegalAggregationFn` before pattern matching ran.
-    ///
-    /// 2. After fixing (1), `flatten_query_info` validates the aggregation's
-    ///    "value column" against `schema.is_valid_value_column`, which only
-    ///    knows table value columns. `COUNT(DISTINCT col)` legitimately targets
-    ///    metadata/label columns (e.g. `COUNT(DISTINCT dstip)`), so the
-    ///    validator rejected it with `InvalidValueCol`. The fix accepts
-    ///    metadata columns *only* for CARDINALITY.
-    ///
-    /// 3. With both fixed, the query classifies as `SpatioTemporal` because
-    ///    `GROUP BY` only covers a subset of metadata columns — exactly the
-    ///    shape of the user's real `COUNT(DISTINCT dstip) GROUP BY srcip`
-    ///    query, which selects on `srcip` and aggregates over `dstip` (so
-    ///    labels ⊊ metadata_columns).
-    ///
-    /// Observed log line that motivated this test:
-    ///   error: Some(IllegalAggregationFn),
-    ///   msg: Some("attempt to use illegal aggregation function CARDINALITY")
-    #[test]
-    fn test_count_distinct_passes_aggregation_allowlist() {
-        check_query(
-            "SELECT COUNT(DISTINCT L4) FROM cpu_usage \
-             WHERE time BETWEEN DATEADD(s, -10, NOW()) AND NOW() \
-             GROUP BY L1, L2, L3",
-            vec![QueryType::SpatioTemporal],
-            None,
-        );
-    }
-
-    /// Companion: when `GROUP BY` covers all metadata columns *except* the
-    /// distinct-target itself, the query is still SpatioTemporal — the
-    /// distinct-target is the value column, not a grouping label, so labels
-    /// always form a strict subset of metadata_columns. Guards against future
-    /// "treat L4 as both label and value" regressions in the classifier.
-    #[test]
-    fn test_count_distinct_with_full_remaining_labels_is_spatiotemporal() {
-        check_query(
-            "SELECT COUNT(DISTINCT L4) FROM cpu_usage \
-             WHERE time BETWEEN DATEADD(s, -10, NOW()) AND NOW() \
-             GROUP BY L1, L2, L3",
-            vec![QueryType::SpatioTemporal],
-            None,
-        );
-    }
-
-    /// Negative case: `COUNT(DISTINCT not_in_schema)` against a column that's
-    /// neither a value_column nor a metadata_column must still be rejected as
-    /// `InvalidValueCol`. The CARDINALITY relaxation widens what's *allowed*
-    /// (metadata columns) but doesn't disable the schema check entirely.
-    #[test]
-    fn test_count_distinct_unknown_column_still_rejected() {
-        check_query(
-            "SELECT COUNT(DISTINCT bogus_column) FROM cpu_usage \
-             WHERE time BETWEEN DATEADD(s, -10, NOW()) AND NOW() \
-             GROUP BY L1, L2, L3",
-            vec![],
-            Some(QueryError::InvalidValueCol),
-        );
-    }
-
     #[test]
     fn test_error_spatial_scrape_duration_too_small() {
         check_query(
@@ -1107,8 +1040,7 @@ mod tests {
     // `COUNT(DISTINCT col)` must be normalised to a cardinality aggregation
     // (`AggregationInfo.name == "CARDINALITY"`) so the engine routes it to a
     // distinct-tracking sketch (SetAggregator / HLL) instead of a plain Count
-    // sketch. The parser today drops `DISTINCT` silently — a parser-level bug
-    // that would dispatch streaming counts as totals.
+    // sketch. 
 
     #[test]
     fn test_count_distinct_single_column_maps_to_cardinality() {
@@ -1240,4 +1172,49 @@ mod tests {
         )
         .is_none());
     }
+
+    /// Matcher must accept parser-normalised `CARDINALITY` (not `IllegalAggregationFn`),
+    /// allow distinct targets in metadata_columns (e.g. `dstip`), and classify
+    /// `COUNT(DISTINCT col) GROUP BY <label subset>` as `SpatioTemporal`.
+    #[test]
+    fn test_count_distinct_passes_aggregation_allowlist() {
+        check_query(
+            "SELECT COUNT(DISTINCT L4) FROM cpu_usage \
+             WHERE time BETWEEN DATEADD(s, -10, NOW()) AND NOW() \
+             GROUP BY L1, L2, L3",
+            vec![QueryType::SpatioTemporal],
+            None,
+        );
+    }
+
+    /// Companion: when `GROUP BY` covers all metadata columns *except* the
+    /// distinct-target itself, the query is still SpatioTemporal — the
+    /// distinct-target is the value column, not a grouping label, so labels
+    /// always form a strict subset of metadata_columns. Guards against future
+    /// "treat L4 as both label and value" regressions in the classifier.
+    #[test]
+    fn test_count_distinct_with_full_remaining_labels_is_spatiotemporal() {
+        check_query(
+            "SELECT COUNT(DISTINCT L4) FROM cpu_usage \
+             WHERE time BETWEEN DATEADD(s, -10, NOW()) AND NOW() \
+             GROUP BY L1, L2, L3",
+            vec![QueryType::SpatioTemporal],
+            None,
+        );
+    }
+
+    /// Negative case: `COUNT(DISTINCT not_in_schema)` against a column that's
+    /// neither a value_column nor a metadata_column must still be rejected as
+    /// `InvalidValueCol`. The CARDINALITY relaxation widens what's *allowed*
+    /// (metadata columns) but doesn't disable the schema check entirely.
+    #[test]
+    fn test_count_distinct_unknown_column_still_rejected() {
+        check_query(
+            "SELECT COUNT(DISTINCT bogus_column) FROM cpu_usage \
+             WHERE time BETWEEN DATEADD(s, -10, NOW()) AND NOW() \
+             GROUP BY L1, L2, L3",
+            vec![],
+            Some(QueryError::InvalidValueCol),
+        );
+    }
 }

asap-common/dependencies/rs/sql_utilities/src/ast_matching/sqlpattern_matcher.rs

@@ -102,11 +102,7 @@ impl SQLPatternMatcher {
         legal_aggregations.insert("MIN");
         legal_aggregations.insert("MAX");
         legal_aggregations.insert("QUANTILE");
-        // `COUNT(DISTINCT col)` is normalised by the parser to the aggregation
-        // name "CARDINALITY" (see `SQLPatternParser::get_aggregation`). Without
-        // this entry the simple_engine SQL handler rejects every COUNT(DISTINCT)
-        // query with `IllegalAggregationFn` before pattern matching runs, which
-        // in turn blocks routing to the precompute engine's HLL accumulator.
+        // COUNT(DISTINCT col) is normalised by the parser to the aggregationname "CARDINALITY"
         legal_aggregations.insert("CARDINALITY");
 
         Self {

asap-query-engine/src/precompute_engine/accumulator_factory.rs

@@ -896,13 +896,8 @@ mod tests {
         }
     }
 
-    // ── HLL updater ──────────────────────────────────────────────────────
-    //
-    // `COUNT(DISTINCT col)` queries flow through `AggregationType::HLL`. The
-    // factory must produce an `HllAccumulatorUpdater` (not the silent
-    // `SumAccumulatorUpdater` fallback that the old default arm gave) so the
-    // streaming layer actually hashes incoming samples into an HLL register
-    // array rather than summing them.
+    // HLL: `AggregationType::HLL` must build `HllAccumulatorUpdater` (hashes samples
+    // into a sketch), not fall through to the default `SumAccumulatorUpdater`.
 
     #[test]
     fn test_hll_updater_via_factory_routes_to_hll_accumulator() {

asap-query-engine/src/precompute_operators/hll_accumulator.rs

@@ -1,25 +1,3 @@
-//! HyperLogLog accumulator for `COUNT(DISTINCT col)` queries.
-//!
-//! Wraps `asap_sketchlib::HllSketch` (precision-14 `HllVariant::Regular` by default)
-//! and exposes it through the precompute-engine accumulator traits so the streaming
-//! worker can feed values into it and the query path can dispatch `Cardinality`
-//! against the resulting sketch.
-//!
-//! Mirrors the `DatasketchesKLLAccumulator` layout: a single subpopulation per
-//! grouping key, msgpack-encoded byte serialization (round-trippable, unlike the
-//! lossy `datafusion_summary_library::physical::hll::HllSketch::to_bytes`).
-//!
-//! End-to-end pipeline for `COUNT(DISTINCT col)`:
-//! 1. SQL parser normalises the call to `name="CARDINALITY", value_column=col`.
-//! 2. `AggregationOperator::Cardinality.to_statistics()` → `[Statistic::Cardinality]`.
-//! 3. Capability matching pairs the query with an `AggregationType::HLL` config.
-//! 4. The precompute worker feeds `col`'s f64 samples through
-//!    `HllAccumulatorUpdater::update_single`, which calls `HllAccumulator::update`.
-//! 5. Query path: `plan_builder` maps `Statistic::Cardinality → InferOperation::CountDistinct`;
-//!    `SummaryInferExec` deserializes the stored sketch and calls
-//!    `SingleSubpopulationAggregate::query(Cardinality)`, which returns
-//!    `HllSketch::estimate()`.
-
 use std::collections::HashMap;
 
 use asap_sketchlib::sketches::hll::{HllSketch, HllVariant};
@@ -33,15 +11,12 @@ use crate::data_model::{
     SingleSubpopulationAggregate,
 };
 
-/// Default precision when none is supplied via streaming-config parameters.
-/// Matches `datafusion_summary_library::physical::hll::HllSketch` (~0.8% std error,
-/// ~16 KiB per sketch).
+/// Default HLL precision when streaming config omits `parameters.precision`.
 pub const DEFAULT_HLL_PRECISION: u32 = 14;
 
 /// HLL sketch accumulator — wraps `asap_sketchlib::HllSketch`.
 /// Core insert/merge/serde logic lives in `asap_sketchlib`; this file retains
-/// the QE-specific trait impls (`AggregateCore`, `SingleSubpopulationAggregate`,
-/// `SerializableToSink`, `MergeableAccumulator`).
+/// QE-specific trait impls.
 #[derive(Debug, Clone)]
 pub struct HllAccumulator {
     pub inner: HllSketch,
@@ -58,19 +33,10 @@ impl HllAccumulator {
         Self::new(DEFAULT_HLL_PRECISION)
     }
 
-    /// Feed a value into the sketch. The streaming layer surfaces all column
-    /// values as `f64`; we hash their little-endian bytes via `HllSketch::update`,
-    /// which goes through the canonical sketchlib hash (`hash64_seeded` with
-    /// `CANONICAL_HASH_SEED`). That makes the on-disk sketch byte-identical to
-    /// what `sketchlib-go` would produce for the same stream — cross-language
-    /// parity is locked in by upstream's golden-byte test.
     pub fn update(&mut self, value: f64) {
         self.inner.update(&value.to_le_bytes());
     }
 
-    /// Current cardinality estimate. Returned as `f64` so callers don't lose the
-    /// fractional small-range correction; the engine truncates to an integer in
-    /// the final result column when needed.
     pub fn estimate(&self) -> f64 {
         self.inner.estimate()
     }
@@ -96,9 +62,6 @@ impl Default for HllAccumulator {
 
 impl SerializableToSink for HllAccumulator {
     fn serialize_to_json(&self) -> Value {
-        // Mirrors KLL's JSON shape: opaque base64 of the canonical msgpack body so
-        // downstream consumers can round-trip through any serde-capable channel
-        // without losing the register array.
         let bytes = self.inner.serialize_msgpack().unwrap_or_default();
         let b64 = general_purpose::STANDARD.encode(&bytes);
         serde_json::json!({
@@ -152,8 +115,6 @@ impl AggregateCore for HllAccumulator {
     }
 
     fn get_keys(&self) -> Option<Vec<crate::KeyByLabelValues>> {
-        // HLL is a probabilistic counter, not a set: it estimates |S| without
-        // retaining the individual members of S. There are no keys to enumerate.
         None
     }
 
@@ -163,8 +124,6 @@ impl AggregateCore for HllAccumulator {
         _key: &Option<crate::KeyByLabelValues>,
         query_kwargs: &HashMap<String, String>,
     ) -> Result<f64, Box<dyn std::error::Error + Send + Sync>> {
-        // HLL is a single-population accumulator (one sketch per grouping key
-        // partition), so `key` is unused — same shape as KllAccumulator.
         SingleSubpopulationAggregate::query(self, statistic, Some(query_kwargs))
     }
 }