Repository navigation
perf(compare): replace RecordBuf with raw byte comparison for compare-bams - #197
Conversation
📝 WalkthroughWalkthroughReplaces RecordBuf-based BAM comparison with raw-byte comparison. Adds 🚥 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 #197 +/- ##
==========================================
+ Coverage 86.12% 87.84% +1.71%
==========================================
Files 110 111 +1
Lines 51974 52217 +243
==========================================
+ Hits 44765 45870 +1105
+ Misses 7209 6347 -862 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/commands/compare/bams.rs (1)
406-417:⚠️ Potential issue | 🟠 MajorDon't collapse the read key to a single
u64.Different
(read_name, R1/R2)pairs can collide here, and the latermi_map.insert(...)turns that into silent overwrites. For a validator, that makes grouping/full comparison probabilistic on large BAMs. Keep the real key bytes, or use a collision bucket behind the hash.Also applies to: 448-489
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/compare/bams.rs` around lines 406 - 417, Avoid collapsing read keys to a single u64: the ReadKeyHash type alias and hash_read_key_raw(name, is_read1) produce collisions that allow mi_map.insert(...) to silently overwrite different (read_name, R1/R2) pairs. Fix by keeping the original key bytes (e.g., use the owned name bytes + is_read1 as the map key) or implement a hash->bucket strategy (store a Vec of full keys/values per hash and compare full bytes on insert/lookup) so you still use ahash for speed but resolve collisions deterministically instead of relying on a lone u64.
🧹 Nitpick comments (1)
tests/integration/test_compare_bams.rs (1)
73-231: Add regressions for reordered tags and--ignore-order.The suite doesn't hit the two new branches most likely to regress: equal tags serialized in different aux order, and grouping with
--ignore-order. One CLI case for each would lock the intended behavior in.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/test_compare_bams.rs` around lines 73 - 231, Add two integration tests: one that writes two BAMs whose records have identical auxiliary tags but serialized in different aux order (use RecordBuilder or mapped_record_with_mi + tag(...) calls in different orders, write_bam, then run_compare(..., "full", &[]) and assert success and "IDENTICAL"/"EQUIVALENT" as appropriate); and one that exercises grouping mode with the --ignore-order flag by creating paired reads whose segments are in different order between the two BAMs (use RecordBuilder to flip R1/R2 ordering, write_bam, then run_compare(..., "grouping", &["--ignore-order"]) and assert success and that output contains "EQUIVALENT"). Reference helpers write_bam, run_compare, RecordBuilder, mapped_record_with_mi to locate where to add these 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 `@src/commands/compare/bams.rs`:
- Around line 633-642: The fields from RawCompareResult are mapped backwards
when constructing a RecordCompareResult: raw_compare_structured() uses
RawCompareResult.tags_match to mean byte-identical aux data and
RawCompareResult.tag_order_match to mean order-independent (semantic) equality,
while RecordCompareResult.tags_match is intended to represent semantic "values
match"; fix both construction sites (the one around raw_compare_structured(...)
at ~633-642 and the other at ~743-748) by swapping the mappings so that
RecordCompareResult.tags_match = raw_result.tag_order_match and
RecordCompareResult.tag_order_match = raw_result.tags_match (ensure you update
the code that builds the RecordCompareResult from raw_result accordingly).
- Around line 803-844: The GroupingCompareResult currently sets
read_name_for_display to None causing missing-MI branches to show "?"—when
needs_detail is true and exactly one of mi1/mi2 is present, capture the
appropriate qname and set read_name_for_display to Some(qname) so the downstream
missing-MI reporting uses the real read name; to implement this, reuse the
qname1/qname2 you already create inside the diff_detail branch
(String::from_utf8_lossy(name1_bytes).into_owned() and name2_bytes variant) and
assign that value to read_name_for_display in the GroupingCompareResult when
creating DiffDetail for either DiffType::FlagMismatch or
DiffType::ReadNameMismatch (or when mi1.is_some() != mi2.is_some()), ensuring
you only allocate the string when needs_detail is true.
---
Outside diff comments:
In `@src/commands/compare/bams.rs`:
- Around line 406-417: Avoid collapsing read keys to a single u64: the
ReadKeyHash type alias and hash_read_key_raw(name, is_read1) produce collisions
that allow mi_map.insert(...) to silently overwrite different (read_name, R1/R2)
pairs. Fix by keeping the original key bytes (e.g., use the owned name bytes +
is_read1 as the map key) or implement a hash->bucket strategy (store a Vec of
full keys/values per hash and compare full bytes on insert/lookup) so you still
use ahash for speed but resolve collisions deterministically instead of relying
on a lone u64.
---
Nitpick comments:
In `@tests/integration/test_compare_bams.rs`:
- Around line 73-231: Add two integration tests: one that writes two BAMs whose
records have identical auxiliary tags but serialized in different aux order (use
RecordBuilder or mapped_record_with_mi + tag(...) calls in different orders,
write_bam, then run_compare(..., "full", &[]) and assert success and
"IDENTICAL"/"EQUIVALENT" as appropriate); and one that exercises grouping mode
with the --ignore-order flag by creating paired reads whose segments are in
different order between the two BAMs (use RecordBuilder to flip R1/R2 ordering,
write_bam, then run_compare(..., "grouping", &["--ignore-order"]) and assert
success and that output contains "EQUIVALENT"). Reference helpers write_bam,
run_compare, RecordBuilder, mapped_record_with_mi to locate where to add these
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: 76f0e23c-7a95-4bda-a4e6-18a90447e996
📒 Files selected for processing (5)
src/commands/compare/bams.rssrc/commands/compare/mod.rssrc/commands/compare/raw_compare.rstests/integration/main.rstests/integration/test_compare_bams.rs
…-bams Switch all three compare modes (content, full, grouping) from noodles RecordBuf-based comparison to raw byte comparison using fgumi_raw_bam. Three-tier comparison strategy: 1. Full byte memcmp short-circuit for identical records (common case) 2. Structured raw field comparison for records with different tag order 3. RecordBuf deserialization only for human-readable diff reporting Reader threads now do only length-prefix reads + memcpy instead of full record parsing, making them I/O-bound rather than CPU-bound.
e5378b1 to
a3db899
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/commands/compare/raw_compare.rs (1)
76-108: Docstring slightly misleading on "same values".Line 76-77 says "same tags with the same values, regardless of order" but the comparison is byte-level (includes type byte). Tags encoded differently (e.g.,
MI:i:123vsMI:C:123) would fail even if logically equal.This is fine as a fast-path check—Tier 3 deserialization handles semantic equality—but the comment could clarify it's byte-identical values, not logically-equal values.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/compare/raw_compare.rs` around lines 76 - 108, The docstring for raw_tags_equal_order_independent is misleading: the function compares aux tag encodings byte-for-byte (including the type byte) rather than semantic/logical values; update the comment to state it checks for byte-identical aux tag encodings (including tag type byte) and that semantically-equivalent but differently-encoded tags (e.g., different type encodings) will be considered unequal; reference raw_tags_equal_order_independent, collect_tag_entries and the aux slices returned by fgumi_raw_bam::fields::aux_data_slice so readers know this is a fast byte-level check and that semantic equality is handled elsewhere.
🤖 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/compare/raw_compare.rs`:
- Around line 76-108: The docstring for raw_tags_equal_order_independent is
misleading: the function compares aux tag encodings byte-for-byte (including the
type byte) rather than semantic/logical values; update the comment to state it
checks for byte-identical aux tag encodings (including tag type byte) and that
semantically-equivalent but differently-encoded tags (e.g., different type
encodings) will be considered unequal; reference
raw_tags_equal_order_independent, collect_tag_entries and the aux slices
returned by fgumi_raw_bam::fields::aux_data_slice so readers know this is a fast
byte-level check and that semantic equality is handled elsewhere.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 77add30d-fb1f-47c9-aaee-4a25885cdc92
📒 Files selected for processing (5)
src/commands/compare/bams.rssrc/commands/compare/mod.rssrc/commands/compare/raw_compare.rstests/integration/main.rstests/integration/test_compare_bams.rs
✅ Files skipped from review due to trivial changes (1)
- src/commands/compare/mod.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/integration/main.rs
- tests/integration/test_compare_bams.rs
The raw byte comparison introduced in #197 included the BAM `bin` field (bytes 10-11) when comparing core fields. The bin field is a BAM-specific index optimization not part of the SAM data model, and different tools may compute different values for the same alignment. This caused false core_diff reports when comparing outputs from different tools (e.g. fgumi vs fgbio) that only differed in tag order and bin values. Skip bytes 10-11 in `raw_core_fields_equal` so that bin differences no longer trigger false mismatches. Fixes #207
The raw byte comparison introduced in #197 included the BAM `bin` field (bytes 10-11) when comparing core fields. The bin field is a BAM-specific index optimization not part of the SAM data model, and different tools may compute different values for the same alignment. This caused false core_diff reports when comparing outputs from different tools (e.g. fgumi vs fgbio) that only differed in tag order and bin values. Skip bytes 10-11 in `raw_core_fields_equal` so that bin differences no longer trigger false mismatches. Fixes #207
This branch was previously deployed
1 participant
Summary
RecordBuf-based reading and comparison infgumi compare bamswith raw byte comparison usingfgumi-raw-bam, significantly improving performance for all three modes (content, full, grouping)RecordBufdeserialization only for diff reportingraw_comparemodule with 30 unit tests and 7 integration tests covering all comparison modesTest plan
cargo nextest run --features compare -E 'test(compare)')cargo ci-fmtpassescargo ci-lintpasses