Precompute factory mis-builds DeltaSetAggregator and SetAggregator as Sum

[zaoxing]

Summary

The precompute engine still mishandles DeltaSetAggregator and SetAggregator configurations generated by the planner. Both aggregation types are implemented in the query engine, but create_accumulator_updater() does not route them to their corresponding keyed accumulators. They therefore fall through to the generic fallback and are built as SumAccumulatorUpdater instances.

The planner emits a DeltaSetAggregator companion for CountMinSketch plans so the query path can enumerate the subpopulation keys represented in the sketch. Losing that key set causes approximate keyed sum/count/avg results to be wrong or empty.

Environment

• Component: asap-query-engine precompute engine
• Affected code: asap-query-engine/src/precompute_engine/accumulator_factory.rs
• Producer: asap-planner with --streaming_engine precompute
• Mode: standalone query_engine_rust --config-file <engine_config.yaml> with remote-write ingest

Steps to reproduce

1. Plan a workload with an approximate keyed aggregation, for example:

   sum by (label) (metric)
   

2. Inspect the generated streaming configuration. It contains a CountMinSketch and a companion DeltaSetAggregator entry.

3. Run query_engine_rust with that configuration and ingest samples for the planned metric.

4. Query the approximate keyed aggregation.

Actual behavior

The DeltaSetAggregator/SetAggregator configuration is not recognized by the accumulator factory. It falls through to the generic fallback and constructs a sum updater, so the key-tracking accumulator is never populated. A warning is emitted, but ingest continues with incorrect semantics.

config_is_keyed() also does not classify these aggregation types as keyed, which is inconsistent with the accumulator implementations and makes the worker route them incorrectly.

Expected behavior

The factory should construct the correct keyed accumulator for DeltaSetAggregator and SetAggregator, and config_is_keyed() should agree with the resulting updater's is_keyed() value. Approximate keyed sum/count/avg queries should retain and enumerate the planned subpopulation keys correctly.

Proposed fix

• Add a DeltaSetAggregatorUpdater that records keys in DeltaSetAggregatorAccumulator.
• Add a SetAggregatorUpdater that records keys in SetAggregatorAccumulator.
• Add both aggregation types to config_is_keyed().
• Remove the generic silent fallback for unhandled aggregation types; see the triage comment below.
• Add factory tests covering accumulator type, distinct key tracking, and config_is_keyed()/is_keyed() consistency.

Related work

The separate CountMinSketch depth/width parameter mismatch from the original report was fixed by #483. It is no longer part of this issue.

Affected files

• asap-query-engine/src/precompute_engine/accumulator_factory.rs
• asap-query-engine/src/precompute_operators/delta_set_aggregator_accumulator.rs
• asap-query-engine/src/precompute_operators/set_aggregator_accumulator.rs

[milindsrivastava1997]

#483 opened for defect 1

[milindsrivastava1997]

This was generated by AI during triage.

One implementation constraint is important here: we should not retain a generic _ => default to Sum fallback in create_accumulator_updater().

An unhandled AggregationType has different semantics from Sum; silently constructing a sum updater produces incorrect data while only emitting a warning. The factory should either use an exhaustive match so newly added variants fail at compile time, or return a loud configuration error for unsupported types. This also prevents future planner/query-engine aggregation types from being silently degraded.