refactor(sam): reuse shared utilities and avoid inner-loop allocations - #147
Conversation
Use fgumi_dna::reverse_complement in revcomp_buf_value instead of inline reverse-complement logic, and remove the unused complement_base import. Replace match_count.to_string() with write!(md_string, ...) in both alignment tag regeneration functions, avoiding a String allocation per mismatch in the inner loop. Replace inline reference span calculation in regenerate_alignment_tags_raw with fgumi_raw_bam::reference_length_from_cigar.
📝 WalkthroughWalkthroughThe changes refactor MD tag construction in alignment processing to use write! macro-based formatting instead of manual string concatenation. Reference length computation is consolidated via a helper function. Validation and error handling in the raw parsing path is enhanced with explicit bounds checks on sequence, quality, and CIGAR offsets. The reverse complement implementation switches to the fgumi_dna library function with an expect on UTF-8 conversion failures, replacing previous complement mapping logic. Error handling transitions from implicit lossy conversion to explicit panics on non-ASCII data. 🚥 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #147 +/- ##
=======================================
Coverage 83.61% 83.61%
=======================================
Files 126 126
Lines 51510 51510
=======================================
Hits 43069 43069
Misses 8441 8441 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/fgumi-sam/src/alignment_tags.rs`:
- Line 297: The cast of reference_length_from_cigar(&cigar_ops) to usize is
unsafe: call usize::try_from(reference_length_from_cigar(&cigar_ops)) and handle
the Err case (return an error or skip/make the record invalid) to avoid
negative-to-usize wrapping for ref_span; then replace direct arithmetic around
ref_span (the math using ref_span and the alignment start/end near where
position is adjusted, e.g., the code around the later coordinate math) with
checked_add/checked_sub (checked_add, checked_sub) and propagate or return an
error if any check returns None so downstream coordinate computation cannot be
corrupted.
In `@crates/fgumi-sam/src/lib.rs`:
- Around line 313-317: The doc claims "Cannot panic" but the code in the reverse
complement path uses String::from_utf8(revcomp).expect(...) which can panic for
invalid UTF-8; change that to use the same lossy conversion pattern as
reverse_buf_value (i.e., call std::string::String::from_utf8_lossy or
from_utf8_lossy(&revcomp).into_owned()) so non-UTF8 bytes are handled without
panicking; update the conversion site where reverse/complement bytes are turned
into a String (referencing the reverse_buf_value behavior at line ~287 and the
String::from_utf8(...) call in the reverse-complement code).
This branch was previously deployed
1 participant
Summary
revcomp_buf_valuecomplement logic withfgumi_dna::reverse_complementmatch_count.to_string()allocations in MD tag regeneration inner loop withwrite!(md_string, "{match_count}")usingstd::fmt::Writeregenerate_alignment_tags_rawwithfgumi_raw_bam::reference_length_from_cigarTest plan
cargo nextest run -p fgumi-sam— all tests passcargo ci-fmt— cleancargo ci-lint— clean