fix(query-engine): give range-query merges the CMS/KLL batch fast path - #614
Merged
Conversation
#596) NaiveMerger::merge_all (range path) only did a sequential merge_with fold, never the merge_multiple batch fast path that SimpleEngine::merge_accumulators (instant path) already had for CountMinSketch/DatasketchesKLL. The two folds also disagreed on error handling: SimpleEngine's warned and silently kept the partial result, NaiveMerger's aborted via `?`. Extract the shared "try batch merge, else fold" logic into engines::merge_utils::merge_accumulators_batch and have both callers delegate to it, so both now take the fast path and both abort on the first merge_with failure instead of one of them dropping data silently. Adds AccumulatorError::MergeFailed for the new abort case. Co-Authored-By: Claude Sonnet 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01WDS34V2EexpbihCfH9SDWh
… test fixtures Two follow-ups from review of the #596 fix: - merge_accumulators_batch always cloned the first accumulator, even for the common single-bucket case, regressing the move-based shortcut SimpleEngine::merge_accumulators had before this refactor. Restore it at the call site (NaiveMerger can't do the same since it merges from a borrowed Vec it keeps for the next slide()). - The new regression tests duplicated PoisonableAccumulator, cms_from_matrix, and oracle_sequential_fold verbatim between window_merger.rs and simple_engine/mod.rs; moved them into tests::test_utilities::merge_fixtures, shared by both. Co-Authored-By: Claude Sonnet 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01WDS34V2EexpbihCfH9SDWh
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
NaiveMerger::merge_all(range path) never took themerge_multiplebatch fast path thatSimpleEngine::merge_accumulators(instant path) already had forCountMinSketch/DatasketchesKLL, so every range query merging those bucket types was slower than the equivalent instant query.merge_withfailure, the range path aborted via?.engines::merge_utils::merge_accumulators_batch; both callers now delegate to it, so both take the fast path and both abort on the firstmerge_withfailure (no more silent partial merges). AddedAccumulatorError::MergeFailedfor the new abort case on the instant path.merge_precomputed_outputs) and per-timestamp (range step loop) catch-and-skip-with-warn behavior is unchanged.Test plan
cargo test --lib— 598 passed, 0 failed, 5 ignoredcargo clippy --lib --tests -- -D warnings— cleancargo fmt --check— cleanCloses #596
🤖 Generated with Claude Code
https://claude.ai/code/session_01WDS34V2EexpbihCfH9SDWh