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
When running query_engine_rust (precompute engine) against configs generated by asap-planner, any workload that produces approximate aggregations (sum / count / avg, including avg_over_time and ... by (label)) fails at ingest time. Two distinct defects in asap-query-engine/src/precompute_engine/accumulator_factory.rs are involved, both rooted in the factory not faithfully handling the aggregation types/parameters the planner emits — and a _ => default to Sum catch-all that turned them into warnings / wrong results instead of loud failures.
Mode: standalone query_engine_rust --config-file <engine_config.yaml> with ingest.type: http_remote_write, Prometheus backend
Steps to reproduce
# 1. Plan a workload with approximate aggregations, e.g.:# avg(metric{...})# sum by(label)(metric{...})# avg_over_time(metric{...}[5m])
asap-planner --input_config workload.yaml --output_dir plan \
--streaming_engine precompute --prometheus_scrape_interval 15
# -> plan/streaming_config.yaml now contains CountMinSketch + DeltaSetAggregator entries# 2. Run the engine on the generated configs
query_engine_rust --config-file engine_config.yaml
# (streaming_config: plan/streaming_config.yaml)# 3. remote_write samples for the planned metrics (e.g. fake_remote_writer --from-config)
A CountMinSketch aggregation, exactly as the planner emits it:
WARN ... worker: error processing group (21, ): CMS config missing required parameter: row_num
WARN ... Unknown aggregation_type 'DeltaSetAggregator', defaulting to SingleSubpopulation Sum
Expected
The engine builds the correct sketch for every aggregation the planner emits; CountMinSketch and its companion DeltaSetAggregator ingest cleanly and serve approximate sum/count/avg queries.
cms_params() (used by CountMinSketch and HydraKLL) only reads row_num/col_num, but the planner (asap-planner-rs/src/planner/sketch.rs) emits depth/width for CountMinSketch. So every plain-CMS aggregation throws CMS config missing required parameter: row_num on its first sample and never ingests.
cms_heap_params() (for CountMinSketchWithHeap) already accepts the depth/width aliases, so it works.
HydraKLL works only because the planner happens to emit row_num/col_num for it.
create_accumulator_updater() had no arm for DeltaSetAggregator or SetAggregator; they fell into the _ => default to Sum catch-all, which logged a warning and built a SumAccumulator.
The planner emits a DeltaSetAggregatorcompanion with every CountMinSketch to record the set of subpopulation keys the CMS was fed (a CMS is a point-query structure with no key list of its own; the read path needs this set to enumerate per-key results). Built as a Sum, the key set is lost.
Impact: approximate keyed sum/count queries return wrong/empty results. Only a warning is logged, so it’s easy to miss.
Underlying issue
The _ => default to Sum catch-all in create_accumulator_updater() masked unhandled aggregation types as warnings + wrong results rather than failing loudly. An exhaustive match would have surfaced both defects as compile errors.
Proposed fix
cms_params(): accept depth/width first, then row_num/col_num (mirror cms_heap_params()).
Wire DeltaSetAggregator → new DeltaSetAggregatorUpdater and SetAggregator → new SetAggregatorUpdater (keyed updaters that add_key the aggregated-label key; value ignored). Add both to config_is_keyed().
Remove the _ => default to Sum catch-all so the match is exhaustive over AggregationType (future unhandled types become compile errors, not silent degradation).
Tests: CMS depth/width parses; row_num/col_num still works; DeltaSetAggregator builds the right accumulator and records distinct keys; config_is_keyed() ↔ is_keyed() consistency for the new types.
Summary
When running
query_engine_rust(precompute engine) against configs generated byasap-planner, any workload that produces approximate aggregations (sum/count/avg, includingavg_over_timeand... by (label)) fails at ingest time. Two distinct defects inasap-query-engine/src/precompute_engine/accumulator_factory.rsare involved, both rooted in the factory not faithfully handling the aggregation types/parameters the planner emits — and a_ => default to Sumcatch-all that turned them into warnings / wrong results instead of loud failures.Environment
asap-query-engine(precompute engine),src/precompute_engine/accumulator_factory.rsasap-planner(--streaming_engine precompute)query_engine_rust --config-file <engine_config.yaml>withingest.type: http_remote_write, Prometheus backendSteps to reproduce
A
CountMinSketchaggregation, exactly as the planner emits it:Observed (actual)
Expected
The engine builds the correct sketch for every aggregation the planner emits;
CountMinSketchand its companionDeltaSetAggregatoringest cleanly and serve approximatesum/count/avgqueries.Defect 1 —
CountMinSketchparameter-name mismatch (hard error)cms_params()(used byCountMinSketchandHydraKLL) only readsrow_num/col_num, but the planner (asap-planner-rs/src/planner/sketch.rs) emitsdepth/widthforCountMinSketch. So every plain-CMS aggregation throwsCMS config missing required parameter: row_numon its first sample and never ingests.cms_heap_params()(forCountMinSketchWithHeap) already accepts thedepth/widthaliases, so it works.HydraKLLworks only because the planner happens to emitrow_num/col_numfor it.depth/widthalias on this path.Impact: all
CountMinSketchaggregations fail to ingest → approximatesum/count(andavg,avg_over_time) queries are not served.Defect 2 —
DeltaSetAggregator/SetAggregatornot wired (silent wrong result)create_accumulator_updater()had no arm forDeltaSetAggregatororSetAggregator; they fell into the_ => default to Sumcatch-all, which logged a warning and built aSumAccumulator.The planner emits a
DeltaSetAggregatorcompanion with everyCountMinSketchto record the set of subpopulation keys the CMS was fed (a CMS is a point-query structure with no key list of its own; the read path needs this set to enumerate per-key results). Built as aSum, the key set is lost.Impact: approximate keyed
sum/countqueries return wrong/empty results. Only a warning is logged, so it’s easy to miss.Underlying issue
The
_ => default to Sumcatch-all increate_accumulator_updater()masked unhandled aggregation types as warnings + wrong results rather than failing loudly. An exhaustivematchwould have surfaced both defects as compile errors.Proposed fix
cms_params(): acceptdepth/widthfirst, thenrow_num/col_num(mirrorcms_heap_params()).DeltaSetAggregator→ newDeltaSetAggregatorUpdaterandSetAggregator→ newSetAggregatorUpdater(keyed updaters thatadd_keythe aggregated-label key; value ignored). Add both toconfig_is_keyed()._ => default to Sumcatch-all so thematchis exhaustive overAggregationType(future unhandled types become compile errors, not silent degradation).depth/widthparses;row_num/col_numstill works;DeltaSetAggregatorbuilds the right accumulator and records distinct keys;config_is_keyed()↔is_keyed()consistency for the new types.Affected files
asap-query-engine/src/precompute_engine/accumulator_factory.rs(fix)asap-planner-rs/src/planner/sketch.rs(producer side, for the parameter-naming context)