`SimpleEngine::merge_accumulators` (`asap-query-engine/src/engines/simple_engine/mod.rs`, used only by the instant-query path via `merge_precomputed_outputs`) tries an optimized batch merge for `DatasketchesKLL` and `CountMinSketch` accumulators (`DatasketchesKLLAccumulator::merge_multiple` / `CountMinSketchAccumulator::merge_multiple`) before falling back to a sequential `merge_with` fold.
`NaiveMerger::merge_all` (`asap-query-engine/src/engines/window_merger.rs`) — the merger the range query path uses for values (always) and, as of #583, for keys too — only does the sequential fallback fold. It never takes the batch-merge fast path, regardless of accumulator type. `create_window_merger` always returns `NaiveMerger` regardless of its `accumulator_type` argument; the doc comment on the module even names `IncrementalMerger`/other variants as intended future work that was never built.
Net effect: every range query merging `CountMinSketch` or `DatasketchesKLL` buckets takes a slower path than the identical instant query.
Investigated during #583's follow-up cleanup whether this could be a risk-free mechanical dedup instead of a real behavior/perf change, and found it can't be done for free: the two fallback folds also differ in error handling, not just in whether they have a fast path.
- `SimpleEngine::merge_accumulators`'s fold: on a `merge_with` failure, `warn!`s and keeps the existing result, continuing the loop (drops that one bucket's contribution silently).
- `NaiveMerger::merge_all`'s fold: on a `merge_with` failure, propagates the error immediately via `?`, aborting the whole merge.
So closing this needs two decisions, not one: (1) should `NaiveMerger` gain the CMS/KLL fast path (the actual perf fix), and (2) should the two fold implementations' error policies be unified, and if so which one wins. Neither is a mechanical refactor — deliberately left out of #583's cleanup scope.
Related: #581 (broader instant/range fetch-merge unification) — this is a sharper, narrower instance of the same drift.
`SimpleEngine::merge_accumulators` (`asap-query-engine/src/engines/simple_engine/mod.rs`, used only by the instant-query path via `merge_precomputed_outputs`) tries an optimized batch merge for `DatasketchesKLL` and `CountMinSketch` accumulators (`DatasketchesKLLAccumulator::merge_multiple` / `CountMinSketchAccumulator::merge_multiple`) before falling back to a sequential `merge_with` fold.
`NaiveMerger::merge_all` (`asap-query-engine/src/engines/window_merger.rs`) — the merger the range query path uses for values (always) and, as of #583, for keys too — only does the sequential fallback fold. It never takes the batch-merge fast path, regardless of accumulator type. `create_window_merger` always returns `NaiveMerger` regardless of its `accumulator_type` argument; the doc comment on the module even names `IncrementalMerger`/other variants as intended future work that was never built.
Net effect: every range query merging `CountMinSketch` or `DatasketchesKLL` buckets takes a slower path than the identical instant query.
Investigated during #583's follow-up cleanup whether this could be a risk-free mechanical dedup instead of a real behavior/perf change, and found it can't be done for free: the two fallback folds also differ in error handling, not just in whether they have a fast path.
So closing this needs two decisions, not one: (1) should `NaiveMerger` gain the CMS/KLL fast path (the actual perf fix), and (2) should the two fold implementations' error policies be unified, and if so which one wins. Neither is a mechanical refactor — deliberately left out of #583's cleanup scope.
Related: #581 (broader instant/range fetch-merge unification) — this is a sharper, narrower instance of the same drift.