refactor: unify merge resolution via bloom-filtered iterator pipeline#69
Merged
polaz merged 10 commits intoMar 22, 2026
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefactors point-read merge resolution to use a bloom-filtered single-key iterator pipeline with optional key-hash prefilter, adds Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Tree as Tree
participant Pipeline as Iterator\n(IterState)
participant Bloom as BloomFilter
participant Table as Table/Memtable
participant Stream as MvccStream\n(Merger)
Client->>Tree: get(key, seqno)
Tree->>Pipeline: build single-key range\nset IterState.key_hash = hash(key)
Pipeline->>Bloom: bloom_may_contain_key_hash(hash)
Bloom-->>Pipeline: pass / skip
Pipeline->>Table: open iterators for active/sealed + bloom-passing tables
Table-->>Stream: emit entries for key
Stream->>Stream: resolve merges / apply RT & indirection rules
Stream-->>Tree: resolved value / None / error
Tree-->>Client: return result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
This PR refactors point-read merge resolution to reuse the existing Merger → MvccStream iterator pipeline (via a single-key key..=key range), while adding bloom-based pre-filtering so point reads can skip irrelevant SSTs without duplicating merge logic.
Changes:
- Replace the hand-rolled point-read merge collector with
resolve_merge_via_pipeline()backed byTreeIter/MvccStream. - Add a standard bloom key-check API on
Tableand thread akey_hashthrough iterator state to enable bloom skipping for single-key pipelines. - Remove the now-unused memtable helper (
get_all_for_key) and its tests; update test comments to reference the new function.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
src/table/mod.rs |
Factors bloom checking into a hash-based helper and exposes bloom_may_contain_key() for point-read pre-filtering. |
src/range.rs |
Extends IterState with key_hash and centralizes bloom filtering via bloom_passes() for both prefix and key-based skips. |
src/tree/mod.rs |
Switches point-read merge resolution to the iterator pipeline (resolve_merge_via_pipeline) and updates merge-operator plumbing accordingly. |
src/memtable/mod.rs |
Removes get_all_for_key() and its unit tests, which were only used by the old point-read merge path. |
tests/merge_operator.rs |
Updates comments referring to the old merge-resolution function name. |
polaz
force-pushed
the
feat/#46-refactor-unify-merge-resolution-via-bloom-filtered
branch
from
cec697b to
feb7585
Compare
Member
Author
|
@coderabbitai re-review |
|
🧠 Learnings used✅ Actions performedFull review triggered. |
polaz
force-pushed
the
feat/#46-refactor-unify-merge-resolution-via-bloom-filtered
branch
2 times, most recently
from
58fcb1a to
b53720a
Compare
Member
Author
|
@coderabbitai re-review |
|
🧠 Learnings used✅ Actions performedFull review triggered. |
Member
Author
|
@coderabbitai re-review |
|
🧠 Learnings used✅ Actions performedFull review triggered. |
polaz
force-pushed
the
feat/#46-refactor-unify-merge-resolution-via-bloom-filtered
branch
from
661ea79 to
17131f7
Compare
Member
Author
|
@coderabbitai re-review |
|
🧠 Learnings used✅ Actions performedFull review triggered. |
- Remove stale line number reference from merge_rt test comment - Reword prefix bloom test docstring to describe correctness validation independent of bloom filter behavior - Add comment explaining why multi-table runs skip bloom filtering
- Remove brittle line number reference from merge_tombstone test - Add code comment explaining intentional TreeIter reuse in resolve_merge_via_pipeline (bloom pre-filter + shared logic)
- Broaden multi-table run bloom guard to trigger on key_hash too (was prefix_hash-only, so point-read merges skipped bloom pruning) - Replace inline prefix bloom check with bloom_passes() for consistent prefix+key bloom handling and metrics - Soften test comments to not assert definite bloom behavior - Rename prefix bloom test to describe correctness scenario
polaz
force-pushed
the
feat/#46-refactor-unify-merge-resolution-via-bloom-filtered
branch
from
2f5304f to
56aafdb
Compare
![sw-release-bot[bot]](https://avatars.githubusercontent.com/u/228206483?s=60&v=4){kind=small}
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark 'lsm-tree db_bench'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.15.
| Benchmark suite | Current: 3d97a40 | Previous: b9c6897 | Ratio |
|---|---|---|---|
fillrandom |
875131.7860956681 ops/sec |
1078840.3879110864 ops/sec |
1.23 |
readrandom |
550753.7447771127 ops/sec |
655440.3305655628 ops/sec |
1.19 |
overwrite |
740668.2265781432 ops/sec |
929551.8754039077 ops/sec |
1.26 |
This comment was automatically generated by workflow using github-action-benchmark.
CC: @polaz
Member
Author
|
@coderabbitai re-review |
|
🧠 Learnings used✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/tree/mod.rs (1)
776-816: Please attach the deep-tree point-read benchmark before merge.This rewires the hot merge point-read path, and the PR acceptance criteria still call for staying within roughly 5% latency on a 100-table tree. The semantic coverage looks solid; the missing release signal here is the benchmark data.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tree/mod.rs` around lines 776 - 816, Before merging, add and attach the deep-tree point-read benchmark results that measure latency on a ~100-table tree for the new hot-path in resolve_merge_via_pipeline (the TreeIter-based point-read/merge path). Run the existing benchmark harness (the deep-tree point-read benchmark used in previous PRs) against the rewritten path (resolve_merge_via_pipeline) and include the before/after latency numbers and configuration (table count, data size, seqno, merge_operator used) in the PR so reviewers can verify you stayed within the ~5% latency budget.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/tree/mod.rs`:
- Around line 776-816: Before merging, add and attach the deep-tree point-read
benchmark results that measure latency on a ~100-table tree for the new hot-path
in resolve_merge_via_pipeline (the TreeIter-based point-read/merge path). Run
the existing benchmark harness (the deep-tree point-read benchmark used in
previous PRs) against the rewritten path (resolve_merge_via_pipeline) and
include the before/after latency numbers and configuration (table count, data
size, seqno, merge_operator used) in the PR so reviewers can verify you stayed
within the ~5% latency budget.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ef11b0e0-045a-49ef-871a-68b9173f069a
📒 Files selected for processing (6)
src/memtable/mod.rssrc/range.rssrc/table/mod.rssrc/tree/mod.rstests/merge_operator.rstests/tree_prefix_bloom.rs
💤 Files with no reviewable changes (1)
- src/memtable/mod.rs
5 tasks
Criterion benchmark measuring point-read merge resolution latency on trees with 10/50/100 L0 tables, both cached and uncached. Exercises the bloom-filtered iterator pipeline.
Member
Author
Benchmark: merge point-read on deep L0
Setup: 1 base value + N-1 unrelated tables (bloom rejects) + 1 merge operand in memtable. CounterMerge operator. macOS ARM64.
Key observations:
Benchmark added in commit 4836e82 ( |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
benches/merge_point_read.rs (1)
35-112: Extract shared tree-population logic for cached/uncached cases.Both branches duplicate insert/flush/merge setup. A small helper (parameterized by cache size) would reduce drift and make benchmark intent easier to maintain.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@benches/merge_point_read.rs` around lines 35 - 112, Duplicate setup for populating trees (in merge_point_read.rs using variables tree and tree_cached and their insert/flush/merge flows) should be extracted into a single helper function (e.g., build_populated_tree or populate_tree) that accepts parameters for cache size (or Cache::with_capacity_bytes arg), table_count and initial seqno and returns the opened tree and the final sequence number; replace both uncached and cached blocks with calls to this helper and use its returned seqno for the get/merge checks to eliminate duplicated insert/flush/merge logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@benches/merge_point_read.rs`:
- Around line 37-47: Multiple calls in the benchmark setup (tempdir(),
Config::new(...) .use_cache(...).with_merge_operator(...).open(), and other
operations referenced in this diff) use unwrap() on fallible filesystem and tree
operations which can panic; change these to fallible handling by returning
Result from the benchmark helper and using the ? operator (or explicit match/if
let error handling) so errors propagate to the harness boundary, and update the
caller to handle the Result; specifically replace tempdir().unwrap(), the
Config::new(...).open().unwrap(), and the other listed unwrap() sites with ? (or
proper error branching) and ensure functions like the setup helper return
Result<..., E> so the harness can handle/report I/O/runtime errors instead of
panicking.
---
Nitpick comments:
In `@benches/merge_point_read.rs`:
- Around line 35-112: Duplicate setup for populating trees (in
merge_point_read.rs using variables tree and tree_cached and their
insert/flush/merge flows) should be extracted into a single helper function
(e.g., build_populated_tree or populate_tree) that accepts parameters for cache
size (or Cache::with_capacity_bytes arg), table_count and initial seqno and
returns the opened tree and the final sequence number; replace both uncached and
cached blocks with calls to this helper and use its returned seqno for the
get/merge checks to eliminate duplicated insert/flush/merge logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9a481a27-0b5f-43e4-b36d-3c7914474811
📒 Files selected for processing (2)
Cargo.tomlbenches/merge_point_read.rs
✅ Files skipped from review due to trivial changes (1)
- Cargo.toml
polaz
deleted the
feat/#46-refactor-unify-merge-resolution-via-bloom-filtered
branch
Closed
This was referenced Mar 22, 2026
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
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
resolve_merge_get()withresolve_merge_via_pipeline()that reusesMerger → MvccStreamon akey..=keyrangeTable::bloom_may_contain_key_hash()) to skip many disk tables for point readsNet -143 lines — merge resolution now lives in one place (
MvccStream).Changes
table/mod.rsbloom_may_contain_hash()base, addbloom_may_contain_key_hash()range.rskey_hashtoIterState,bloom_passes()helper for unified prefix+key bloomtree/mod.rsresolve_merge_via_pipeline()replaces ~150-lineresolve_merge_get()memtable/mod.rsget_all_for_key()and its teststests/merge_operator.rsTest plan
cargo check— 0 warnings, 0 errorscargo test— 757 passed, 0 failedCloses #46
Summary by CodeRabbit
Improvements
Refactor
Tests
Chores