Repository navigation
perf(zipper): eliminate RecordBuf re-encode bottleneck with raw-byte merge - #228
Merged
Merged
Conversation
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughAdds raw-byte BAM manipulation across the crate. New public APIs and helpers: 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #228 +/- ##
==========================================
+ Coverage 88.18% 88.88% +0.69%
==========================================
Files 113 113
Lines 53388 54452 +1064
==========================================
+ Hits 47080 48398 +1318
+ Misses 6308 6054 -254 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Member
Author
|
@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
🧹 Nitpick comments (1)
src/commands/zipper.rs (1)
737-750: Avoid leavingTemplatein mixed raw/RecordBufmode.After this conversion,
raw_recordsbecomes the source of truth, butrecordsstill holds the pre-mergeRecordBufs. Any later caller that touchesrecords,r1(), orall_reads()on this template will see stale data instead of the bytes being written.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/zipper.rs` around lines 737 - 750, convert_template_to_raw creates template.raw_records but leaves template.records populated, causing callers of records/r1()/all_reads() to see stale RecordBufs; after successfully building raw_records in convert_template_to_raw, replace or clear template.records so the Template is no longer in a mixed mode (e.g., set template.records = Vec::new() or rebuild RecordBufs from the new raw_records), while preserving any read_count compatibility semantics; update convert_template_to_raw to mutate the Template.fields (records and raw_records) consistently so subsequent callers use raw_records as the source of truth.
🤖 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/zipper.rs`:
- Around line 724-729: The loop over mapped.raw_records currently
unconditionally calls bam_fields::append_i32_array_tag(record, pa_tag,
&pa_values) which can create duplicate PA tags; change this to overwrite the PA
tag instead of appending: for each record in mapped.raw_records (rr/record)
detect if the PA tag (pa_tag) already exists and either remove it first or use
the appropriate "set" operation so the existing PA value is replaced with
pa_values (mirroring the RecordBuf path that replaced PA_TAG) rather than
appending a second AUX field.
In `@src/lib/template.rs`:
- Around line 1064-1070: The code currently converts mate AS values with
i32::try_from(...).unwrap_or(0) and writes ms=0 on overflow, which silently
corrupts the tag; change the logic so you only call bam_fields::update_int_tag
when the try_from succeeds (e.g., if let Ok(v) = i32::try_from(as_value) {
bam_fields::update_int_tag(&mut rr[r1_i], b"ms", v); }) and skip updating the
tag when conversion fails, mirroring the RecordBuf path; apply the same fix to
the other occurrences that use r1_as/r2_as and bam_fields::update_int_tag (the
analogous blocks that currently use unwrap_or(0)).
---
Nitpick comments:
In `@src/commands/zipper.rs`:
- Around line 737-750: convert_template_to_raw creates template.raw_records but
leaves template.records populated, causing callers of records/r1()/all_reads()
to see stale RecordBufs; after successfully building raw_records in
convert_template_to_raw, replace or clear template.records so the Template is no
longer in a mixed mode (e.g., set template.records = Vec::new() or rebuild
RecordBufs from the new raw_records), while preserving any read_count
compatibility semantics; update convert_template_to_raw to mutate the
Template.fields (records and raw_records) consistently so subsequent callers use
raw_records as the source of truth.
🪄 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: 3ea9a6d4-8d3b-4733-aea7-1aef675cf33d
📒 Files selected for processing (5)
crates/fgumi-raw-bam/src/cigar.rscrates/fgumi-raw-bam/src/fields.rscrates/fgumi-raw-bam/src/tags.rssrc/commands/zipper.rssrc/lib/template.rs
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/commands/zipper.rs (2)
789-796: Doc comment skips step 4.Numbering goes 1, 2, 3, 5, 6, 7. Likely should be 1-7 sequential.
Minor doc fix
/// 1. Fix mate info (raw) /// 2. Remove tags from mapped records /// 3. Copy tags from unmapped to mapped (with reverse/revcomp for neg strand) -/// 5. Transfer QC flags -/// 6. Normalize AS/XS tags -/// 7. Add PA tags +/// 4. Transfer QC flags +/// 5. Normalize AS/XS tags +/// 6. Add PA tags🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/zipper.rs` around lines 789 - 796, Update the doc comment above the function (the comment that starts "Performs the same 7 operations as `merge()`") to fix the step numbering so it reads 1 through 7 sequentially (change the "5." entry to "4." and renumber remaining items if needed) and ensure the list accurately matches the seven operations performed by merge(); refer to the comment mentioning `merge()` and the surrounding doc block in zipper.rs when making the edit.
597-607: Silent truncation on array overflow.
unwrap_or(0)writes count=0 but proceeds to write all elements ifvals.len() > u32::MAX. Practically impossible, but inconsistent withappend_i32_array_tagwhich panics.Consider using
expect()for consistency with other array tag functions, or document the behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/zipper.rs` around lines 597 - 607, The Array::UInt8 branch silently uses u32::try_from(vals.len()).unwrap_or(0) which sets count=0 on overflow but still writes all elements; change this to mirror append_i32_array_tag by using expect() (or otherwise explicitly panic) on failed conversion so an overflow cannot silently truncate the length, i.e. locate the Array::UInt8 arm in the zipper serialization code and replace the unwrap_or(0) with a conversion that fails loudly (e.g., u32::try_from(...).expect("array length overflow") ) so behavior is consistent with other array tag functions.src/lib/template.rs (1)
1076-1110: Repeated unwrap pattern after immutable borrows.The
self.raw_records.as_ref().unwrap()at line 1078 andas_mut().unwrap()at line 1090 are safe (initial check guarantees Some), but rebindingrrmultiple times is verbose.Consider passing the slice directly to avoid repeated unwraps:
let rr = self.raw_records.as_mut().unwrap(); // work directly with rr throughout🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/template.rs` around lines 1076 - 1110, The code currently does two borrows of self.raw_records (as_ref() then as_mut()); replace them with a single mutable borrow to reduce verbosity and avoid repeated unwraps: after confirming self.r2 (and obtaining r2_i) do let rr = self.raw_records.as_mut().unwrap(); then read r2 fields from rr[r2_i] (r2_ref_id, r2_pos, r2_flags, r2_is_reverse, r2_is_unmapped, r2_tlen, r2_mapq, r2_cigar_str, r2_as) and then use the same rr slice to iterate the supplementals (self.r1_supplementals) and update records via set_mate_flags_raw and bam_fields::set_mate_ref_id/ set_mate_pos/ set_template_length/ update_int_tag/ update_string_tag/ update_int_tag for ms; this removes the prior as_ref().unwrap() and the second as_mut().unwrap() while keeping the same logic.
🤖 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/zipper.rs`:
- Around line 789-796: Update the doc comment above the function (the comment
that starts "Performs the same 7 operations as `merge()`") to fix the step
numbering so it reads 1 through 7 sequentially (change the "5." entry to "4."
and renumber remaining items if needed) and ensure the list accurately matches
the seven operations performed by merge(); refer to the comment mentioning
`merge()` and the surrounding doc block in zipper.rs when making the edit.
- Around line 597-607: The Array::UInt8 branch silently uses
u32::try_from(vals.len()).unwrap_or(0) which sets count=0 on overflow but still
writes all elements; change this to mirror append_i32_array_tag by using
expect() (or otherwise explicitly panic) on failed conversion so an overflow
cannot silently truncate the length, i.e. locate the Array::UInt8 arm in the
zipper serialization code and replace the unwrap_or(0) with a conversion that
fails loudly (e.g., u32::try_from(...).expect("array length overflow") ) so
behavior is consistent with other array tag functions.
In `@src/lib/template.rs`:
- Around line 1076-1110: The code currently does two borrows of self.raw_records
(as_ref() then as_mut()); replace them with a single mutable borrow to reduce
verbosity and avoid repeated unwraps: after confirming self.r2 (and obtaining
r2_i) do let rr = self.raw_records.as_mut().unwrap(); then read r2 fields from
rr[r2_i] (r2_ref_id, r2_pos, r2_flags, r2_is_reverse, r2_is_unmapped, r2_tlen,
r2_mapq, r2_cigar_str, r2_as) and then use the same rr slice to iterate the
supplementals (self.r1_supplementals) and update records via set_mate_flags_raw
and bam_fields::set_mate_ref_id/ set_mate_pos/ set_template_length/
update_int_tag/ update_string_tag/ update_int_tag for ms; this removes the prior
as_ref().unwrap() and the second as_mut().unwrap() while keeping the same logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: aef2d937-e36b-4b30-8c6c-e7558630856c
📒 Files selected for processing (5)
crates/fgumi-raw-bam/src/cigar.rscrates/fgumi-raw-bam/src/fields.rscrates/fgumi-raw-bam/src/tags.rssrc/commands/zipper.rssrc/lib/template.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/fgumi-raw-bam/src/cigar.rs
- crates/fgumi-raw-bam/src/fields.rs
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
src/commands/zipper.rs (1)
857-864: Add an end-to-endPGpreservation test for the raw path.
merge_rawnow carries its own "keep mappedPG, ignore unmappedPG" logic, but the current tests only cover that behavior throughcopy_tags. A smallrun_zippercase withPGon both inputs would pin this branch down.Also applies to: 893-898
🤖 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/tags.rs`:
- Around line 711-715: Update the docstring for
normalize_int_tag_to_smallest_signed to accurately describe its behavior: state
that when the tag exists and is an integer type it is re-encoded using
append_signed_int_tag semantics (i.e., it promotes only through signed types i8
-> i16 -> i32), and remove any mention of unsigned-preferring behavior or
u8/u16; reference normalize_int_tag_to_smallest_signed and append_signed_int_tag
in the description so readers know which function controls the re-encoding path.
In `@src/commands/zipper.rs`:
- Around line 1071-1093: The code currently calls writer.finish() in the error
branches that handle leftover mapped reads (the branches that inspect
mapped_peek and mapped_iter.next()), which finalizes the BAM and can leave a
valid-looking but incomplete output on fatal input-order errors; remove the
writer.finish() calls from those error paths in zipper.rs (the branches handling
mapped_peek and the mapped_iter.next() result) so that writer.finish() is only
invoked on successful completion, and simply bail with anyhow::bail! including
the String::from_utf8_lossy(&remaining.name) context instead of finalizing the
writer.
- Around line 590-649: The match arm for BufValue::Array currently uses
u32::try_from(vals.len()).unwrap_or(0), which silently writes a zero length on
overflow; change each place that calls u32::try_from(vals.len()).unwrap_or(0)
(inside Array::Int8/UInt8/Int16/UInt16/Int32/UInt32/Float branches) to fail fast
on overflow instead of inserting 0: perform the try_from and propagate or return
an explicit error (or panic) when conversion fails (e.g., map_err/ok_or and
return Err) before writing the count or payload so the function that serializes
arrays (the BufValue::Array handling) aborts on oversized arrays rather than
producing a corrupt header.
In `@src/lib/template.rs`:
- Around line 1100-1102: The code only calls bam_fields::update_string_tag(rec,
b"MC", ...) when r2_cigar_str is present, so existing MC tags remain when the
mate becomes unmapped or has "*" / empty CIGAR; update the logic in the MC
branches (e.g., the block using r2_cigar_str and r2_is_unmapped and the
analogous blocks at lines referenced for r1/r2) to explicitly remove the MC tag
when the mate CIGAR should disappear: detect the "mate absent" condition
(r2_is_unmapped || r2_cigar_str.is_empty() || r2_cigar_str == "*") and call the
tag-removal path (the same mechanism used elsewhere for deleting tags) instead
of skipping update_string_tag, while keeping update_string_tag for the non-empty
case.
- Around line 1064-1072: Replace calls to bam_fields::update_int_tag that write
the "ms" tag with the signed-int writer so positive values >127 keep the signed
aux type; specifically, where code currently does
bam_fields::update_int_tag(&mut rr[r1_i], b"ms", v) or update_int_tag(&mut
rr[r2_i], b"ms", v) use bam_fields::append_signed_int_tag(&mut rr[r1_i], b"ms",
v) (or append_signed_int_tag(&mut rr[r2_i], b"ms", v)) so the signed encoder is
used, or alternatively call bam_fields::normalize_int_tag_to_smallest_signed
after writing to normalize existing tags; apply the same replacement at the
other occurrences mentioned (around the blocks with r1_as/r2_as and the lines
referenced in the comment).
🪄 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: d25112ed-b2ba-4c5a-9ee1-f8e49d64d24f
📒 Files selected for processing (5)
crates/fgumi-raw-bam/src/cigar.rscrates/fgumi-raw-bam/src/fields.rscrates/fgumi-raw-bam/src/tags.rssrc/commands/zipper.rssrc/lib/template.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/fgumi-raw-bam/src/cigar.rs
- crates/fgumi-raw-bam/src/fields.rs
…merge Replace the noodles RecordBuf encode path in the zipper with direct raw Vec<u8> BAM record manipulation, eliminating the serialization bottleneck visible in profiling. The mapped template is converted to raw bytes once (via the vendored BAM encoder), then all 7 merge operations (fix mate info, tag removal, tag copying with reverse/revcomp, QC flag transfer, AS/XS normalization, PA tags) are performed directly on the raw bytes using fgumi_raw_bam primitives. Output is written via RawBamWriter::write_raw_record, bypassing the noodles re-encode entirely. New raw BAM primitives: field setters (set_mapq, set_mate_ref_id, set_mate_pos, set_template_length, set_ref_id, set_pos), cigar_to_string_from_raw, append_i32_array_tag, append_signed_int_tag, normalize_int_tag_to_smallest_signed, copy_aux_tags. New Template method: fix_mate_info_raw operating on raw_records. Adds tests for AS/XS normalization, negative strand tag copying, and fix_mate_info value verification.
Merged
This was referenced Jul 9, 2026
This branch was previously deployed
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
RecordBufserialization bottleneck inzipperby performing all 7 merge operations directly on rawVec<u8>BAM bytes, then writing viaRawBamWriterfgumi-raw-bam: field setters (set_mapq,set_mate_ref_id, etc.),cigar_to_string_from_raw, signed int normalization,i32array tags, and bulk tag copyfix_mate_info_rawonTemplatewith three helper methods for the mapped/unmapped/mixed pairing casesprocess_rawmethod, removing ~330 lines of dead/duplicate codeTest plan
cargo ci-fmtandcargo ci-lintclean