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
{{ message }}
Repository navigation
Range query merges lack the CMS/KLL batch-merge fast path instant queries get #596
`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.
Activity
refactor(query-engine): fix stale doc comment, dedup bucket_map/scan_…
fix(query-engine): range queries expand keys_query per output step (#595
fix(query-engine): restore move-based single-accumulator path, dedupe…
fix(query-engine): give range-query merges the CMS/KLL batch fast path (