Skip to content

Commit 6826c52

Browse files
fix(planner): sync Python subpopulation logic, drop redundant heap override
does_precompute_operator_support_subpopulations in the Python copy of this function still returned False for CountMinSketchWithHeap + TOPK with a stale comment, the same bug the Rust copy fixed for planner/SQL in a prior commit. Update it to return True, matching Rust (the heap is self-keyed but still tracks many keys internally, like MultipleSum). Also remove the manual grouping/aggregated-label override for SQL top-k in planner/sql.rs: build_agg_configs_for_statistics already produces the same split via set_subpopulation_labels now that does_precompute_operator_support_subpopulations returns true for this case, so the override was dead code. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V66Z7Fge2EYW2N1CDJzrxk
1 parent 0e0f2c5 commit 6826c52

2 files changed

Lines changed: 7 additions & 15 deletions

File tree

  • asap-common/dependencies/py/promql_utilities/promql_utilities/query_logics
  • asap-planner-rs/src/planner

‎asap-common/dependencies/py/promql_utilities/promql_utilities/query_logics/logics.py‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -97,9 +97,12 @@ def does_precompute_operator_support_subpopulations(
9797
elif (
9898
precompute_operator == "CountMinSketchWithHeap" and statistic == Statistic.TOPK
9999
):
100-
# topk and bottomk do not support subpopulations!
101-
# other usages of CountMinSketchWithHeap will fall through.
102-
return False
100+
# CountMinSketchWithHeap (topk) is a heavy-hitters sketch: one heap
101+
# instance tracks many keys internally (like MultipleSum), it just
102+
# doesn't need an *external* paired key aggregation to enumerate them
103+
# -- it discovers its own top-k keys. So it supports subpopulations
104+
# the same way MultipleSum does.
105+
return True
103106
# elif precompute_operator == "UnivMon":
104107
# return statistic in ["sum", "count", "avg"]
105108
else:

‎asap-planner-rs/src/planner/sql.rs‎

Lines changed: 1 addition & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -141,7 +141,7 @@ impl SQLSingleQueryProcessor {
141141
let topk_k = sql_topk.map(|t| t.k);
142142
let topk_count_events = sql_topk.map(|t| t.count_events());
143143

144-
let mut configs = build_agg_configs_for_statistics(
144+
let configs = build_agg_configs_for_statistics(
145145
&statistics,
146146
treatment_type,
147147
&spatial_output,
@@ -163,17 +163,6 @@ impl SQLSingleQueryProcessor {
163163
)
164164
.map_err(ControllerError::SqlParse)?;
165165

166-
if sql_topk.is_some() {
167-
for cfg in &mut configs {
168-
if cfg.aggregation_type == AggregationType::CountMinSketchWithHeap {
169-
// Heap-only self-keyed layout: the GROUP BY column is tracked
170-
// inside the sketch's aggregated dimension, not as a partition key.
171-
cfg.grouping_labels = KeyByLabelNames::empty();
172-
cfg.aggregated_labels = spatial_output.clone();
173-
}
174-
}
175-
}
176-
177166
// SQLPatternParser always produces second-based durations; convert to ms.
178167
// For a single-scrape-interval query this equals data_ingestion_interval_ms
179168
// by construction (the matcher's classification boundary), so this is a

0 commit comments

Comments
 (0)