Repository navigation
feat(sort): add fgumi merge command with loser tree - #186
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #186 +/- ##
==========================================
+ Coverage 85.65% 86.01% +0.36%
==========================================
Files 128 110 -18
Lines 51982 51737 -245
==========================================
- Hits 44524 44502 -22
+ Misses 7458 7235 -223 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughAdds a new 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/commands/merge.rs`:
- Around line 119-121: The output path may alias an input and get truncated
during writing; update the merge command to compare the resolved output path
(self.output) against each resolved input path after validation and return an
error if any match. Specifically, after calling validate_file_exists on
input_paths, canonicalize or otherwise resolve both self.output and each input
path and check equality, and if any input equals the resolved output, bail out
with a clear error (referencing self.output and input_paths/validate_file_exists
in the merge.rs loop).
- Around line 191-205: The code currently merges read-groups/programs from each
header without checking `@SQ` reference dictionaries; add validation so the first
header's reference sequence list is captured (e.g., save the first header's
reference dictionary from the Header returned by create_bam_reader) and for
every subsequent header compare its `@SQ` entries and order against that saved
dictionary, returning an error (or aborting the merge) if they differ; implement
this check inside the input_paths loop before mutating builder (around the
create_bam_reader(...) usage), and keep using the existing symbols
(create_bam_reader, header, builder, rg_ids, pg_ids) so you only proceed to
add_read_group/add_program when the reference dictionaries match.
In `@src/lib/sort/raw.rs`:
- Around line 698-700: The code currently calls create_output_header(header)
which rebuilds the `@HD` and drops fields from the merged header; instead preserve
the merged header's `@HD` by starting from header.header() (the result of
merge_headers) and only overwrite/update sort-order fields (e.g., SO) there
before using it as output_header; update the call site in
raw::open_bam_prefetch_readers / where open_bam_prefetch_readers and
create_output_header are used so you reuse header.header() as the base and only
modify the sort-order keys rather than reconstructing the entire `@HD`.
- Around line 762-775: The early return when initial_keys.is_empty() skips
creating the output BAM; move the call to crate::bam_io::create_raw_bam_writer
(the writer creation used for
output/output_header/self.threads/self.output_compression) to before the
empty-check (i.e., create writer before calling LoserTree::new or checking
initial_keys), then on the zero-record path log "Merge complete: 0 records
merged" and properly finish/close the writer (drop or call its finish/flush
method) before returning Ok(0) so an output BAM is always created even when
there are no records.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 784d6a6b-6ba5-43bc-87c3-4c26b21831dc
📒 Files selected for processing (7)
src/commands/merge.rssrc/commands/mod.rssrc/commands/sort.rssrc/lib/sort/loser_tree.rssrc/lib/sort/mod.rssrc/lib/sort/raw.rssrc/main.rs
260d306 to
da49849
Compare
da49849 to
94901ce
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/commands/merge.rs (1)
182-184: Prefer returning an error overassert!.Line 115 guards this, but
merge_headersis a public-ish helper. Returningbail!("No input files")is more defensive.Proposed fix
fn merge_headers(input_paths: &[PathBuf]) -> Result<Header> { - assert!(!input_paths.is_empty()); + if input_paths.is_empty() { + bail!("No input files to merge headers from"); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/merge.rs` around lines 182 - 184, The function merge_headers currently uses assert!(!input_paths.is_empty()) which can panic; change it to return an error instead (e.g., use anyhow::bail!("No input files") or return Err(anyhow!("No input files"))) so callers receive a Result error rather than a panic; update the merge_headers signature usage as needed and add the necessary anyhow import (bail or anyhow) to ensure compile-time resolution.src/lib/sort/loser_tree.rs (1)
176-181:is_empty()is unreachable for valid trees.Constructor asserts
k > 0(line 56), sois_empty()always returnsfalse. Consider removing or documenting this as a trait-compat placeholder.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/sort/loser_tree.rs` around lines 176 - 181, The is_empty() method on LoserTree is effectively unreachable because the constructor/new enforces k > 0; either remove the is_empty(&self) -> bool method entirely (and update any call sites) or keep it only as a trait-compatibility placeholder by changing its implementation to return false unconditionally and adding a doc comment stating “constructor guarantees k > 0, kept for trait compatibility” (refer to is_empty() and the LoserTree constructor/new that asserts k > 0 to locate the code).
🤖 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/commands/merge.rs`:
- Around line 182-184: The function merge_headers currently uses
assert!(!input_paths.is_empty()) which can panic; change it to return an error
instead (e.g., use anyhow::bail!("No input files") or return Err(anyhow!("No
input files"))) so callers receive a Result error rather than a panic; update
the merge_headers signature usage as needed and add the necessary anyhow import
(bail or anyhow) to ensure compile-time resolution.
In `@src/lib/sort/loser_tree.rs`:
- Around line 176-181: The is_empty() method on LoserTree is effectively
unreachable because the constructor/new enforces k > 0; either remove the
is_empty(&self) -> bool method entirely (and update any call sites) or keep it
only as a trait-compatibility placeholder by changing its implementation to
return false unconditionally and adding a doc comment stating “constructor
guarantees k > 0, kept for trait compatibility” (refer to is_empty() and the
LoserTree constructor/new that asserts k > 0 to locate the code).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b1ad4c37-e13b-4e42-8d17-9dcc7e5fd7d4
📒 Files selected for processing (4)
src/commands/merge.rssrc/commands/sort.rssrc/lib/sort/loser_tree.rssrc/lib/sort/raw.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/commands/sort.rs
94901ce to
edf48c9
Compare
edf48c9 to
076eb77
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
076eb77 to
f19d327
Compare
f19d327 to
2b6dfdf
Compare
Add `fgumi merge` subcommand for k-way merging of pre-sorted BAM files, supporting coordinate, queryname, and template-coordinate sort orders. Features: - CLI follows fgumi conventions (-i, -o, --threads, --order) - Merges headers from all inputs (read groups, program records) - Validates reference dictionaries match across inputs - Detects output aliasing input paths - Accepts single input (acts as copy, matching samtools merge) - Supports -b/--input-list for file-of-filenames input Includes a loser tree (tournament tree) data structure for the merge, providing log2(k) comparisons per element vs 2*log2(k) for a binary heap. Based on the incremental insertion approach from Apache DataFusion. Also fixes chunk file naming collision during sort consolidation by replacing chunk_files.len() with a monotonic counter.
2b6dfdf to
c6567d0
Compare
This branch was previously deployed
1 participant
Summary
fgumi mergesubcommand for k-way merging of pre-sorted BAM files, supporting coordinate, queryname, and template-coordinate orderssamtools mergeconventions (-o,-b,-@,--order)log2(k)comparisons per element vs2·log2(k)for a binary heapchunk_files.len())Loser tree
Uses the incremental insertion approach from Apache DataFusion — no power-of-2 padding required. 12 comprehensive tests covering stability, non-power-of-2 fan-in, large fan-in (k=64), and duplicate keys.
Test plan
fgumi mergeproduces IDENTICAL output to baseline (verified withfgumi compare bamson 89M records)fgumi sort --verifypasses on all merge outputscargo ci-fmtcleancargo ci-lintcleancargo ci-test(running)