Repository navigation
feat(raw-bam): unified view/editor API + close #272 gaps - #278
Conversation
Adds the borrowed read-only view type that hosts all BAM read accessors. Constructors: new (trust-caller, debug_assert) + try_new (validates min length). Coexists with existing free functions; subsequent commits port methods over. Refs #272
Adds method-form accessors for every fixed-offset BAM header field, plus flag-bit convenience predicates (is_reverse, is_unmapped, ...). bin/set_bin were absent from the free-function API and are added here for completeness. Refs #272
Method-form CIGAR accessors with a zero-allocation cigar_ops_iter alongside the existing cigar_ops_vec convenience. Length, end-position, and unclipped-5' helpers exposed as inherent methods. Refs #272
Method-form accessors for packed/iterated/decoded sequence and quality scores. Zero-allocation sequence_packed, sequence_iter, quality_scores; sequence_vec remains for convenience. Refs #272
Borrowed read-only view over BAM aux data. Hosts find_string/int/uint8/float/ array, find_mi, find_mc plus contains/len/is_empty. RawRecordView::tags() returns one. TagEntry struct defined for upcoming iter(). Refs #272
Closes the "Nice gap" from #272. AuxTagsIter walks the aux section yielding TagEntry { tag, type_byte, value_bytes } without allocation. Uses the existing tag_value_size for variable-length (Z/H/B) entries. Implements IntoIterator for &RawTagsView to satisfy clippy::iter_without_into_iter. Refs #272
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #278 +/- ##
==========================================
+ Coverage 89.88% 90.03% +0.15%
==========================================
Files 120 122 +2
Lines 61585 61771 +186
==========================================
+ Hits 55354 55615 +261
+ Misses 6231 6156 -75 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 35 minutes and 0 seconds. ⌛ 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 (3)
📝 WalkthroughWalkthroughThis change implements comprehensive raw-byte BAM record access and mutation APIs. It introduces 🚥 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)
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 review |
1 similar comment
|
@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: 4
🧹 Nitpick comments (1)
crates/fgumi-raw-bam/tests/noodles_roundtrip.rs (1)
44-64: Strengthen the noodles-side assertions.These properties are weaker than they look.
set_cigar_ops_roundtripsonly checks op count, andset_sequence_and_qualities_roundtripsonly checks that noodles parses at all. A bug that preserves parseability but changes op values / decoded bases / quals still passes. Compare the noodles-decoded contents tocigar_ops,bases, andqualsdirectly.Also applies to: 67-92
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/fgumi-raw-bam/tests/noodles_roundtrip.rs` around lines 44 - 64, The tests set_cigar_ops_roundtrips and set_sequence_and_qualities_roundtrips only assert parseability or op count; update them to assert full content equality between what our record produces and what noodles decodes: after calling decode_with_noodles(...) on the record built by build_record(...) and modified via set_cigar_ops / set_sequence_and_qualities, iterate the noodles-decoded CIGAR (buf.cigar()) and compare each decoded op (length and op code) against cigar_ops (or rec.cigar_ops_vec()), compare the decoded sequence/bases bytes against the original sequence used to build the record, and compare the decoded quality scores against the original quals vector; update both test functions to use proptest::prop_assert_eq! (or appropriate proptest assertions) for these element-wise comparisons so mismatched op values, bases, or quals fail the tests.
🤖 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-raw-bam/src/raw_bam_record.rs`:
- Around line 626-649: In set_read_name, reject any embedded NULs in the
provided new_name before splicing: check new_name for a 0 byte (e.g., if
new_name.contains(&0)) and panic with a clear message like "read name contains
embedded NUL" (consistent with the existing panic behavior for length); perform
this validation before computing new_l/new_l_u8 and performing the splice so you
never write an interior NUL into the record. Ensure you still include the
trailing NUL when constructing the replacement and update self.0[8] as before.
- Around line 192-199: tags_mut() can panic when aux_data_offset_from_record
returns an offset greater than the buffer length; clamp the computed offset to
the buffer length before slicing. Change the logic in tags_mut to compute len =
self.0.len(), compute off = aux_data_offset_from_record(&self.0).unwrap_or(len)
and then set off = off.min(len) (or use a conditional check) before calling
RawTagsMut::new(&mut self.0[off..]); reference functions/values: tags_mut,
aux_data_offset_from_record, RawTagsMut::new, and self.0.
In `@crates/fgumi-raw-bam/src/tags.rs`:
- Around line 1206-1215: set_float_in_place currently writes four bytes without
validating that the aux buffer contains enough data, causing a panic on
truncated aux data; update set_float_in_place (the function shown) to perform
the same bounds check as find_float_tag/RawTagsEditor::update_float by verifying
p + 7 <= self.0.len() (or otherwise that the slice range p+3..p+7 is in-bounds)
before calling copy_from_slice, and return false if the check fails.
- Around line 1163-1200: In set_int_in_place, there are unsafe slice writes
without ensuring the buffer has enough bytes; before each match arm that writes
multi-byte values (for tag types 's'/'S' check that p + 5 <= self.0.len(), for
'i'/'I' check that p + 7 <= self.0.len(), and for single-byte 'c'/'C' check p +
4 <= self.0.len()), return false if the check fails; perform these bounds checks
immediately after finding the tag (using the p from find_tag_position) or at the
start of each corresponding match arm so the subsequent copy_from_slice or index
assignment cannot panic. Ensure you reference set_int_in_place and the match on
val_type when implementing these guards.
---
Nitpick comments:
In `@crates/fgumi-raw-bam/tests/noodles_roundtrip.rs`:
- Around line 44-64: The tests set_cigar_ops_roundtrips and
set_sequence_and_qualities_roundtrips only assert parseability or op count;
update them to assert full content equality between what our record produces and
what noodles decodes: after calling decode_with_noodles(...) on the record built
by build_record(...) and modified via set_cigar_ops /
set_sequence_and_qualities, iterate the noodles-decoded CIGAR (buf.cigar()) and
compare each decoded op (length and op code) against cigar_ops (or
rec.cigar_ops_vec()), compare the decoded sequence/bases bytes against the
original sequence used to build the record, and compare the decoded quality
scores against the original quals vector; update both test functions to use
proptest::prop_assert_eq! (or appropriate proptest assertions) for these
element-wise comparisons so mismatched op values, bases, or quals fail the
tests.
🪄 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: 1ef17ff7-14b1-4cfe-ac93-4aad1631478c
📒 Files selected for processing (6)
crates/fgumi-raw-bam/src/cigar.rscrates/fgumi-raw-bam/src/fields.rscrates/fgumi-raw-bam/src/raw_bam_record.rscrates/fgumi-raw-bam/src/sequence.rscrates/fgumi-raw-bam/src/tags.rscrates/fgumi-raw-bam/tests/noodles_roundtrip.rs
Closes the issue 272 Easy gap "update_array_tag_element_*". Adds the full u8/u16/i16/i32/f32 family of in-place B-array element writers. Type-checked against the array's sub-type byte; no-op on mismatch or out-of-range index. Refs #272
Editor borrows the full record Vec plus a cached aux_offset, amortizing the header scan across all tag ops. view() returns a RawTagsView over the cached sub-slice. Subsequent commits add update/append/remove methods. Refs #272
Closes the remaining Easy gaps from #272. update_float overwrites in place when the tag exists as 'f', else removes and re-appends. update_array_* covers u8/u16/i16/i32/f32: same-length overwrite is in-place, length change splices via remove + append. Adds append_u16_array_tag and append_f32_array_tag helpers. Refs #272
Adds view/view_mut/tags/tags_mut/tags_editor accessors plus inherent #[inline] delegators for every read accessor and fixed-length setter, so callers can write rec.flags()/rec.set_pos(0)/rec.tags().find_*() directly. Refs #272
Add integration tests in `crates/fgumi-raw-bam/tests/noodles_roundtrip.rs` (requires `feature = "noodles"`) that cross-check the three length-changing edits — `set_read_name`, `set_cigar_ops`, and `set_sequence_and_qualities` — against noodles decoding. Each proptest verifies that the internal field view and the bytes noodles parses agree, giving structural confidence that the splice logic is byte-level correct.
4eeb0d4 to
e416550
Compare
This branch was previously deployed
1 participant
Summary
Part 1 of 5 in the stacked series closing #272.
Introduces the six-type
fgumi-raw-bamwrapper hierarchy and closes every gap catalogued in the issue. This PR is purely additive — the old free-function API remainspubso no downstream caller breaks. Migration to the new types, demotion of the old API, tests/benches, and perf optimizations land in follow-up PRs.New types
RawRecordView<'a>&'a [u8]RawRecordMut<'a>&'a mut [u8]RawRecordVec<u8>RawTagsView<'a>&'a [u8]RawTagsMut<'a>&'a mut [u8]RawTagsEditor<'a>&'a mut Vec<u8>+ cachedaux_offsetIssue 272 gaps closed (13 new methods)
Easy:
bin(),set_bin(),update_float(),set_array_element_{u8,u16,i16,i32,f32}(),update_array_{u8,u16,i16,i32,f32}()Medium (length-changing):
set_read_name(),set_cigar_ops(),set_sequence_and_qualities()Nice:
RawTagsView::iter()(zero-allocation)Test coverage in this PR
tests/noodles_roundtrip.rs— proptests cross-checking all three length-changing edits against noodles decodeSuggested reading order
crates/fgumi-raw-bam/src/fields.rs—RawRecordView+RawRecordMutcrates/fgumi-raw-bam/src/tags.rs— tag sub-typescrates/fgumi-raw-bam/src/raw_bam_record.rs—RawRecorddelegators + length-changing editscrates/fgumi-raw-bam/src/{cigar,sequence}.rs— view method implsTest plan
cargo ci-test— workspace tests passcargo ci-fmt— cleancargo ci-lint— cleanStack
pub(crate)(BREAKING)Closes #272 when the full stack lands.
Supersedes #277.