refactor: eliminate code duplication in precompute engine + bench_precompute_sketch binary#233
Open
zzylol wants to merge 4 commits intopr/E-correctness-tests-comparisonfrom
Open
refactor: eliminate code duplication in precompute engine + bench_precompute_sketch binary#233zzylol wants to merge 4 commits intopr/E-correctness-tests-comparisonfrom
zzylol wants to merge 4 commits intopr/E-correctness-tests-comparisonfrom
Conversation
accumulator_factory.rs: - Add impl_clone_accumulator_methods! macro to replace 16 identical take_accumulator/snapshot_accumulator bodies across 8 updater structs - Add config_is_keyed() free fn for static keyed check without allocating an updater (replaces throwaway-updater pattern used 4+ times) - Add kll_k_param(), cms_params(), hydra_kll_params() helpers to eliminate 4 copy-pasted parameter extraction blocks in the factory - Fix latent bug: SingleSubpopulation/KLL arm now checks "K" (capital) before "k", consistent with all other arms - Add tests: test_config_is_keyed, test_kll_k_param_capital_k worker.rs: - AggregationState.config: AggregationConfig -> Arc<AggregationConfig> eliminating N×M deep config copies across series×aggregations - Worker.agg_configs and Worker::new param updated to Arc map; matching_agg_configs returns owned Arc clones (removes lifetime tie) - Add apply_sample() free fn replacing 3 copies of is_keyed dispatch - Add merge_panes_for_window() free fn replacing identical ~40-line pane-merge loop in both process_samples and flush_all - Fix ForwardToStore path: is_keyed() and extract_key_from_series() each called once instead of twice engine.rs: - Wrap aggregation configs in Arc once at engine startup; per-worker agg_configs.clone() is now N pointer bumps, not N full deep copies Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Rewrites the "Optional second-tier merge workers" section to fix seven correctness issues identified in review: 1. Deadlock (critical): workers with no matching series never emitted WindowCompletion. Fix: introduce per_agg_watermark per worker, advanced by both data arrival and the flush timer (wall-clock floor), so every worker emits WindowCompletion for every aggregation on every flush cycle regardless of whether it has matching series. 2. WindowCompletion routing (critical): routing partials by (agg_id, key, window) while routing key-less completions by (agg_id, window) required an O(M) broadcast of completions to all merge workers. Fix: route ALL messages — partials and completions — by hash(agg_id, window_start, window_end) only, co-locating all keys for a window on one merge worker and eliminating the broadcast. 3. Conflicting struct definitions (critical): two contradictory WindowCompletion structs were present (one with key, one without). Fix: adopt a single key-less definition; explain the rationale. 4. Merge-tier watermark undefined (significant): PendingWindowState had no eviction bound. Fix: define merge_watermark as the min of worker_watermark_ms values received via completions; add force-eviction after eviction_grace_period_ms for lagging workers. 5. Late data re-finalization (significant): "re-open the merged result" was unspecified after a window was already written to the store. Fix: late corrections remain store appends with query-time merge (same as non-merge-tier path); canonical finalized output is never mutated. 6. Message ordering (minor): partials and completions sent through separate channels had no ordering guarantee. Fix: both go through the same MPSC channel from a given first-tier worker; partials always enqueued before the completion for the same window. 7. State structure (minor): per-key completed_sources tracking was incompatible with key-less completions (required O(keys) scan). Fix: split state into window_completions (window-level) and pending_partials (key-level) maps; completion check is O(1), finalization scan is O(keys per window) done once at close. Also updates the rollout strategy to note that per_agg_watermark and flush-timer-driven WindowCompletion emission must be deployed in first-tier workers before any merge worker is started. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…arking Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
zzylol
force-pushed
the
pr/F-refactor-bench-binary
branch
from
2372141 to
decbcc8
Compare
zzylol
force-pushed
the
pr/E-correctness-tests-comparison
branch
from
8c04bc5 to
47d279e
Compare
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
bench_precompute_sketchbinary for standalone precompute engine benchmarkinge2e_precompute_equivalenceintests/This is PR F of 6 stacked PRs splitting #162
Stacking order:
Test plan
cargo build --bin bench_precompute_sketchsucceedscargo testintegration test intests/e2e_precompute_equivalence.rspasses🤖 Generated with Claude Code