Repository navigation
perf(sort): optimize sort pipeline with LoserTree merge, EMBEDDED_IN_RECORD, and queryname specifiers - #217
Merged
Merged
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #217 +/- ##
==========================================
+ Coverage 88.06% 88.18% +0.11%
==========================================
Files 113 113
Lines 52804 53388 +584
==========================================
+ Hits 46503 47081 +578
- Misses 6301 6307 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughQueryname sorting was parameterized by comparator (lexicographic or natural) with new key types ( 🚥 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: 10
🧹 Nitpick comments (2)
crates/fgumi-raw-bam/src/cigar.rs (1)
249-274: Add direct tests forunclipped_5prime_raw.Line [256] introduces a new API path; please add direct cases for forward/reverse and malformed/no-CIGAR fallbacks to lock semantics.
Suggested test shape
#[cfg(test)] mod tests { use super::*; + use rstest::rstest; @@ + #[rstest] + #[case(false, &[(5 << 4) | 4, (10 << 4)], 100, 95)] // forward 5S10M + #[case(true, &[(10 << 4), (5 << 4) | 4], 100, 114)] // reverse 10M5S (0-based basis) + fn test_unclipped_5prime_raw_basic( + #[case] is_reverse: bool, + #[case] cigar: &[u32], + #[case] pos: i32, + #[case] expected: i32, + ) { + let flags = if is_reverse { flags::REVERSE } else { 0 }; + let rec = make_bam_bytes(0, pos, flags, b"rea", cigar, 15, -1, -1, &[]); + assert_eq!(unclipped_5prime_raw(&rec, pos, is_reverse), expected); + }As per coding guidelines
**/*.rs: Use rstest for parameterized tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/fgumi-raw-bam/src/cigar.rs` around lines 249 - 274, Add rstest-based unit tests that directly exercise unclipped_5prime_raw: create parameterized cases covering (1) forward strand with a valid CIGAR where the result should match unclipped_start_from_raw_cigar behavior, (2) reverse strand with a valid CIGAR matching unclipped_end_from_raw_cigar, (3) no-CIGAR (n_cigar_op == 0) returning the input pos, and (4) malformed/truncated BAM bytes where cigar_end > bam.len() and the function falls back to returning pos; for each case construct minimal raw BAM byte slices and call unclipped_5prime_raw(pos, is_reverse) asserting the expected output, using rstest to enumerate the scenarios.src/lib/sort/inline_buffer.rs (1)
284-287: These accessors publish internal buffer invariants.
refs_mut()lets downstream code rewrite offsets and lengths directly, anddata()/header_size()expose the inline storage format. If this is only for sorter internals, preferpub(crate)or a narrower helper so the layout can still evolve safely.Also applies to: 319-329, 772-775
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/sort/inline_buffer.rs` around lines 284 - 287, The public accessors refs_mut(), data(), and header_size() expose internal inline-buffer invariants; change their visibility to pub(crate) or to a more specific internal module-level accessor (e.g., make refs_mut, data, header_size non-pub or pub(in crate::sorter) and/or replace refs_mut with a controlled API that performs safe mutations (like swap_refs(), update_ref_at(), or a Sorter-only mutable borrow wrapper) so external code cannot directly rewrite offsets/lengths or rely on the inline storage layout; update any tests/uses to go through the new internal-only helpers or the sorter module API (target symbols: refs_mut, data, header_size).
🤖 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 330-347: The current single-pass branch in the aux parsing block
(variables: cell_tag, t, result.rg, result.cell, result.mc, found, target_bits)
uses mutually exclusive else-if branches so one aux entry can only populate one
field; change this to perform independent checks for each target field (i.e.,
separate if checks for t == b"RG", cell_tag.is_some_and(|ct| t == *ct), and t ==
b"MC") or else validate/reject overlapping tag names up front so a single aux
tag can populate multiple result fields as before; update found/target_bits
logic so all matching checks set their bits and you still early-return when all
target_bits are satisfied.
In `@src/commands/clip.rs`:
- Line 68: The help text incorrectly claims NM/UQ/MD repair only occurs with
--regenerate-tags, but the code always calls regenerate_alignment_tags; either
make the help text unconditional or reintroduce gating using the regenerate_tags
flag where regenerate_alignment_tags is invoked (and likewise adjust the related
description at the other occurrence around lines 114-117). Locate calls to
regenerate_alignment_tags and the CLI option/variable named regenerate_tags in
clip.rs and either (A) remove the conditional wording from the help/usage string
to reflect always-on behavior, or (B) wrap the calls to
regenerate_alignment_tags (and any mate-pair update paths) in an if
regenerate_tags { ... } so the runtime behavior matches the help text; update
the logged message accordingly to use the same conditional state.
In `@src/commands/codec.rs`:
- Around line 227-229: The new sort_order field on the command struct is
currently unused; update Codec::validate(), execute(), and
execute_threads_mode() to read and apply the sort_order Option<String> (the
sort_order field declared with #[arg(short = 'S', long = "sort-order")]) so the
flag controls output order and header generation. Parse/validate the provided
string into your internal enum (Unsorted, Queryname, Coordinate, Unknown) inside
Codec::validate() (or a helper), store the resolved enum in the codec runtime
config, and then use that config in execute() and execute_threads_mode() when
producing output and headers so the flag is honored. Ensure invalid values
produce a clear validation error from Codec::validate().
In `@src/commands/common.rs`:
- Around line 107-108: The bool flags defined with #[arg(... default_value =
"true")] (e.g., the output_per_base_tags field) are presence-only and don't
accept explicit false; update each such Arg attribute to include action =
clap::ArgAction::Set and use a typed default (e.g., default_value_t = true) so
the flag can be explicitly set to true or false; follow the same pattern used in
src/commands/sort.rs and apply to the three occurrences (including
output_per_base_tags).
In `@src/commands/compare/raw_compare.rs`:
- Around line 37-39: The current slice-equality in raw_core_fields_equal (using
core1/core2 derived from r1/r2 and off1/off2) includes the 2-byte "bin" field
(bytes 10-11), causing unmapped records from different builders to appear
different; fix by excluding or normalizing that field before comparing: either
slice out bytes 10-11 from core1/core2 (skip those indices when creating the
comparison slices) or set the bin bytes to a canonical value (e.g., zero) on
both r1 and r2 prior to equality, and update raw_compare_structured to use the
adjusted cores so semantically equivalent unmapped records match.
In `@src/commands/filter.rs`:
- Around line 158-159: The bool field filter_by_template is currently annotated
with #[arg(default_value = "true")], which clap v4 treats as a presence flag
(ArgAction::SetTrue) so users cannot pass `--filter-by-template false`; change
the attribute to accept an explicit boolean value by using a typed default and
explicit action, e.g. replace default_value = "true" with default_value_t = true
and add action = clap::ArgAction::Set (i.e., #[arg(long = "filter-by-template",
default_value_t = true, action = clap::ArgAction::Set)]) so the
filter_by_template flag can be set to false from the CLI and the single-read
pipeline logic that checks filter_by_template will be reachable.
In `@src/commands/simplex.rs`:
- Around line 205-207: The CLI flag sort_order (pub sort_order: Option<String>)
is parsed but never used; update the command handler (the method that executes
this struct, e.g., Simplex::run or the impl that processes self) to either wire
it through to the BAM writer or, until that wiring exists, explicitly reject or
warn when self.sort_order.is_some(); implement a guard such as returning an
error or printing a clear message if sort_order is provided so the option is not
silently ignored.
In `@src/commands/sort.rs`:
- Around line 67-87: The parse function in src/commands/sort.rs currently
accepts "queryname" and "queryname::lexicographic" but not the new alias
"queryname::lex", causing parses to fail; update the match in pub fn parse(s:
&str) to also accept "queryname::lex" and return the same variant (Queryname or
the lexicographic Queryname variant) as "queryname::lexicographic", and update
any related error strings to list "queryname::lex" as an accepted sub-sort so
both CLI inputs and merge behavior accept the advertised spelling.
In `@src/lib/progress.rs`:
- Around line 37-52: The update method can apply stale smaller counts when
concurrent log_if_needed() calls race for the ema mutex; before mutating EMA
state in update(), early-return the bias-corrected rate if current_count is <=
self.last_count (or if dt <= 0), so last_count and last_time are not moved
backward — i.e., check current_count against self.last_count and ignore the
observation (return self.corrected_rate()) when stale, leaving EMA state
untouched; reference function names: update, corrected_rate, and fields:
last_count, last_time, smoothed_rate, calls, EMA_ALPHA, and the caller
log_if_needed which holds the ema mutex.
In `@src/lib/sort/external.rs`:
- Around line 166-168: The match arm for SortOrder::Queryname(_) ignores the
comparator variant and always uses QuerynameKey, so sort_with_key() never learns
whether lexicographic or natural ordering was requested; update the dispatch in
external.rs to branch on the comparator inside SortOrder::Queryname and call
sort_with_key::<QuerynameLexKey>(...) for lexicographic and
sort_with_key::<QuerynameNaturalKey>(...) for natural (or return an error if a
comparator is unsupported), and ensure create_output_header() still receives the
matching SS tag for the chosen comparator.
---
Nitpick comments:
In `@crates/fgumi-raw-bam/src/cigar.rs`:
- Around line 249-274: Add rstest-based unit tests that directly exercise
unclipped_5prime_raw: create parameterized cases covering (1) forward strand
with a valid CIGAR where the result should match unclipped_start_from_raw_cigar
behavior, (2) reverse strand with a valid CIGAR matching
unclipped_end_from_raw_cigar, (3) no-CIGAR (n_cigar_op == 0) returning the input
pos, and (4) malformed/truncated BAM bytes where cigar_end > bam.len() and the
function falls back to returning pos; for each case construct minimal raw BAM
byte slices and call unclipped_5prime_raw(pos, is_reverse) asserting the
expected output, using rstest to enumerate the scenarios.
In `@src/lib/sort/inline_buffer.rs`:
- Around line 284-287: The public accessors refs_mut(), data(), and
header_size() expose internal inline-buffer invariants; change their visibility
to pub(crate) or to a more specific internal module-level accessor (e.g., make
refs_mut, data, header_size non-pub or pub(in crate::sorter) and/or replace
refs_mut with a controlled API that performs safe mutations (like swap_refs(),
update_ref_at(), or a Sorter-only mutable borrow wrapper) so external code
cannot directly rewrite offsets/lengths or rely on the inline storage layout;
update any tests/uses to go through the new internal-only helpers or the sorter
module API (target symbols: refs_mut, data, header_size).
🪄 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: ce619986-be8b-480e-abe6-b19b7f5b0514
📒 Files selected for processing (26)
CLAUDE.mdbenches/core_functions.rscrates/fgumi-raw-bam/src/cigar.rscrates/fgumi-raw-bam/src/tags.rssrc/commands/clip.rssrc/commands/codec.rssrc/commands/common.rssrc/commands/compare/bams.rssrc/commands/compare/raw_compare.rssrc/commands/dedup.rssrc/commands/downsample.rssrc/commands/duplex_metrics.rssrc/commands/fastq.rssrc/commands/filter.rssrc/commands/merge.rssrc/commands/review.rssrc/commands/simplex.rssrc/commands/sort.rssrc/commands/zipper.rssrc/lib/bam_io.rssrc/lib/progress.rssrc/lib/sort/external.rssrc/lib/sort/inline_buffer.rssrc/lib/sort/keys.rssrc/lib/sort/mod.rssrc/lib/sort/raw.rs
nh13
force-pushed
the
nh/sort-optimizations-v3
branch
from
April 2, 2026 01:45
aa4718b to
f2613ee
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/commands/common.rs (1)
107-108:⚠️ Potential issue | 🟠 MajorRestore
ArgAction::Seton these default-true flags.These options are presence-only now, so users cannot pass
false. That makesoutput-per-base-tagsandconsensus-call-overlapping-basesimpossible to disable, and--queue-memory-per-thread falsenow errors, which also breaks the documented invocations indocs/performance-tuning.mdon Line 45 and Line 52.Proposed fix
- #[arg(short = 'B', long = "output-per-base-tags", default_value = "true")] + #[arg(short = 'B', long = "output-per-base-tags", default_value_t = true, action = clap::ArgAction::Set)] pub output_per_base_tags: bool, @@ - #[arg(long = "consensus-call-overlapping-bases", default_value = "true")] + #[arg(long = "consensus-call-overlapping-bases", default_value_t = true, action = clap::ArgAction::Set)] pub consensus_call_overlapping_bases: bool, @@ - #[arg(long = "queue-memory-per-thread", default_value = "true")] + #[arg(long = "queue-memory-per-thread", default_value_t = true, action = clap::ArgAction::Set)] pub queue_memory_per_thread: bool,Also applies to: 217-218, 454-455
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/common.rs` around lines 107 - 108, The boolean CLI flags output_per_base_tags, consensus_call_overlapping_bases and queue_memory_per_thread were changed to presence-only (preventing users from passing false); restore ArgAction::Set for these arguments so they accept explicit true/false values; locate the struct fields/output_per_base_tags, consensus_call_overlapping_bases, and queue_memory_per_thread in common.rs and change their #[arg(...)] attributes to include arg action = ArgAction::Set (and ensure default_value stays as appropriate) so --flag false works and documented invocations continue to function.
🧹 Nitpick comments (2)
src/lib/sort/raw.rs (2)
2399-2437: Testtest_sort_many_chunks_with_semaphoremay be flaky with small memory limits.200 pairs with 4096-byte limit assumes records are small enough to guarantee ≥2 chunks. If record overhead changes, the assertion
stats.chunks_written >= 2could fail intermittently.Consider increasing
num_pairsor decreasingmemory_limitfurther to ensure the test reliably produces multiple chunks regardless of minor overhead changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/sort/raw.rs` around lines 2399 - 2437, The test test_sort_many_chunks_with_semaphore is brittle because num_pairs = 200 with memory_limit(4096) may not reliably produce >=2 chunks; increase the chance of spilling by either raising num_pairs (e.g., severalx larger) or lowering the memory limit (e.g., to 2048 or smaller) in the test setup where RawExternalSorter::new(...).memory_limit(4096) and the local variable num_pairs are defined so the assertion stats.chunks_written >= 2 becomes deterministic.
483-493:try_next_recordreturn type is potentially confusing.
Option<Option<(K, Vec<u8>)>>has three states:Some(Some(...))= record,Some(None)= EOF,None= would block. Consider a dedicated enum for clarity if this becomes a public API.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/sort/raw.rs` around lines 483 - 493, The current try_next_record signature Option<Option<(K, Vec<u8>)>> is confusing; replace it with a small dedicated enum (e.g., TryRecord::Record(K, Vec<u8>), TryRecord::Eof, TryRecord::WouldBlock) and change pub fn try_next_record(&mut self) -> TryRecord to return those variants; inside try_next_record map receiver.try_recv() results (Ok(Some) => Record, Ok(None)|Disconnected => Eof, Empty => WouldBlock), update the function docstring to describe the three explicit variants, and update all call sites that expect Option<Option<...>> to handle the new enum.
🤖 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/lib/sort/keys.rs`:
- Around line 728-799: The deserialization path in RawQuerynameKey::read_from
fails to guarantee the null-terminated invariant required by unsafe fn
natural_compare_nul; update RawQuerynameKey::read_from to ensure the internal
name buffer is null-terminated (either by calling RawQuerynameKey::new()/the
same constructor used by QuerynameKey::from_record or by appending a NUL byte
after deserializing) so any RawQuerynameKey produced from read_from is safe to
pass to natural_compare_nul.
---
Duplicate comments:
In `@src/commands/common.rs`:
- Around line 107-108: The boolean CLI flags output_per_base_tags,
consensus_call_overlapping_bases and queue_memory_per_thread were changed to
presence-only (preventing users from passing false); restore ArgAction::Set for
these arguments so they accept explicit true/false values; locate the struct
fields/output_per_base_tags, consensus_call_overlapping_bases, and
queue_memory_per_thread in common.rs and change their #[arg(...)] attributes to
include arg action = ArgAction::Set (and ensure default_value stays as
appropriate) so --flag false works and documented invocations continue to
function.
---
Nitpick comments:
In `@src/lib/sort/raw.rs`:
- Around line 2399-2437: The test test_sort_many_chunks_with_semaphore is
brittle because num_pairs = 200 with memory_limit(4096) may not reliably produce
>=2 chunks; increase the chance of spilling by either raising num_pairs (e.g.,
severalx larger) or lowering the memory limit (e.g., to 2048 or smaller) in the
test setup where RawExternalSorter::new(...).memory_limit(4096) and the local
variable num_pairs are defined so the assertion stats.chunks_written >= 2
becomes deterministic.
- Around line 483-493: The current try_next_record signature Option<Option<(K,
Vec<u8>)>> is confusing; replace it with a small dedicated enum (e.g.,
TryRecord::Record(K, Vec<u8>), TryRecord::Eof, TryRecord::WouldBlock) and change
pub fn try_next_record(&mut self) -> TryRecord to return those variants; inside
try_next_record map receiver.try_recv() results (Ok(Some) => Record,
Ok(None)|Disconnected => Eof, Empty => WouldBlock), update the function
docstring to describe the three explicit variants, and update all call sites
that expect Option<Option<...>> to handle the new enum.
🪄 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: 56dfd93f-8047-4ade-ae69-e756ede63870
📒 Files selected for processing (26)
CLAUDE.mdbenches/core_functions.rscrates/fgumi-raw-bam/src/cigar.rscrates/fgumi-raw-bam/src/tags.rssrc/commands/clip.rssrc/commands/codec.rssrc/commands/common.rssrc/commands/compare/bams.rssrc/commands/compare/raw_compare.rssrc/commands/dedup.rssrc/commands/downsample.rssrc/commands/duplex_metrics.rssrc/commands/fastq.rssrc/commands/filter.rssrc/commands/merge.rssrc/commands/review.rssrc/commands/simplex.rssrc/commands/sort.rssrc/commands/zipper.rssrc/lib/bam_io.rssrc/lib/progress.rssrc/lib/sort/external.rssrc/lib/sort/inline_buffer.rssrc/lib/sort/keys.rssrc/lib/sort/mod.rssrc/lib/sort/raw.rs
✅ Files skipped from review due to trivial changes (2)
- CLAUDE.md
- src/commands/compare/bams.rs
🚧 Files skipped from review as they are similar to previous changes (11)
- src/commands/merge.rs
- src/commands/codec.rs
- src/commands/downsample.rs
- src/commands/simplex.rs
- src/lib/bam_io.rs
- src/lib/sort/mod.rs
- crates/fgumi-raw-bam/src/cigar.rs
- src/commands/dedup.rs
- src/commands/duplex_metrics.rs
- src/commands/filter.rs
- src/lib/sort/inline_buffer.rs
nh13
force-pushed
the
nh/sort-optimizations-v3
branch
from
April 2, 2026 02:01
f2613ee to
9af004c
Compare
nh13
force-pushed
the
nh/sort-optimizations-v3
branch
from
April 2, 2026 02:07
9af004c to
cd104cd
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
src/commands/review.rs (1)
186-192: Operator precedence is correct but implicit.Line 188 relies on
&&binding tighter than||. Works fine, but parens would clarify intent.Clarify precedence
- ext_str == "vcf" || ext_str == "gz" && path.to_string_lossy().ends_with(".vcf.gz") + ext_str == "vcf" || (ext_str == "gz" && path.to_string_lossy().ends_with(".vcf.gz"))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/review.rs` around lines 186 - 192, The boolean expression that checks file type is relying on implicit operator precedence (ext_str == "vcf" || ext_str == "gz" && path.to_string_lossy().ends_with(".vcf.gz")); update the condition to make intent explicit by adding parentheses around the gz check (e.g., ext_str == "vcf" || (ext_str == "gz" && path.to_string_lossy().ends_with(".vcf.gz"))) so the logic using ext_str and path.to_string_lossy() is unambiguous; locate this conditional in the file (the block that computes ext_str from path.extension()) and apply the parentheses there.src/lib/sort/keys.rs (1)
984-987:read_fromdoes not null-terminate, but path is dead whenEMBEDDED_IN_RECORD=true.The deserializer constructs
Self { name, flags }directly without callingnew(), so the name lacks the required null terminator. However, sinceEMBEDDED_IN_RECORD=true, the merge path usesextract_from_recordinstead ofread_from(confirmed by context snippet from raw.rs lines 390-440).Still, for defensive correctness if the constant ever changes or if
read_fromis called directly elsewhere:♻️ Use constructor to ensure invariant
fn read_from<R: Read>(reader: &mut R) -> std::io::Result<Self> { let (name, flags) = read_queryname_key(reader)?; - Ok(Self { name, flags }) + Ok(Self::new(name, flags)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/sort/keys.rs` around lines 984 - 987, The deserializer read_from constructs the struct directly (Ok(Self { name, flags })) so the name may be missing the required null terminator; change read_from to use the type's constructor that enforces the invariant (e.g., call new(name, flags) or otherwise ensure name is null-terminated before building the instance) instead of direct struct init, referencing read_from, new(), and the direct Self { name, flags } construction; keep behavior compatible with EMBEDDED_IN_RECORD/extract_from_record but make read_from defensive by guaranteeing the null terminator.crates/fgumi-raw-bam/src/cigar.rs (1)
1895-1936: Add one proptest parity check for long-term safety.Current examples are solid. I’d still add one property test asserting
unclipped_5prime_raw(...) == unclipped_5prime(...)for generated valid CIGAR patterns and strand/position combinations.Suggested test addition
+ use proptest::prelude::*; + + proptest! { + #[test] + fn proptest_unclipped_5prime_raw_matches_vec_path( + pos in 0i32..1_000_000, + is_reverse in any::<bool>(), + lead_soft in 0u32..25, + match_len in 1u32..500, + trail_soft in 0u32..25, + ) { + let mut cigar = Vec::new(); + if lead_soft > 0 { cigar.push(encode_op(4, lead_soft)); } + cigar.push(encode_op(0, match_len)); + if trail_soft > 0 { cigar.push(encode_op(4, trail_soft)); } + + let flags = if is_reverse { flags::REVERSE } else { 0 }; + let read_len = (lead_soft + match_len + trail_soft) as usize; + let rec = make_bam_bytes(0, pos, flags, b"read", &cigar, read_len, -1, -1, &[]); + + let expected = unclipped_5prime(pos, is_reverse, &cigar); + prop_assert_eq!(unclipped_5prime_raw(&rec, pos, is_reverse), expected); + } + }As per coding guidelines, "Use proptest for property-based testing".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/fgumi-raw-bam/src/cigar.rs` around lines 1895 - 1936, Add a proptest that for generated valid CIGARs and strand/pos values asserts unclipped_5prime_raw(&rec, pos, is_reverse) == unclipped_5prime(&rec, pos, is_reverse): use proptest to generate (is_reverse: bool, pos: i32 within valid reference range, cigar: Vec<u32>) where each cigar element is built via encode_op(len, op) with op drawn from valid CIGAR ops (e.g., 0,1,2,3,4,5) and len > 0, construct rec with make_bam_bytes(rec_id, pos, flags (use flags::REVERSE when is_reverse), b"read", &cigar, ...), and assert equality; add this test near the existing unclipped_5prime_raw tests to provide parity coverage.src/commands/clip.rs (1)
1586-1614: New regenerate-tags tests are tautological and miss parser behavior.These tests only assert struct literals and don’t validate CLI parsing compatibility claims. Please switch them to
Clip::try_parse_from(...)cases (omitted flag,--regenerate-tags, and explicit value form).Also applies to: 1710-1739
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/clip.rs` around lines 1586 - 1614, Replace the tautological struct-literal tests with real CLI-parse checks: in test_clip_regenerate_tags_always_true (and the similar test around lines 1710-1739), call Clip::try_parse_from(...) with three scenarios — omit the flag, include "--regenerate-tags", and include "--regenerate-tags=false" — then assert the parsed Clip.regenerate_tags is the expected boolean for each case; use the existing test names (or create distinct test functions) and keep other required args (input/output/reference and required options) so parsing succeeds.src/commands/filter.rs (1)
150-170: Keep parser coverage for the bool flag split.These attributes now mix two
clapbool modes:reverse_per_base_tags/require_single_strand_agreementare presence flags, whilefilter_by_templatestill accepts an explicit false value. The tests below buildFilterdirectly, so they won't catch a parsing regression here. Please add a smallrstestmatrix aroundtry_parse_fromfor--reverse-per-base-tags,--require-single-strand-agreement, and--filter-by-template=false. As per coding guidelines,**/*.rs: Use rstest for parameterized tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/filter.rs` around lines 150 - 170, Add an rstest parameterized parsing test that exercises clap parsing via Filter::try_parse_from for the three flags --reverse-per-base-tags, --require-single-strand-agreement, and --filter-by-template (including the explicit false form `--filter-by-template=false`) to ensure presence-mode vs explicit-value behavior is preserved; create a small rstest matrix with combinations of those three flags (true/false for filter-by-template, present/absent for the two presence flags) and assert the resulting Filter instance fields reverse_per_base_tags, require_single_strand_agreement, and filter_by_template match the expected booleans after calling Filter::try_parse_from.src/lib/progress.rs (2)
381-389: Userstestfor the duration table.This is a parameterized test written as repeated assertions. Converting it keeps new cases cheap and matches repo convention.
Possible refactor
- #[test] - fn test_fmt_duration() { - assert_eq!(fmt_duration(0.0), "0s"); - assert_eq!(fmt_duration(59.0), "59s"); - assert_eq!(fmt_duration(59.5), "1m"); - assert_eq!(fmt_duration(90.0), "1m 30s"); - assert_eq!(fmt_duration(3600.0), "1h"); - assert_eq!(fmt_duration(5400.0), "1h 30m"); - } + #[rstest] + #[case(0.0, "0s")] + #[case(59.0, "59s")] + #[case(59.5, "1m")] + #[case(90.0, "1m 30s")] + #[case(3600.0, "1h")] + #[case(5400.0, "1h 30m")] + fn test_fmt_duration(#[case] secs: f64, #[case] expected: &str) { + assert_eq!(fmt_duration(secs), expected); + }Also add
use rstest::rstest;near Line 265.As per coding guidelines, "Use rstest for parameterized tests".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/progress.rs` around lines 381 - 389, Replace the repetitive assertions in the test function test_fmt_duration with a parameterized rstest table that iterates inputs and expected outputs for fmt_duration; add the attribute #[rstest] to the test, convert the function signature to accept parameters (e.g., input: f64, expected: &str), and provide the cases for (0.0, "0s"), (59.0, "59s"), (59.5, "1m"), (90.0, "1m 30s"), (3600.0, "1h"), (5400.0, "1h 30m"); also add use rstest::rstest; alongside the other imports so the macro is available and keep the test name test_fmt_duration and function fmt_duration references intact.
391-405: Make the EMA test deterministic.Sleeping for 10ms only proves
update()returned something positive. It does not pin the bias-correction math, and it adds avoidable timing variance in CI. Seed the EMA state directly and assert the corrected value.Possible refactor
#[test] fn test_ema_bias_correction() { let mut ema = EmaState::new(); // With zero calls, corrected rate should be 0 assert!(ema.corrected_rate().abs() < f64::EPSILON); - // After first update, corrected rate equals instantaneous rate - // (bias correction factor is 1/(1-0.7^1) = 1/0.3 = 3.33, - // and smoothed_rate = 0.3 * rate, so corrected = rate) - std::thread::sleep(std::time::Duration::from_millis(10)); - ema.last_count = 0; - let rate = ema.update(1000); - assert!(rate > 0.0, "rate should be positive after first update"); + // After one update, bias correction should recover the original rate. + ema.smoothed_rate = EMA_ALPHA * 1000.0; + ema.calls = 1; + assert!((ema.corrected_rate() - 1000.0).abs() < 1e-9); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/progress.rs` around lines 391 - 405, The test test_ema_bias_correction is nondeterministic because it relies on sleeping; instead seed the EmaState internals so the bias-correction math is testable without time. Remove the sleep and directly initialize the EmaState (use EmaState::new()), set the relevant internal fields (e.g. last_count = 0, alpha = 0.7 or the struct's smoothing parameter, smoothed_rate = 0.0 and any call/cycle counter to represent "first update") so that a deterministic call to update(1000) yields a known instantaneous rate and bias-corrected value; then assert corrected_rate() equals the expected corrected value (or that update(...) > 0 and corrected_rate() == expected within EPSILON). Locate these changes around the test function name test_ema_bias_correction and methods EmaState::new, update(), and corrected_rate() to make the test deterministic and remove timing/sleep.
🤖 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/lib/progress.rs`:
- Around line 170-172: The with_total(mut self, total: u64) setter currently
accepts 0 which later causes division by zero and bad percentages/ETA; change
its behavior to treat 0 as absent by setting self.total = None when total == 0
(otherwise Some(total)), or alternatively return a Result and reject zero —
implement the normalize-to-None approach in the with_total method of the
Progress type so callers passing 0 get the same semantics as not setting a
total.
---
Nitpick comments:
In `@crates/fgumi-raw-bam/src/cigar.rs`:
- Around line 1895-1936: Add a proptest that for generated valid CIGARs and
strand/pos values asserts unclipped_5prime_raw(&rec, pos, is_reverse) ==
unclipped_5prime(&rec, pos, is_reverse): use proptest to generate (is_reverse:
bool, pos: i32 within valid reference range, cigar: Vec<u32>) where each cigar
element is built via encode_op(len, op) with op drawn from valid CIGAR ops
(e.g., 0,1,2,3,4,5) and len > 0, construct rec with make_bam_bytes(rec_id, pos,
flags (use flags::REVERSE when is_reverse), b"read", &cigar, ...), and assert
equality; add this test near the existing unclipped_5prime_raw tests to provide
parity coverage.
In `@src/commands/clip.rs`:
- Around line 1586-1614: Replace the tautological struct-literal tests with real
CLI-parse checks: in test_clip_regenerate_tags_always_true (and the similar test
around lines 1710-1739), call Clip::try_parse_from(...) with three scenarios —
omit the flag, include "--regenerate-tags", and include
"--regenerate-tags=false" — then assert the parsed Clip.regenerate_tags is the
expected boolean for each case; use the existing test names (or create distinct
test functions) and keep other required args (input/output/reference and
required options) so parsing succeeds.
In `@src/commands/filter.rs`:
- Around line 150-170: Add an rstest parameterized parsing test that exercises
clap parsing via Filter::try_parse_from for the three flags
--reverse-per-base-tags, --require-single-strand-agreement, and
--filter-by-template (including the explicit false form
`--filter-by-template=false`) to ensure presence-mode vs explicit-value behavior
is preserved; create a small rstest matrix with combinations of those three
flags (true/false for filter-by-template, present/absent for the two presence
flags) and assert the resulting Filter instance fields reverse_per_base_tags,
require_single_strand_agreement, and filter_by_template match the expected
booleans after calling Filter::try_parse_from.
In `@src/commands/review.rs`:
- Around line 186-192: The boolean expression that checks file type is relying
on implicit operator precedence (ext_str == "vcf" || ext_str == "gz" &&
path.to_string_lossy().ends_with(".vcf.gz")); update the condition to make
intent explicit by adding parentheses around the gz check (e.g., ext_str ==
"vcf" || (ext_str == "gz" && path.to_string_lossy().ends_with(".vcf.gz"))) so
the logic using ext_str and path.to_string_lossy() is unambiguous; locate this
conditional in the file (the block that computes ext_str from path.extension())
and apply the parentheses there.
In `@src/lib/progress.rs`:
- Around line 381-389: Replace the repetitive assertions in the test function
test_fmt_duration with a parameterized rstest table that iterates inputs and
expected outputs for fmt_duration; add the attribute #[rstest] to the test,
convert the function signature to accept parameters (e.g., input: f64, expected:
&str), and provide the cases for (0.0, "0s"), (59.0, "59s"), (59.5, "1m"),
(90.0, "1m 30s"), (3600.0, "1h"), (5400.0, "1h 30m"); also add use
rstest::rstest; alongside the other imports so the macro is available and keep
the test name test_fmt_duration and function fmt_duration references intact.
- Around line 391-405: The test test_ema_bias_correction is nondeterministic
because it relies on sleeping; instead seed the EmaState internals so the
bias-correction math is testable without time. Remove the sleep and directly
initialize the EmaState (use EmaState::new()), set the relevant internal fields
(e.g. last_count = 0, alpha = 0.7 or the struct's smoothing parameter,
smoothed_rate = 0.0 and any call/cycle counter to represent "first update") so
that a deterministic call to update(1000) yields a known instantaneous rate and
bias-corrected value; then assert corrected_rate() equals the expected corrected
value (or that update(...) > 0 and corrected_rate() == expected within EPSILON).
Locate these changes around the test function name test_ema_bias_correction and
methods EmaState::new, update(), and corrected_rate() to make the test
deterministic and remove timing/sleep.
In `@src/lib/sort/keys.rs`:
- Around line 984-987: The deserializer read_from constructs the struct directly
(Ok(Self { name, flags })) so the name may be missing the required null
terminator; change read_from to use the type's constructor that enforces the
invariant (e.g., call new(name, flags) or otherwise ensure name is
null-terminated before building the instance) instead of direct struct init,
referencing read_from, new(), and the direct Self { name, flags } construction;
keep behavior compatible with EMBEDDED_IN_RECORD/extract_from_record but make
read_from defensive by guaranteeing the null terminator.
🪄 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: 049e0502-4970-493e-b1a9-c333bea0f7e4
📒 Files selected for processing (24)
CLAUDE.mdbenches/core_functions.rscrates/fgumi-raw-bam/src/cigar.rscrates/fgumi-raw-bam/src/tags.rssrc/commands/clip.rssrc/commands/common.rssrc/commands/compare/bams.rssrc/commands/compare/raw_compare.rssrc/commands/dedup.rssrc/commands/downsample.rssrc/commands/duplex_metrics.rssrc/commands/fastq.rssrc/commands/filter.rssrc/commands/merge.rssrc/commands/review.rssrc/commands/sort.rssrc/commands/zipper.rssrc/lib/bam_io.rssrc/lib/progress.rssrc/lib/sort/external.rssrc/lib/sort/inline_buffer.rssrc/lib/sort/keys.rssrc/lib/sort/mod.rssrc/lib/sort/raw.rs
✅ Files skipped from review due to trivial changes (2)
- CLAUDE.md
- src/lib/bam_io.rs
🚧 Files skipped from review as they are similar to previous changes (10)
- src/commands/downsample.rs
- src/commands/merge.rs
- src/lib/sort/mod.rs
- src/commands/duplex_metrics.rs
- src/commands/compare/bams.rs
- src/commands/compare/raw_compare.rs
- crates/fgumi-raw-bam/src/tags.rs
- src/lib/sort/inline_buffer.rs
- src/commands/dedup.rs
- src/lib/sort/raw.rs
nh13
force-pushed
the
nh/sort-optimizations-v3
branch
from
April 2, 2026 02:20
cd104cd to
798850f
Compare
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/commands/clip.rs (1)
114-117:⚠️ Potential issue | 🟠 Major
--regenerate-tagsis not backward-compatible as written.Line 116 says this flag is kept for compatibility, but the current derive shape is still
SetTrue, so--regenerate-tags=falsewill not parse. Line 213 also logs it like an effective knob even though tag regeneration is unconditional. (docs.rs)Proposed fix
- #[arg(long = "regenerate-tags", default_value = "true", hide = true)] + #[arg( + long = "regenerate-tags", + default_value_t = true, + num_args = 0..=1, + default_missing_value = "true", + action = clap::ArgAction::Set, + hide = true + )] pub regenerate_tags: bool, @@ - info!(" Regenerate tags: {}", self.regenerate_tags); + info!(" Regenerate tags: always enabled");The value-taking hybrid requires
default_missing_valuewithnum_args(0..=1). (docs.rs)Also applies to: 213-213
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/clip.rs` around lines 114 - 117, The flag `regenerate_tags` is declared as a boolean SetTrue in the struct but is supposed to accept explicit true/false for backward compatibility; change the clap arg configuration on `regenerate_tags` to a value-taking hybrid by adding default_missing_value="true" and num_args(0..=1) (keep default_value="true" and hide=true) so `--regenerate-tags=false` parses. Also update the log site that references `regenerate_tags` (the log at/around where it's printed at line ~213) to clarify that tag regeneration is unconditional/ignored (or remove the misleading “effective knob” wording) so the log doesn't imply the flag controls behavior.
🧹 Nitpick comments (3)
crates/fgumi-raw-bam/src/cigar.rs (1)
251-254: Clarify semantic differences vsunclipped_5prime_from_raw_bam.The docs currently imply close equivalence, but this function returns
poson no-CIGAR/truncation (and doesn’t handle unmapped sentinel semantics the same way). Please call out these differences explicitly to prevent misuse.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/fgumi-raw-bam/src/cigar.rs` around lines 251 - 254, Update the doc comment to explicitly state how this function differs from unclipped_5prime_from_raw_bam: note that it expects pre-extracted pos and is_reverse (avoiding field reads), that on no-CIGAR or truncated CIGAR it simply returns the input pos unchanged (rather than applying the unmapped sentinel semantics), and that it does not interpret or translate unmapped sentinel values the same way unclipped_5prime_from_raw_bam does; mention the exact function names unclipped_5prime_from_raw_bam and this function (the one with pre-extracted pos/is_reverse) so callers know to choose the correct variant.src/lib/sort/raw.rs (1)
2108-2116: Add explicit lexicographic comparator test coverage.These tests use
QuerynameComparator::default()only. Please add explicitNaturalandLexicographiccases so comparator behavior doesn’t silently depend on default choice.Also applies to: 2644-2664, 2696-2716
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/sort/raw.rs` around lines 2108 - 2116, The current unit tests (e.g., test_create_output_header_queryname) only exercise QuerynameComparator::default(), leaving comparator behavior untested for explicit Natural and Lexicographic modes; update the tests that construct a RawExternalSorter via RawExternalSorter::new(SortOrder::Queryname(QuerynameComparator::default())) to add two additional assertions/cases that construct the sorter with SortOrder::Queryname(QuerynameComparator::Natural) and SortOrder::Queryname(QuerynameComparator::Lexicographic), call create_output_header(&header) for each, and assert the HD.SO field equals b"natural" and b"lexicographic" respectively; repeat the same addition for the other test blocks that follow the same pattern (the similar test functions around the other ranges) so all comparator variants are explicitly covered.src/commands/clip.rs (1)
1586-1614: These tests miss the parser path.Lines 1586-1739 only assert hand-built structs, so they will not catch the clap parsing change above. A small
Clip::try_parse_from(...)table over absent / bare / explicit-value forms would cover the actual contract. (docs.rs)Based on learnings Applies to **/*.rs : Use rstest for parameterized tests.
Also applies to: 1710-1739
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/clip.rs` around lines 1586 - 1614, The current test test_clip_regenerate_tags_always_true only constructs a Clip struct directly and misses exercising the clap parser; update the test (or add a new parameterized test using rstest) to call Clip::try_parse_from(...) with cases for absent flag, bare flag (e.g. "--regenerate-tags"), and explicit values to ensure parsing preserves regenerate_tags == true; reference the Clip struct, Clip::try_parse_from, and the test name test_clip_regenerate_tags_always_true when locating where to add/replace the assertions and use rstest for the input table to cover all parser permutations.
🤖 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/clip.rs`:
- Line 103: The bool CLI flags using default_value = "false" (e.g., the
attributes for --clip-overlapping-reads, --clip-bases-past-mate,
--upgrade-clipping, and --auto-clip-attributes in clip.rs) currently use the
derive macro's SetTrue behavior and reject explicit false values; update each
#[arg(...)] for those flags to the pattern used in filter.rs/sort.rs by adding
action = clap::ArgAction::Set, num_args = 0..=1, default_missing_value = "true"
(keep default_value = "false" if present) so the flags accept both bare usage
and explicit true/false values.
In `@src/commands/common.rs`:
- Line 111: The --trim flag was changed to a plain boolean and lost the previous
boolean-flag parsing semantics from commit 481f2d4; restore the attribute
pattern used by --output-per-base-tags so --trim supports forms like "--trim",
"--trim=true", and "--trim=false" by setting action = clap::ArgAction::Set,
num_args = 0..=1 and default_missing_value = "true" on the same arg (the
attribute on the --trim arg in src/commands/common.rs), and re-add the unit
tests that covered those parsing cases from 481f2d4 to ensure "--trim=false" and
"--trim true" invocations are validated.
In `@src/lib/sort/raw.rs`:
- Around line 1651-1654: When initial_keys.is_empty() causes an early return, no
output BAM is created; instead, use the same output creation path to emit an
empty BAM file (write the header and finalize/close the writer) before returning
Ok(0). Locate the branch using initial_keys in src/lib/sort/raw.rs and, rather
than returning immediately, instantiate the output writer (the same writer used
elsewhere in the merge routine), write the BAM header (and any required empty
structures/index) and finalize the file, then return Ok(0).
---
Duplicate comments:
In `@src/commands/clip.rs`:
- Around line 114-117: The flag `regenerate_tags` is declared as a boolean
SetTrue in the struct but is supposed to accept explicit true/false for backward
compatibility; change the clap arg configuration on `regenerate_tags` to a
value-taking hybrid by adding default_missing_value="true" and num_args(0..=1)
(keep default_value="true" and hide=true) so `--regenerate-tags=false` parses.
Also update the log site that references `regenerate_tags` (the log at/around
where it's printed at line ~213) to clarify that tag regeneration is
unconditional/ignored (or remove the misleading “effective knob” wording) so the
log doesn't imply the flag controls behavior.
---
Nitpick comments:
In `@crates/fgumi-raw-bam/src/cigar.rs`:
- Around line 251-254: Update the doc comment to explicitly state how this
function differs from unclipped_5prime_from_raw_bam: note that it expects
pre-extracted pos and is_reverse (avoiding field reads), that on no-CIGAR or
truncated CIGAR it simply returns the input pos unchanged (rather than applying
the unmapped sentinel semantics), and that it does not interpret or translate
unmapped sentinel values the same way unclipped_5prime_from_raw_bam does;
mention the exact function names unclipped_5prime_from_raw_bam and this function
(the one with pre-extracted pos/is_reverse) so callers know to choose the
correct variant.
In `@src/commands/clip.rs`:
- Around line 1586-1614: The current test test_clip_regenerate_tags_always_true
only constructs a Clip struct directly and misses exercising the clap parser;
update the test (or add a new parameterized test using rstest) to call
Clip::try_parse_from(...) with cases for absent flag, bare flag (e.g.
"--regenerate-tags"), and explicit values to ensure parsing preserves
regenerate_tags == true; reference the Clip struct, Clip::try_parse_from, and
the test name test_clip_regenerate_tags_always_true when locating where to
add/replace the assertions and use rstest for the input table to cover all
parser permutations.
In `@src/lib/sort/raw.rs`:
- Around line 2108-2116: The current unit tests (e.g.,
test_create_output_header_queryname) only exercise
QuerynameComparator::default(), leaving comparator behavior untested for
explicit Natural and Lexicographic modes; update the tests that construct a
RawExternalSorter via
RawExternalSorter::new(SortOrder::Queryname(QuerynameComparator::default())) to
add two additional assertions/cases that construct the sorter with
SortOrder::Queryname(QuerynameComparator::Natural) and
SortOrder::Queryname(QuerynameComparator::Lexicographic), call
create_output_header(&header) for each, and assert the HD.SO field equals
b"natural" and b"lexicographic" respectively; repeat the same addition for the
other test blocks that follow the same pattern (the similar test functions
around the other ranges) so all comparator variants are explicitly covered.
🪄 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: 8298cc5a-f977-44f7-9124-f704790c5d9d
📒 Files selected for processing (24)
CLAUDE.mdbenches/core_functions.rscrates/fgumi-raw-bam/src/cigar.rscrates/fgumi-raw-bam/src/tags.rssrc/commands/clip.rssrc/commands/common.rssrc/commands/compare/bams.rssrc/commands/compare/raw_compare.rssrc/commands/dedup.rssrc/commands/downsample.rssrc/commands/duplex_metrics.rssrc/commands/fastq.rssrc/commands/filter.rssrc/commands/merge.rssrc/commands/review.rssrc/commands/sort.rssrc/commands/zipper.rssrc/lib/bam_io.rssrc/lib/progress.rssrc/lib/sort/external.rssrc/lib/sort/inline_buffer.rssrc/lib/sort/keys.rssrc/lib/sort/mod.rssrc/lib/sort/raw.rs
✅ Files skipped from review due to trivial changes (4)
- src/lib/sort/mod.rs
- CLAUDE.md
- src/commands/duplex_metrics.rs
- benches/core_functions.rs
🚧 Files skipped from review as they are similar to previous changes (11)
- src/commands/compare/raw_compare.rs
- src/commands/downsample.rs
- src/commands/fastq.rs
- src/commands/merge.rs
- src/lib/bam_io.rs
- src/commands/review.rs
- src/commands/compare/bams.rs
- src/lib/sort/inline_buffer.rs
- src/commands/dedup.rs
- src/commands/filter.rs
- src/commands/sort.rs
nh13
force-pushed
the
nh/sort-optimizations-v3
branch
from
April 2, 2026 18:36
798850f to
f8288d5
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
src/lib/progress.rs (1)
391-405: Test relies on wall-clock timing.
sleep(10ms)at line 401 could flake on heavily loaded CI runners. Consider injecting a mock clock or accepting that this test has low (but nonzero) flakiness risk.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/progress.rs` around lines 391 - 405, The test test_ema_bias_correction relies on wall-clock sleep which can flake; modify EmaState (or its API) to accept a testable time source so tests can control elapsed time deterministically: add a Clock/TimeProvider trait or an update_with_interval(delta_ms) helper and use that in the test instead of std::thread::sleep; reference EmaState, update, last_count, and corrected_rate so you can either inject the clock into EmaState::new or add a new method (e.g., update_with_interval or set_last_instant) and update the test to call that deterministic method to simulate 10ms elapsed.benches/core_functions.rs (2)
1047-1054: Safety assumption documented inline would help.Pointer arithmetic relies on
safe_prefix ≤ min(name lengths). Comment at call site or here clarifies precondition.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@benches/core_functions.rs` around lines 1047 - 1054, The function sort_natural_nul_digit_safe calls unsafe pointer arithmetic via raw_nul[a].as_ptr().add(safe_prefix) and raw_nul[b].as_ptr().add(safe_prefix) without documenting the precondition; add a short safety comment in the sort_natural_nul_digit_safe function (and/or at its call sites) stating the required invariant that safe_prefix ≤ min(len) for all entries in raw_nul so callers know they must ensure the buffer lengths before this unsafe add, and mention that natural_compare_nul expects NUL-terminated byte slices starting at that offset.
476-520: Redundant innerunsafeblock.Function is already
unsafe, so the inner block is unnecessary.Suggested fix
unsafe fn strnum_cmp_samtools(a: *const u8, b: *const u8) -> i32 { - unsafe { - let mut pa = a; - ... - } + let mut pa = a; + ... }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@benches/core_functions.rs` around lines 476 - 520, The inner redundant unsafe block inside the already-unsafe function strnum_cmp_samtools should be removed: delete the inner "unsafe { ... }" wrapper and move its contents to the function body (preserving all logic, variables pa/pb, loops and return points) so the function remains unsafe but no nested unsafe block is present; ensure indentation and scoping remain correct and all return expressions are unchanged.crates/fgumi-raw-bam/src/tags.rs (1)
282-286: API signature inconsistency withextract_aux_string_tags.
extract_aux_string_tagstakesaux_datadirectly; this takes a full BAM record (bam). The naming doesn't signal this difference clearly — callers could accidentally pass aux bytes expecting similar behavior.Consider either:
- Renaming to
extract_template_aux_tags_from_record, or- Adding a sibling that takes aux_data directly for consistency
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/fgumi-raw-bam/src/tags.rs` around lines 282 - 286, The function extract_template_aux_tags currently accepts a full BAM record (bam: &[u8]) while extract_aux_string_tags accepts aux_data bytes directly, causing API inconsistency and potential misuse; either rename extract_template_aux_tags to extract_template_aux_tags_from_record and document that it calls aux_data_slice(bam) (and update all call sites), or add a new sibling function (e.g., extract_template_aux_tags_from_aux_data) that accepts aux_data: &[u8] and reuses the core logic so both styles are available; make sure to keep extract_aux_string_tags usage consistent and update any callers to the new/renamed function names.src/commands/sort.rs (1)
876-894: Table-drive the order pass cases withrstest, and includequeryname::lex.These tests differ only by
order_str, and the short alias still is not exercised through the full parser → sorter → verifier path. One table would keep the accepted spellings in one place.As per coding guidelines,
**/*.rs: Use rstest for parameterized tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/sort.rs` around lines 876 - 894, Replace the four near-identical test functions (test_verify_coordinate_sorted_pass, test_verify_queryname_default_sorted_pass, test_verify_queryname_lexicographic_sorted_pass, test_verify_queryname_natural_sorted_pass) with a single parameterized rstest that calls sort_and_verify_pass for each order_str value (include "coordinate", "queryname", "queryname::lexicographic", "queryname::natural" and also the short alias "queryname::lex"); use the rstest attribute to table-drive the inputs so the full parser→sorter→verifier path exercises the short alias as well and removes duplicated test functions.
🤖 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 277-361: Add comprehensive unit tests for
extract_template_aux_tags to exercise all branches: create parameterized rstest
cases that supply bam aux byte slices covering (1) all tags present to trigger
the early-return bitmask, (2) MI as Z-type (with and without extra suffix
bytes), (3) MI as integer values including positive and negative, (4) cell_tag =
None and Some([u8;2]) to verify cell extraction, (5) non-Z tags mixed between
other tags, and (6) truncated/malformed aux_data to ensure the function returns
safely; assert returned TemplateAuxTags fields (mi, rg, cell, mc) match expected
values for each case and include cases that validate the early-exit based on
target_bits.
In `@src/commands/sort.rs`:
- Around line 117-123: The help text for the --order option is missing the
accepted alias "queryname::lex" and must be updated to match the parser; update
the descriptions where --order help enumerates queryname variants to include
"queryname::lex" alongside "queryname" and "queryname::lexicographic" (and
likewise add "queryname::lex" in the second help block), so the documented
accepted values match the parser that accepts queryname::lex; locate the --order
help strings in src/commands/sort.rs (the blocks describing "queryname",
"queryname::lexicographic", and the later repeated block) and add the
"queryname::lex" token to those help lines.
- Around line 231-232: The --write-index flag currently can be passed alongside
verify and gets silently ignored; update the argument definition for the
write_index field to declare a conflict with the verify flag (e.g. add
conflicts_with = "verify" to the #[arg(...)] attribute for write_index) so clap
will reject the combination up front, or alternatively add an explicit runtime
check before calling execute_verify() (if self.verify && self.write_index {
return Err(...) }) to fail early; reference the write_index field and the
execute_verify() code path when making this change.
In `@src/lib/sort/external.rs`:
- Around line 556-563: The test test_create_output_header_queryname must be
updated to assert the new SUBSORT_ORDER (SS) header when SortOrder::Queryname is
used: check that header_tag::SUBSORT_ORDER equals "lexicographic" in addition to
asserting header_tag::SORT_ORDER == "queryname"; also add a separate test case
using the "natural" comparator to cover the branch where header_ss_tag() returns
that value so the SortOrder::Queryname branch emitting SS is exercised (these
relate to the code path in SortOrder::Queryname and header_tag::SUBSORT_ORDER).
---
Nitpick comments:
In `@benches/core_functions.rs`:
- Around line 1047-1054: The function sort_natural_nul_digit_safe calls unsafe
pointer arithmetic via raw_nul[a].as_ptr().add(safe_prefix) and
raw_nul[b].as_ptr().add(safe_prefix) without documenting the precondition; add a
short safety comment in the sort_natural_nul_digit_safe function (and/or at its
call sites) stating the required invariant that safe_prefix ≤ min(len) for all
entries in raw_nul so callers know they must ensure the buffer lengths before
this unsafe add, and mention that natural_compare_nul expects NUL-terminated
byte slices starting at that offset.
- Around line 476-520: The inner redundant unsafe block inside the
already-unsafe function strnum_cmp_samtools should be removed: delete the inner
"unsafe { ... }" wrapper and move its contents to the function body (preserving
all logic, variables pa/pb, loops and return points) so the function remains
unsafe but no nested unsafe block is present; ensure indentation and scoping
remain correct and all return expressions are unchanged.
In `@crates/fgumi-raw-bam/src/tags.rs`:
- Around line 282-286: The function extract_template_aux_tags currently accepts
a full BAM record (bam: &[u8]) while extract_aux_string_tags accepts aux_data
bytes directly, causing API inconsistency and potential misuse; either rename
extract_template_aux_tags to extract_template_aux_tags_from_record and document
that it calls aux_data_slice(bam) (and update all call sites), or add a new
sibling function (e.g., extract_template_aux_tags_from_aux_data) that accepts
aux_data: &[u8] and reuses the core logic so both styles are available; make
sure to keep extract_aux_string_tags usage consistent and update any callers to
the new/renamed function names.
In `@src/commands/sort.rs`:
- Around line 876-894: Replace the four near-identical test functions
(test_verify_coordinate_sorted_pass, test_verify_queryname_default_sorted_pass,
test_verify_queryname_lexicographic_sorted_pass,
test_verify_queryname_natural_sorted_pass) with a single parameterized rstest
that calls sort_and_verify_pass for each order_str value (include "coordinate",
"queryname", "queryname::lexicographic", "queryname::natural" and also the short
alias "queryname::lex"); use the rstest attribute to table-drive the inputs so
the full parser→sorter→verifier path exercises the short alias as well and
removes duplicated test functions.
In `@src/lib/progress.rs`:
- Around line 391-405: The test test_ema_bias_correction relies on wall-clock
sleep which can flake; modify EmaState (or its API) to accept a testable time
source so tests can control elapsed time deterministically: add a
Clock/TimeProvider trait or an update_with_interval(delta_ms) helper and use
that in the test instead of std::thread::sleep; reference EmaState, update,
last_count, and corrected_rate so you can either inject the clock into
EmaState::new or add a new method (e.g., update_with_interval or
set_last_instant) and update the test to call that deterministic method to
simulate 10ms elapsed.
🪄 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: ee3a9495-f3fd-4085-8e86-61fa1cceaa62
📒 Files selected for processing (24)
CLAUDE.mdbenches/core_functions.rscrates/fgumi-raw-bam/src/cigar.rscrates/fgumi-raw-bam/src/tags.rssrc/commands/clip.rssrc/commands/common.rssrc/commands/compare/bams.rssrc/commands/compare/raw_compare.rssrc/commands/dedup.rssrc/commands/downsample.rssrc/commands/duplex_metrics.rssrc/commands/fastq.rssrc/commands/filter.rssrc/commands/merge.rssrc/commands/review.rssrc/commands/sort.rssrc/commands/zipper.rssrc/lib/bam_io.rssrc/lib/progress.rssrc/lib/sort/external.rssrc/lib/sort/inline_buffer.rssrc/lib/sort/keys.rssrc/lib/sort/mod.rssrc/lib/sort/raw.rs
💤 Files with no reviewable changes (1)
- src/commands/common.rs
✅ Files skipped from review due to trivial changes (3)
- CLAUDE.md
- src/commands/downsample.rs
- src/lib/bam_io.rs
🚧 Files skipped from review as they are similar to previous changes (12)
- src/commands/compare/raw_compare.rs
- src/commands/merge.rs
- src/lib/sort/mod.rs
- src/commands/review.rs
- src/commands/compare/bams.rs
- src/commands/fastq.rs
- src/commands/duplex_metrics.rs
- src/commands/dedup.rs
- src/lib/sort/inline_buffer.rs
- src/commands/clip.rs
- src/lib/sort/raw.rs
- src/lib/sort/keys.rs
nh13
force-pushed
the
nh/sort-optimizations-v3
branch
from
April 2, 2026 19:31
f8288d5 to
fd62ec3
Compare
nh13
force-pushed
the
nh/sort-optimizations-v3
branch
from
April 2, 2026 19:55
fd62ec3 to
28d0598
Compare
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
nh13
force-pushed
the
nh/sort-optimizations-v3
branch
from
April 2, 2026 22:39
af62797 to
987b225
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
crates/fgumi-raw-bam/src/tags.rs (1)
334-343:⚠️ Potential issue | 🟠 MajorHandle overlapping tag aliases with independent checks.
At Line 337, the
else ifchain makes matches mutually exclusive. Ifcell_tagisRGorMC,cellwon’t be set from that same aux entry, changing template key extraction semantics.Suggested fix
- if t == *b"RG" { - result.rg = Some(value); - found |= 2; - } else if cell_tag.is_some_and(|ct| t == *ct) { - result.cell = Some(value); - found |= 4; - } else if t == *b"MC" { - result.mc = std::str::from_utf8(value).ok(); - found |= 8; - } + if t == *b"RG" { + result.rg = Some(value); + found |= 2; + } + if cell_tag.is_some_and(|ct| t == *ct) { + result.cell = Some(value); + found |= 4; + } + if t == *b"MC" { + result.mc = std::str::from_utf8(value).ok(); + found |= 8; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/fgumi-raw-bam/src/tags.rs` around lines 334 - 343, The current else-if chain in the tag parsing logic (checking t == b"RG", cell_tag.is_some_and(|ct| t == *ct), and t == b"MC") makes these matches mutually exclusive so a single aux entry that should set multiple fields (e.g., when cell_tag equals "RG" or "MC") only sets the first branch; change the chain in the function that populates result (the blocks that assign result.rg, result.cell, result.mc and manipulate found) so each condition is evaluated independently (use separate if statements rather than else if), ensuring each matching condition sets its corresponding field and updates the found bitmask appropriately.src/commands/sort.rs (1)
175-180:⚠️ Potential issue | 🟡 MinorDocument and pin the
queryname::lexalias.The parser accepts
queryname::lex, but the per-arg--orderhelp still omits it and the parse tests only coverqueryname/queryname::lexicographic. Add the alias here and one regression case for it.Also applies to: 635-654
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/sort.rs` around lines 175 - 180, The doc comment for the queryname sub-sort should list the accepted alias "queryname::lex" and pin it as equivalent to lexicographic; update the triple-slash comment above the --order arg (the block that currently lists `queryname::lexicographic` and `queryname::natural`) to include `queryname::lex` as an explicit alias, and add a regression test that passes `--order queryname::lex` exercising SortOrderArg::parse so the parser behavior is covered; ensure the other identical queryname doc block is updated too (the duplicate block handled by the same SortOrderArg parsing logic).
🧹 Nitpick comments (1)
src/lib/progress.rs (1)
383-391: Userstestfor this case table.This is a parameterized test written as repeated assertions. Please switch it to
rstestto match the repo test style.As per coding guidelines, `**/*.rs`: Use rstest for parameterized tests.♻️ Suggested rewrite
- #[test] - fn test_fmt_duration() { - assert_eq!(fmt_duration(0.0), "0s"); - assert_eq!(fmt_duration(59.0), "59s"); - assert_eq!(fmt_duration(59.5), "1m"); - assert_eq!(fmt_duration(90.0), "1m 30s"); - assert_eq!(fmt_duration(3600.0), "1h"); - assert_eq!(fmt_duration(5400.0), "1h 30m"); - } + #[rstest] + #[case(0.0, "0s")] + #[case(59.0, "59s")] + #[case(59.5, "1m")] + #[case(90.0, "1m 30s")] + #[case(3600.0, "1h")] + #[case(5400.0, "1h 30m")] + fn test_fmt_duration(#[case] secs: f64, #[case] expected: &str) { + assert_eq!(fmt_duration(secs), expected); + }Also add
use rstest::rstest;near the other test imports.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/progress.rs` around lines 383 - 391, Replace the repeated-assertion unit test test_fmt_duration with an rstest parameterized case table: add use rstest::rstest to the test imports, annotate the test with #[rstest], and declare parameters (input, expected) with a #[case(...)] for each of the six examples, then call fmt_duration(input) and assert_eq! against expected inside the test body; keep the test function name test_fmt_duration and reference the fmt_duration function so CI/style matches repo guidelines.
🤖 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/lib/progress.rs`:
- Around line 241-250: The early return in Progress::log_final that bails out
when count == 0 suppresses the final completion log for zero-record runs; remove
that early return so the existing completion branch (checking self.total and
using info! with fmt_duration) and the fallback log_if_needed(0) path can still
run for a zero count. Update the implementation in log_final (referencing the
method name log_final and fields count, total, start_time, message, and helper
fmt_duration/log_if_needed) to allow emitting the completion message for empty
or fully-filtered inputs, and add a regression test that simulates a zero-count
run (the case exercised by src/lib/sort/raw.rs calling progress.log_final())
asserting the completion log is produced.
In `@src/lib/sort/radix.rs`:
- Around line 334-378: Replace the unsafe test-only radix implementation by
using a safe oracle: remove or stop exercising radix_sort_u64 in tests and
instead sort copies of the test data with the standard library (e.g.,
Vec::sort_by_key or slice::sort_by_key) or call the existing safe function
radix_sort_coordinate_adaptive as the canonical reference; update tests that
currently call radix_sort_u64 to compare results against the safe oracle so no
project unsafe code (radix_sort_u64) is required under #[cfg(test)].
---
Duplicate comments:
In `@crates/fgumi-raw-bam/src/tags.rs`:
- Around line 334-343: The current else-if chain in the tag parsing logic
(checking t == b"RG", cell_tag.is_some_and(|ct| t == *ct), and t == b"MC") makes
these matches mutually exclusive so a single aux entry that should set multiple
fields (e.g., when cell_tag equals "RG" or "MC") only sets the first branch;
change the chain in the function that populates result (the blocks that assign
result.rg, result.cell, result.mc and manipulate found) so each condition is
evaluated independently (use separate if statements rather than else if),
ensuring each matching condition sets its corresponding field and updates the
found bitmask appropriately.
In `@src/commands/sort.rs`:
- Around line 175-180: The doc comment for the queryname sub-sort should list
the accepted alias "queryname::lex" and pin it as equivalent to lexicographic;
update the triple-slash comment above the --order arg (the block that currently
lists `queryname::lexicographic` and `queryname::natural`) to include
`queryname::lex` as an explicit alias, and add a regression test that passes
`--order queryname::lex` exercising SortOrderArg::parse so the parser behavior
is covered; ensure the other identical queryname doc block is updated too (the
duplicate block handled by the same SortOrderArg parsing logic).
---
Nitpick comments:
In `@src/lib/progress.rs`:
- Around line 383-391: Replace the repeated-assertion unit test
test_fmt_duration with an rstest parameterized case table: add use
rstest::rstest to the test imports, annotate the test with #[rstest], and
declare parameters (input, expected) with a #[case(...)] for each of the six
examples, then call fmt_duration(input) and assert_eq! against expected inside
the test body; keep the test function name test_fmt_duration and reference the
fmt_duration function so CI/style matches repo guidelines.
🪄 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: c21ae97a-7d39-42bf-91b6-ff879577ab54
📒 Files selected for processing (15)
CLAUDE.mdbenches/core_functions.rscrates/fgumi-raw-bam/src/cigar.rscrates/fgumi-raw-bam/src/tags.rssrc/commands/merge.rssrc/commands/sort.rssrc/lib/bam_io.rssrc/lib/progress.rssrc/lib/sort/external.rssrc/lib/sort/inline_buffer.rssrc/lib/sort/keys.rssrc/lib/sort/loser_tree.rssrc/lib/sort/mod.rssrc/lib/sort/radix.rssrc/lib/sort/raw.rs
✅ Files skipped from review due to trivial changes (7)
- src/lib/sort/loser_tree.rs
- CLAUDE.md
- src/commands/merge.rs
- src/lib/bam_io.rs
- benches/core_functions.rs
- src/lib/sort/keys.rs
- src/lib/sort/raw.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/fgumi-raw-bam/src/cigar.rs
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/lib/progress.rs (1)
242-245:⚠️ Potential issue | 🟡 MinorZero-count runs still suppress the completion log.
Line 243 early-returns, so empty/fully-filtered runs emit no final completion line.
Suggested fix
pub fn log_final(&self) { let count = self.count.load(Ordering::Relaxed); - if count == 0 { - return; - } - if self.total.is_some() { let elapsed = self.start_time.elapsed().as_secs_f64(); info!("{} {} (complete, {})", self.message, count, fmt_duration(elapsed)); } else if !self.log_if_needed(0) { info!("{} {} (complete)", self.message, count); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/progress.rs` around lines 242 - 245, The early return after reading count (let count = self.count.load(Ordering::Relaxed); if count == 0 { return; }) suppresses the final completion log for empty or fully-filtered runs; change the logic in the function that uses self.count so that when count == 0 it still emits the final completion line (e.g., call the same completion/logging path used for non-zero counts) and only skip other per-item work, or move the return to after the completion/log call; ensure references to self.count, count, and the completion/logging routine are updated so empty runs produce the final log.
🧹 Nitpick comments (1)
src/lib/sort/keys.rs (1)
1909-2002: Useproptestfor the comparator invariants.These symmetry/transitivity/reflexivity checks are property tests in disguise, and most of the heavy coverage in this file still targets
natural_comparerather than the productionnatural_compare_nulpath. A small generator over NUL-terminated ASCII names would cover a lot more surface.As per coding guidelines, "
**/*.rs: Use proptest for property-based testing`."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/sort/keys.rs` around lines 1909 - 2002, Replace the handwritten example-based tests (test_natural_compare_symmetry, test_natural_compare_transitivity, test_natural_compare_reflexive) with proptest-based property tests that generate NUL-terminated ASCII byte sequences and assert the comparator invariants for both natural_compare and the production natural_compare_nul: write a generator using proptest::collection::vec(proptest::char::range('\x01','\x7F'), 0..N) mapped to Vec<u8> then push 0u8 to ensure NUL-termination, and use proptest! blocks to assert symmetry (cmp(a,b) == cmp(b,a).reverse()), transitivity (for generated triples a<b and b<c assert a<c), and reflexivity (cmp(s,s) == Equal); ensure you import proptest macros/traits and run the same properties against natural_compare_nul in addition to natural_compare to increase coverage.
🤖 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/lib/progress.rs`:
- Around line 383-391: Replace the monolithic test_fmt_duration with a
rstest-parameterized test over inputs and expected strings: add rstest as a
dev-dependency if not present, annotate a new test (e.g.,
test_fmt_duration_param) with #[rstest] and provide cases like (input, expected)
= [(0.0, "0s"), (59.0, "59s"), (59.5, "1m"), (90.0, "1m 30s"), (3600.0, "1h"),
(5400.0, "1h 30m")], then assert_eq!(fmt_duration(input), expected) inside the
test; keep the original fmt_duration function name to locate the logic and
remove or replace the old test_fmt_duration to avoid duplication.
In `@src/lib/sort/keys.rs`:
- Around line 594-600: The derived PartialEq/Eq on QuerynameKey is inconsistent
with its Ord implementation that uses natural_compare_nul (which treats b"1" and
b"01" as equal); replace the derived equality by implementing PartialEq for
QuerynameKey to return self.cmp(other) == std::cmp::Ordering::Equal and impl Eq
for it (or alternatively add the same deterministic tiebreaker used in Ord to
equality), ensuring equality semantics match the Ord::cmp; locate the Ord impl
that calls natural_compare_nul and make the equality change there, and apply the
same fix to the other key structs mentioned that use natural_compare_nul so
their PartialEq/Eq match their ordering.
- Around line 610-615: The call site in QuerynameKey::cmp uses the unsafe helper
natural_compare_nul (and there are other unsafe helpers natural_compare) which
rely on raw-pointer get_unchecked and are currently allowed in src via
#[allow(unsafe_code)]; to fix, remove unsafe usage from src by either (A)
rewriting natural_compare_nul/natural_compare to a safe implementation (e.g.,
use CStr, bytes().iter(), or safe indexing/iterators instead of raw-pointer
walking and get_unchecked) and update QuerynameKey::cmp to call the safe
versions, or (B) move the existing unsafe implementations into a separate
module/crate that is explicitly allowed to contain unsafe code (an
ffi/unsafe_impl boundary), expose a safe API (e.g., natural_compare_nul_safe)
and then call that from QuerynameKey::cmp; reference functions
QuerynameKey::cmp, natural_compare_nul, natural_compare and constructor
from_record when making the change.
---
Duplicate comments:
In `@src/lib/progress.rs`:
- Around line 242-245: The early return after reading count (let count =
self.count.load(Ordering::Relaxed); if count == 0 { return; }) suppresses the
final completion log for empty or fully-filtered runs; change the logic in the
function that uses self.count so that when count == 0 it still emits the final
completion line (e.g., call the same completion/logging path used for non-zero
counts) and only skip other per-item work, or move the return to after the
completion/log call; ensure references to self.count, count, and the
completion/logging routine are updated so empty runs produce the final log.
---
Nitpick comments:
In `@src/lib/sort/keys.rs`:
- Around line 1909-2002: Replace the handwritten example-based tests
(test_natural_compare_symmetry, test_natural_compare_transitivity,
test_natural_compare_reflexive) with proptest-based property tests that generate
NUL-terminated ASCII byte sequences and assert the comparator invariants for
both natural_compare and the production natural_compare_nul: write a generator
using proptest::collection::vec(proptest::char::range('\x01','\x7F'), 0..N)
mapped to Vec<u8> then push 0u8 to ensure NUL-termination, and use proptest!
blocks to assert symmetry (cmp(a,b) == cmp(b,a).reverse()), transitivity (for
generated triples a<b and b<c assert a<c), and reflexivity (cmp(s,s) == Equal);
ensure you import proptest macros/traits and run the same properties against
natural_compare_nul in addition to natural_compare to increase coverage.
🪄 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: d74eea51-a7b7-48fc-9139-1580e9c22353
📒 Files selected for processing (15)
CLAUDE.mdbenches/core_functions.rscrates/fgumi-raw-bam/src/cigar.rscrates/fgumi-raw-bam/src/tags.rssrc/commands/merge.rssrc/commands/sort.rssrc/lib/bam_io.rssrc/lib/progress.rssrc/lib/sort/external.rssrc/lib/sort/inline_buffer.rssrc/lib/sort/keys.rssrc/lib/sort/loser_tree.rssrc/lib/sort/mod.rssrc/lib/sort/radix.rssrc/lib/sort/raw.rs
✅ Files skipped from review due to trivial changes (5)
- src/lib/sort/loser_tree.rs
- src/commands/merge.rs
- CLAUDE.md
- src/lib/bam_io.rs
- crates/fgumi-raw-bam/src/tags.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/fgumi-raw-bam/src/cigar.rs
- src/lib/sort/mod.rs
- src/lib/sort/raw.rs
nh13
force-pushed
the
nh/sort-optimizations-v3
branch
from
April 3, 2026 01:21
987b225 to
14a18ea
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (5)
src/lib/sort/mod.rs (1)
64-115:⚠️ Potential issue | 🟠 MajorPreserve existing
@HDmetadata.This rebuilds the header map from scratch, so
VNand any other non-sort@HDfields disappear on output. Start from the incomingheader.header()map and overwrite onlySO/GO/SS; add a regression test with a non-default@HD.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/sort/mod.rs` around lines 64 - 115, create_output_header currently constructs a new `@HD` map from scratch and drops existing non-sort fields (e.g. VN); instead start from the incoming header.header() map, clone or build from that existing Map and then overwrite only the SORT_ORDER / GROUP_ORDER / SUBSORT_ORDER entries (use header.header(), Map::builder() or a builder-from-existing-map pattern) before calling builder.set_header(...); update the function identifiers mentioned (create_output_header, header.header(), builder.set_header, header_tag::SORT_ORDER/header_tag::GROUP_ORDER/header_tag::SUBSORT_ORDER) and add a regression test that supplies a header with a non-default VN to assert VN is preserved after create_output_header.src/lib/progress.rs (1)
241-250:⚠️ Potential issue | 🟡 MinorDon't suppress the only completion log for zero-record runs.
This guard still drops the final line for empty or fully filtered inputs, even when
totalis set. It also contradicts the doc comment abovelog_final.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/progress.rs` around lines 241 - 250, The early return in log_final when count == 0 suppresses the completion log for zero-record runs; change the guard so it only skips logging when count == 0 AND self.total.is_none(). In other words, in the log_final method, replace the unconditional `if count == 0 { return; }` with logic that returns only when there is no total (e.g., `if count == 0 && self.total.is_none() { return; }`) so the total-aware completion branch (`if self.total.is_some() { ... }`) still emits the final completion line using self.message, count, start_time, and fmt_duration.crates/fgumi-raw-bam/src/tags.rs (1)
334-342:⚠️ Potential issue | 🟠 MajorKeep overlapping tag aliases independent.
RG,cell_tag, andMCare still mutually exclusive here. Ifcell_tag == b"RG"orb"MC", one aux entry only populates the first field and template-coordinate extraction changes.Possible fix
- if t == *b"RG" { - result.rg = Some(value); - found |= 2; - } else if cell_tag.is_some_and(|ct| t == *ct) { - result.cell = Some(value); - found |= 4; - } else if t == *b"MC" { - result.mc = std::str::from_utf8(value).ok(); - found |= 8; - } + if t == *b"RG" { + result.rg = Some(value); + found |= 2; + } + if cell_tag.is_some_and(|ct| t == *ct) { + result.cell = Some(value); + found |= 4; + } + if t == *b"MC" { + result.mc = std::str::from_utf8(value).ok(); + found |= 8; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/fgumi-raw-bam/src/tags.rs` around lines 334 - 342, The current else-if chain makes RG, cell_tag, and MC mutually exclusive; change the branches in the block that inspects t (and cell_tag) so each tag check is independent (i.e., use separate if statements rather than else if) to ensure result.rg, result.cell, and result.mc can all be populated from the same aux entry; keep the same parsing for MC (std::str::from_utf8(value).ok()) and still set the corresponding found bits (use the same bit flags 2 for RG, 4 for cell, 8 for MC) when each condition matches.src/lib/sort/external.rs (1)
167-174: 🛠️ Refactor suggestion | 🟠 MajorMake unsupported queryname orders unrepresentable.
ExternalSorterstill acceptsSortOrder::Queryname(QuerynameComparator::Lexicographic), but this branch can only fail at runtime. Either add theRecordBuflexicographic key path here or narrow the API so unsupported orders can't reachsort().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/sort/external.rs` around lines 167 - 174, The code allows SortOrder::Queryname(QuerynameComparator::Lexicographic) to reach ExternalSorter::sort and only fails at runtime; fix by making unsupported queryname orders unrepresentable: either implement the lexicographic path by calling the RecordBuf-based key variant instead of bailing (i.e., replace the anyhow::bail! branch with a call to the RecordBuf lexicographic key via sort_with_key::<RecordBufQuerynameLexicographicKey> or the equivalent RecordBuf key type), or change the API so ExternalSorter::sort (or the type constructing SortOrder) cannot accept Queryname(QuerynameComparator::Lexicographic) (e.g., narrow the SortOrder/QuerynameComparator enum or validate earlier), ensuring you update the branch that now matches SortOrder::Queryname and the call site of sort_with_key::<QuerynameKey> accordingly.src/lib/sort/keys.rs (1)
618-623:⚠️ Potential issue | 🟠 MajorMove the unsafe boundary out of
src/.These
#[allow(unsafe_code)]call sites still bypass the repo rule forsrc/**/*.rs. Please expose a safe NUL-terminated-name wrapper fromfgumi_raw_bamand call that here instead.#!/bin/bash rg -n '#!\[deny\(unsafe_code\)\]' src rg -n 'allow\(unsafe_code\)|natural_compare_nul\(' src/lib/sort/keys.rsAs per coding guidelines "
src/**/*.rs: Deny unsafe code via #![deny(unsafe_code)] - only standard library unsafe is permitted".Also applies to: 818-822
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/sort/keys.rs` around lines 618 - 623, The unsafe block in QuerynameKey::cmp (and the similar site at the other noted location) is bypassing the repo-wide deny of unsafe in src; instead, add a safe NUL-terminated-name wrapper type in fgumi_raw_bam (constructed in from_record) that guarantees a NUL-terminated pointer and exposes a safe method or trait to obtain the C pointer, then replace the unsafe call here to natural_compare_nul(self.name.as_ptr(), ...) with a call that uses that safe wrapper (e.g., natural_compare_nul_safe(&self.name_wrapper, &other.name_wrapper) or a method that returns a safe FFI compare), remove #[allow(unsafe_code)] and the unsafe block from QuerynameKey::cmp, and apply the same refactor to the other cmp site (lines 818–822) so no unsafe remains in src/.
🧹 Nitpick comments (2)
src/lib/sort/keys.rs (1)
1751-1844: Refactor these tests to useproptestfor property-based testing.These tests verify universal invariants (symmetry, transitivity, reflexivity) that should hold for all inputs. Property-based testing will exercise the digit-run and leading-zero space comprehensively rather than relying on hardcoded examples. This aligns with the project's coding guidelines for Rust files.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/sort/keys.rs` around lines 1751 - 1844, Replace the hardcoded example tests with property-based tests using proptest: add proptest as a dev-dependency and import proptest::prelude::*; rewrite test_natural_compare_symmetry to a proptest that generates arbitrary byte slices (e.g., Vec<u8> or proptest::collection strategies) and asserts natural_compare(a,b) == natural_compare(b,a).reverse(); rewrite test_natural_compare_reflexive as a single-argument proptest that asserts natural_compare(s,s) == Ordering::Equal for all generated s; rewrite test_natural_compare_transitivity as a three-argument proptest that generates (a,b,c) and uses prop_assume!(natural_compare(a,b) == Ordering::Less && natural_compare(b,c) == Ordering::Less) then asserts natural_compare(a,c) == Ordering::Less; keep referencing the natural_compare function in the assertions and keep test names (test_natural_compare_symmetry, test_natural_compare_transitivity, test_natural_compare_reflexive) or convert to proptest! blocks as appropriate.src/lib/sort/raw.rs (1)
2522-2533: Stale comment?Line 2525 comment says "With threads > 1, the sort pipeline always writes at least one chunk" but the code paths (lines 1086-1107, 1396-1418) show in-memory-only paths that don't write chunks when everything fits in memory. The assertion at line 2532 only checks record count, not chunk behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/sort/raw.rs` around lines 2522 - 2533, The comment "With threads > 1, the sort pipeline always writes at least one chunk" is stale because RawExternalSorter::new(...).memory_limit(...).threads(...).output_compression(...).sort(...) can take in-memory-only paths (see in-memory paths at the earlier code in sort implementation); update the test/comment in this block to reflect actual behavior: either remove or reword the comment to state that threads>1 does not guarantee chunk writes when data fits in memory, and keep the assertion using count_bam_records(&output) to verify no data loss; reference RawExternalSorter::new, memory_limit, threads, output_compression, sort, and count_bam_records when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@crates/fgumi-raw-bam/src/tags.rs`:
- Around line 334-342: The current else-if chain makes RG, cell_tag, and MC
mutually exclusive; change the branches in the block that inspects t (and
cell_tag) so each tag check is independent (i.e., use separate if statements
rather than else if) to ensure result.rg, result.cell, and result.mc can all be
populated from the same aux entry; keep the same parsing for MC
(std::str::from_utf8(value).ok()) and still set the corresponding found bits
(use the same bit flags 2 for RG, 4 for cell, 8 for MC) when each condition
matches.
In `@src/lib/progress.rs`:
- Around line 241-250: The early return in log_final when count == 0 suppresses
the completion log for zero-record runs; change the guard so it only skips
logging when count == 0 AND self.total.is_none(). In other words, in the
log_final method, replace the unconditional `if count == 0 { return; }` with
logic that returns only when there is no total (e.g., `if count == 0 &&
self.total.is_none() { return; }`) so the total-aware completion branch (`if
self.total.is_some() { ... }`) still emits the final completion line using
self.message, count, start_time, and fmt_duration.
In `@src/lib/sort/external.rs`:
- Around line 167-174: The code allows
SortOrder::Queryname(QuerynameComparator::Lexicographic) to reach
ExternalSorter::sort and only fails at runtime; fix by making unsupported
queryname orders unrepresentable: either implement the lexicographic path by
calling the RecordBuf-based key variant instead of bailing (i.e., replace the
anyhow::bail! branch with a call to the RecordBuf lexicographic key via
sort_with_key::<RecordBufQuerynameLexicographicKey> or the equivalent RecordBuf
key type), or change the API so ExternalSorter::sort (or the type constructing
SortOrder) cannot accept Queryname(QuerynameComparator::Lexicographic) (e.g.,
narrow the SortOrder/QuerynameComparator enum or validate earlier), ensuring you
update the branch that now matches SortOrder::Queryname and the call site of
sort_with_key::<QuerynameKey> accordingly.
In `@src/lib/sort/keys.rs`:
- Around line 618-623: The unsafe block in QuerynameKey::cmp (and the similar
site at the other noted location) is bypassing the repo-wide deny of unsafe in
src; instead, add a safe NUL-terminated-name wrapper type in fgumi_raw_bam
(constructed in from_record) that guarantees a NUL-terminated pointer and
exposes a safe method or trait to obtain the C pointer, then replace the unsafe
call here to natural_compare_nul(self.name.as_ptr(), ...) with a call that uses
that safe wrapper (e.g., natural_compare_nul_safe(&self.name_wrapper,
&other.name_wrapper) or a method that returns a safe FFI compare), remove
#[allow(unsafe_code)] and the unsafe block from QuerynameKey::cmp, and apply the
same refactor to the other cmp site (lines 818–822) so no unsafe remains in
src/.
In `@src/lib/sort/mod.rs`:
- Around line 64-115: create_output_header currently constructs a new `@HD` map
from scratch and drops existing non-sort fields (e.g. VN); instead start from
the incoming header.header() map, clone or build from that existing Map and then
overwrite only the SORT_ORDER / GROUP_ORDER / SUBSORT_ORDER entries (use
header.header(), Map::builder() or a builder-from-existing-map pattern) before
calling builder.set_header(...); update the function identifiers mentioned
(create_output_header, header.header(), builder.set_header,
header_tag::SORT_ORDER/header_tag::GROUP_ORDER/header_tag::SUBSORT_ORDER) and
add a regression test that supplies a header with a non-default VN to assert VN
is preserved after create_output_header.
---
Nitpick comments:
In `@src/lib/sort/keys.rs`:
- Around line 1751-1844: Replace the hardcoded example tests with property-based
tests using proptest: add proptest as a dev-dependency and import
proptest::prelude::*; rewrite test_natural_compare_symmetry to a proptest that
generates arbitrary byte slices (e.g., Vec<u8> or proptest::collection
strategies) and asserts natural_compare(a,b) == natural_compare(b,a).reverse();
rewrite test_natural_compare_reflexive as a single-argument proptest that
asserts natural_compare(s,s) == Ordering::Equal for all generated s; rewrite
test_natural_compare_transitivity as a three-argument proptest that generates
(a,b,c) and uses prop_assume!(natural_compare(a,b) == Ordering::Less &&
natural_compare(b,c) == Ordering::Less) then asserts natural_compare(a,c) ==
Ordering::Less; keep referencing the natural_compare function in the assertions
and keep test names (test_natural_compare_symmetry,
test_natural_compare_transitivity, test_natural_compare_reflexive) or convert to
proptest! blocks as appropriate.
In `@src/lib/sort/raw.rs`:
- Around line 2522-2533: The comment "With threads > 1, the sort pipeline always
writes at least one chunk" is stale because
RawExternalSorter::new(...).memory_limit(...).threads(...).output_compression(...).sort(...)
can take in-memory-only paths (see in-memory paths at the earlier code in sort
implementation); update the test/comment in this block to reflect actual
behavior: either remove or reword the comment to state that threads>1 does not
guarantee chunk writes when data fits in memory, and keep the assertion using
count_bam_records(&output) to verify no data loss; reference
RawExternalSorter::new, memory_limit, threads, output_compression, sort, and
count_bam_records when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a249e58d-7c39-4737-855b-7825344d95c5
📒 Files selected for processing (16)
CLAUDE.mdbenches/core_functions.rscrates/fgumi-raw-bam/src/cigar.rscrates/fgumi-raw-bam/src/sort.rscrates/fgumi-raw-bam/src/tags.rssrc/commands/merge.rssrc/commands/sort.rssrc/lib/bam_io.rssrc/lib/progress.rssrc/lib/sort/external.rssrc/lib/sort/inline_buffer.rssrc/lib/sort/keys.rssrc/lib/sort/loser_tree.rssrc/lib/sort/mod.rssrc/lib/sort/radix.rssrc/lib/sort/raw.rs
✅ Files skipped from review due to trivial changes (4)
- src/lib/sort/loser_tree.rs
- CLAUDE.md
- src/lib/bam_io.rs
- src/lib/sort/inline_buffer.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- crates/fgumi-raw-bam/src/cigar.rs
- src/lib/sort/radix.rs
- benches/core_functions.rs
- src/commands/sort.rs
nh13
force-pushed
the
nh/sort-optimizations-v3
branch
from
April 3, 2026 01:40
14a18ea to
c56b31d
Compare
nh13
force-pushed
the
nh/sort-optimizations-v3
branch
from
April 3, 2026 01:46
c56b31d to
fd77bd8
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/lib/sort/inline_buffer.rs (1)
32-53:⚠️ Potential issue | 🟡 Minor
PackedCoordinateKey::newdoesn't honor the documentedtid = -1case.Line 36 says callers can pass
-1for unmapped, but Lines 45-51 pack(nref, pos, reverse)instead of returning theu64::MAXsentinel thatextract_coordinate_key_inline()uses on Lines 340-344. That gives callers a different ordering for no-reference reads than the extractor.Suggested fix
pub fn new(tid: i32, pos: i32, reverse: bool, nref: u32) -> Self { - // Map unmapped (tid=-1) to nref for proper sorting (after all mapped) - let tid = if tid < 0 { nref } else { tid as u32 }; + if tid < 0 { + return Self::unmapped(); + } + let tid = tid as u32; // Pack: tid in high bits, (pos+1) in middle, reverse in LSB // Using pos+1 so that pos=0 doesn't become 0 in the key #[allow(clippy::cast_lossless)] // Explicit bit packing requires precise control🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/sort/inline_buffer.rs` around lines 32 - 53, PackedCoordinateKey::new currently maps tid=-1 to nref and packs a key, but extract_coordinate_key_inline() expects the unmapped sentinel u64::MAX; update PackedCoordinateKey::new so when tid < 0 it returns Self(u64::MAX) immediately, otherwise perform the existing packing of tid,pos,reverse (use the same packing logic and keep clippy allows); reference PackedCoordinateKey::new and extract_coordinate_key_inline() to ensure both sides use the same sentinel for unmapped reads.src/lib/sort/raw.rs (1)
1366-1370:⚠️ Potential issue | 🟠 Major
sort_unstable_bymakes queryname output depend on chunking.These paths never add a deterministic tiebreaker after
K::cmp, so equal queryname keys can permute between the in-memory, spill, and parallel runs. If reproducible output matters here, switch these sites to stable sorts or add a fallback compare on raw bytes.Also applies to: 1400-1404, 1427-1428, 1437-1438
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/sort/raw.rs` around lines 1366 - 1370, The current use of sort_unstable_by in entries.par_sort_unstable_by and entries.sort_unstable_by (and the similar calls around the other sites) only compares keys via a.0 (K::cmp) and lacks a deterministic tiebreaker, allowing equal queryname keys to permute between in-memory, spill, and parallel runs; change these to a stable sort (par_sort_by / sort_by) or append a secondary compare that deterministically breaks ties (e.g., compare the raw bytes or entire record after a.0) so equal K values are ordered reproducibly—update the calls at entries.par_sort_unstable_by/entries.sort_unstable_by and the analogous calls at the other noted sites to use either stable sorting methods or an explicit fallback comparator.
♻️ Duplicate comments (5)
src/lib/progress.rs (1)
241-250:⚠️ Potential issue | 🟡 MinorDon't suppress zero-count completion logs.
Line 243 reintroduces the early return that drops the only completion line for empty or fully filtered runs.
log_final()should still emit the final0record summary, and this path wants a regression test.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/progress.rs` around lines 241 - 250, log_final currently returns early when count == 0 which suppresses the final completion log for empty or fully filtered runs; modify the log_final method so it does not early-return on a zero count (remove or change the `if count == 0 { return; }` behavior) and allow the function to emit the final summary using the existing branches that reference self.total, self.start_time, and log_if_needed; also add a regression test that calls log_final with a zero count to assert the final "0" summary is logged.src/commands/sort.rs (1)
175-180:⚠️ Potential issue | 🟡 MinorKeep
--orderhelp in sync with the parser.
SortOrderArg::parseacceptsqueryname::lex, but the option help here still omits it. Please list the alias so--helpmatches accepted input.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/sort.rs` around lines 175 - 180, The --order option help string is out of sync with the parser: SortOrderArg::parse accepts the alias "queryname::lex" but the help text only lists "queryname::lexicographic"; update the doc comment above the #[arg(...)] for the --order option in src/commands/sort.rs to include "queryname::lex" as an accepted sub-sort specifier (e.g. list both `queryname::lexicographic` and the alias `queryname::lex`) so that the --help output matches SortOrderArg::parse.src/lib/sort/external.rs (1)
167-174:⚠️ Potential issue | 🟠 Major
ExternalSorterstill accepts a mode that can only fail.Line 168 allows
SortOrder::Queryname(QuerynameComparator::Lexicographic)to be constructed and then rejected only at runtime. Either add the lexicographic key path here or make that variant impossible to pass toExternalSorter.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/sort/external.rs` around lines 167 - 174, The ExternalSorter currently accepts SortOrder::Queryname(QuerynameComparator::Lexicographic) only to bail at runtime; update the code so this impossible-to-handle mode is rejected earlier or handled: either (A) extend ExternalSorter to support the lexicographic key path (implement and call the appropriate lexicographic key routine instead of bailing) or (B) prevent the variant from being passed to ExternalSorter by validating/transforming SortOrder before calling ExternalSorter (e.g., route lexicographic cases to RawExternalSorter at call-site). Locate the match arm handling SortOrder::Queryname in ExternalSorter and apply one of these fixes, referencing SortOrder::Queryname(QuerynameComparator::Lexicographic), ExternalSorter, RawExternalSorter, and QuerynameKey.src/lib/sort/keys.rs (1)
618-623:⚠️ Potential issue | 🟠 MajorThe queryname comparator still crosses the
unsafeboundary insidesrc/.Moving
natural_compare_nulintofgumi_raw_bamhelps, but Lines 619-622 and 818-821 still need#[allow(unsafe_code)]plusunsafe { ... }insrc/lib/sort/keys.rs. Please expose a safe wrapper fromfgumi_raw_bamand call that here instead.As per coding guidelines:
src/**/*.rs: Deny unsafe code via#![deny(unsafe_code)]- only standard library unsafe is permitted.Also applies to: 818-822
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/sort/keys.rs` around lines 618 - 623, The comparator in impl Ord for QuerynameKey uses unsafe { natural_compare_nul(...) } and #[allow(unsafe_code)]—replace that unsafe call by adding and calling a safe wrapper exported from the fgumi_raw_bam crate (e.g., fgumi_raw_bam::natural_compare_nul_safe or fgumi_raw_bam::compare_nul_names) that internally performs the unsafe pointer work; update the QuerynameKey::cmp implementation to call that safe wrapper and remove the #[allow(unsafe_code)] annotation; apply the same change to the other comparator implementation around lines 818-822 so both use the safe wrapper instead of calling natural_compare_nul directly.src/lib/sort/raw.rs (1)
414-419:⚠️ Potential issue | 🔴 CriticalDon't collapse truncation or reader-thread death into EOF.
read_exacton the 4-byte headers cannot distinguish clean EOF from a 1-3 byte partial header, and Lines 512-526 still turn channel disconnects intoOk(None). A truncated spill file—or a panic inextract_from_record—will silently drop the tail of the merge instead of failing.Also applies to: 438-442, 504-513, 522-526
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/sort/raw.rs` around lines 414 - 419, The read_exact call on len_buf currently treats any UnexpectedEof as clean EOF; instead, read into len_buf with reader.read(&mut len_buf) (or call read_exact but on Err inspect how many bytes were read) and only treat it as EOF if 0 bytes were read; if 1..3 bytes were read then return an error indicating a truncated header and propagate it (do not convert to Ok(None)). Likewise, stop converting channel disconnects or panics from extract_from_record into Ok(None): ensure functions that previously returned Ok(None) on these errors propagate Err (or panic) so truncated spill files or thread failure cause a real failure rather than silently dropping the tail; update the logic around reader.read_exact, len_buf handling, and the extract_from_record return mapping accordingly.
🧹 Nitpick comments (2)
crates/fgumi-raw-bam/src/sort.rs (1)
58-234: Please pin the new natural comparators down with direct tests.This is the new queryname ordering core, with leading-zero edge cases and an unsafe NUL walker, but there are no direct regression tests for it in this module. Add
rstestcases for tricky pairs and aproptestcheck thatnatural_compareandnatural_compare_nulagree on NUL-free inputs.As per coding guidelines "
**/*.rs: Use rstest for parameterized tests" and "**/*.rs: Use proptest for property-based testing".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/fgumi-raw-bam/src/sort.rs` around lines 58 - 234, Add direct tests for the new comparators: write rstest parameterized unit tests in this module that call natural_compare(a,b) and unsafe { natural_compare_nul(a.as_ptr(), b.as_ptr()) } for tricky hard-coded pairs (e.g., differing leading zeros like "01" vs "1", numeric run ties like "read2" vs "read10", mixed non-digit boundaries) to pin down behavior; and add a proptest property test that generates arbitrary non-NUL byte strings and asserts natural_compare(slice_a, slice_b) == unsafe { natural_compare_nul(slice_a.as_ptr(), slice_b.as_ptr()) } for all NUL-free inputs to catch regressions. Ensure tests import rstest and proptest macros and mark the NUL-walking call unsafe inside the test.src/commands/sort.rs (1)
630-679: Collapse this parser matrix into onerstest.These cases are one parameter table. Using
#[rstest]keeps the matrix smaller and failures clearer per input.As per coding guidelines "
**/*.rs: Use rstest for parameterized tests".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/sort.rs` around lines 630 - 679, Collapse the multiple unit tests into a single parameterized rstest that iterates over input cases and expected outcomes for SortOrderArg::parse; replace separate functions like test_parse_sort_order_coordinate, test_parse_sort_order_queryname_default, test_parse_sort_order_queryname_lexicographic, test_parse_sort_order_queryname_natural, test_parse_sort_order_template_coordinate, test_parse_sort_order_unknown_subsort, test_parse_sort_order_unknown_order, and test_parse_sort_order_empty_subsort with one #[rstest] that supplies tuples of (input_str, expected_result_or_error), calling SortOrderArg::parse for each case and asserting either equality for Ok variants (e.g., SortOrderArg::Coordinate, Queryname, QuerynameNatural, TemplateCoordinate) or that the error string contains the expected message (e.g., "unknown queryname sub-sort 'fast'", "unknown sort order 'random'", "unknown queryname sub-sort ''"); ensure test names/reference to SortOrderArg::parse remain in assertions so failures are clear.
🤖 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/lib/sort/radix.rs`:
- Around line 355-370: The tests currently use sort_by_key on both sides (or
only use it), so they don't exercise our radix code; change
test_packed_key_sort_small and test_packed_key_sort_large to compute expected
with the standard library (expected.sort_by_key(|(k,_)| *k)) but run the shipped
sorter under test on entries (call insertion_sort_by_key or
radix_sort_coordinate_adaptive as appropriate for the test) instead of
entries.sort_by_key(...), so assertions compare the library sorter output to the
oracle; ensure you call the exact exported functions insertion_sort_by_key and
radix_sort_coordinate_adaptive on the entries used by each test.
---
Outside diff comments:
In `@src/lib/sort/inline_buffer.rs`:
- Around line 32-53: PackedCoordinateKey::new currently maps tid=-1 to nref and
packs a key, but extract_coordinate_key_inline() expects the unmapped sentinel
u64::MAX; update PackedCoordinateKey::new so when tid < 0 it returns
Self(u64::MAX) immediately, otherwise perform the existing packing of
tid,pos,reverse (use the same packing logic and keep clippy allows); reference
PackedCoordinateKey::new and extract_coordinate_key_inline() to ensure both
sides use the same sentinel for unmapped reads.
In `@src/lib/sort/raw.rs`:
- Around line 1366-1370: The current use of sort_unstable_by in
entries.par_sort_unstable_by and entries.sort_unstable_by (and the similar calls
around the other sites) only compares keys via a.0 (K::cmp) and lacks a
deterministic tiebreaker, allowing equal queryname keys to permute between
in-memory, spill, and parallel runs; change these to a stable sort (par_sort_by
/ sort_by) or append a secondary compare that deterministically breaks ties
(e.g., compare the raw bytes or entire record after a.0) so equal K values are
ordered reproducibly—update the calls at
entries.par_sort_unstable_by/entries.sort_unstable_by and the analogous calls at
the other noted sites to use either stable sorting methods or an explicit
fallback comparator.
---
Duplicate comments:
In `@src/commands/sort.rs`:
- Around line 175-180: The --order option help string is out of sync with the
parser: SortOrderArg::parse accepts the alias "queryname::lex" but the help text
only lists "queryname::lexicographic"; update the doc comment above the
#[arg(...)] for the --order option in src/commands/sort.rs to include
"queryname::lex" as an accepted sub-sort specifier (e.g. list both
`queryname::lexicographic` and the alias `queryname::lex`) so that the --help
output matches SortOrderArg::parse.
In `@src/lib/progress.rs`:
- Around line 241-250: log_final currently returns early when count == 0 which
suppresses the final completion log for empty or fully filtered runs; modify the
log_final method so it does not early-return on a zero count (remove or change
the `if count == 0 { return; }` behavior) and allow the function to emit the
final summary using the existing branches that reference self.total,
self.start_time, and log_if_needed; also add a regression test that calls
log_final with a zero count to assert the final "0" summary is logged.
In `@src/lib/sort/external.rs`:
- Around line 167-174: The ExternalSorter currently accepts
SortOrder::Queryname(QuerynameComparator::Lexicographic) only to bail at
runtime; update the code so this impossible-to-handle mode is rejected earlier
or handled: either (A) extend ExternalSorter to support the lexicographic key
path (implement and call the appropriate lexicographic key routine instead of
bailing) or (B) prevent the variant from being passed to ExternalSorter by
validating/transforming SortOrder before calling ExternalSorter (e.g., route
lexicographic cases to RawExternalSorter at call-site). Locate the match arm
handling SortOrder::Queryname in ExternalSorter and apply one of these fixes,
referencing SortOrder::Queryname(QuerynameComparator::Lexicographic),
ExternalSorter, RawExternalSorter, and QuerynameKey.
In `@src/lib/sort/keys.rs`:
- Around line 618-623: The comparator in impl Ord for QuerynameKey uses unsafe {
natural_compare_nul(...) } and #[allow(unsafe_code)]—replace that unsafe call by
adding and calling a safe wrapper exported from the fgumi_raw_bam crate (e.g.,
fgumi_raw_bam::natural_compare_nul_safe or fgumi_raw_bam::compare_nul_names)
that internally performs the unsafe pointer work; update the QuerynameKey::cmp
implementation to call that safe wrapper and remove the #[allow(unsafe_code)]
annotation; apply the same change to the other comparator implementation around
lines 818-822 so both use the safe wrapper instead of calling
natural_compare_nul directly.
In `@src/lib/sort/raw.rs`:
- Around line 414-419: The read_exact call on len_buf currently treats any
UnexpectedEof as clean EOF; instead, read into len_buf with reader.read(&mut
len_buf) (or call read_exact but on Err inspect how many bytes were read) and
only treat it as EOF if 0 bytes were read; if 1..3 bytes were read then return
an error indicating a truncated header and propagate it (do not convert to
Ok(None)). Likewise, stop converting channel disconnects or panics from
extract_from_record into Ok(None): ensure functions that previously returned
Ok(None) on these errors propagate Err (or panic) so truncated spill files or
thread failure cause a real failure rather than silently dropping the tail;
update the logic around reader.read_exact, len_buf handling, and the
extract_from_record return mapping accordingly.
---
Nitpick comments:
In `@crates/fgumi-raw-bam/src/sort.rs`:
- Around line 58-234: Add direct tests for the new comparators: write rstest
parameterized unit tests in this module that call natural_compare(a,b) and
unsafe { natural_compare_nul(a.as_ptr(), b.as_ptr()) } for tricky hard-coded
pairs (e.g., differing leading zeros like "01" vs "1", numeric run ties like
"read2" vs "read10", mixed non-digit boundaries) to pin down behavior; and add a
proptest property test that generates arbitrary non-NUL byte strings and asserts
natural_compare(slice_a, slice_b) == unsafe {
natural_compare_nul(slice_a.as_ptr(), slice_b.as_ptr()) } for all NUL-free
inputs to catch regressions. Ensure tests import rstest and proptest macros and
mark the NUL-walking call unsafe inside the test.
In `@src/commands/sort.rs`:
- Around line 630-679: Collapse the multiple unit tests into a single
parameterized rstest that iterates over input cases and expected outcomes for
SortOrderArg::parse; replace separate functions like
test_parse_sort_order_coordinate, test_parse_sort_order_queryname_default,
test_parse_sort_order_queryname_lexicographic,
test_parse_sort_order_queryname_natural,
test_parse_sort_order_template_coordinate,
test_parse_sort_order_unknown_subsort, test_parse_sort_order_unknown_order, and
test_parse_sort_order_empty_subsort with one #[rstest] that supplies tuples of
(input_str, expected_result_or_error), calling SortOrderArg::parse for each case
and asserting either equality for Ok variants (e.g., SortOrderArg::Coordinate,
Queryname, QuerynameNatural, TemplateCoordinate) or that the error string
contains the expected message (e.g., "unknown queryname sub-sort 'fast'",
"unknown sort order 'random'", "unknown queryname sub-sort ''"); ensure test
names/reference to SortOrderArg::parse remain in assertions so failures are
clear.
🪄 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: 0796a0b2-aad8-412b-9c62-8ec5c769edad
📒 Files selected for processing (16)
CLAUDE.mdbenches/core_functions.rscrates/fgumi-raw-bam/src/cigar.rscrates/fgumi-raw-bam/src/sort.rscrates/fgumi-raw-bam/src/tags.rssrc/commands/merge.rssrc/commands/sort.rssrc/lib/bam_io.rssrc/lib/progress.rssrc/lib/sort/external.rssrc/lib/sort/inline_buffer.rssrc/lib/sort/keys.rssrc/lib/sort/loser_tree.rssrc/lib/sort/mod.rssrc/lib/sort/radix.rssrc/lib/sort/raw.rs
✅ Files skipped from review due to trivial changes (4)
- src/lib/sort/loser_tree.rs
- CLAUDE.md
- src/lib/bam_io.rs
- benches/core_functions.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/fgumi-raw-bam/src/tags.rs
nh13
force-pushed
the
nh/sort-optimizations-v3
branch
from
April 3, 2026 02:04
fd77bd8 to
0b3145e
Compare
…RECORD, and queryname specifiers Optimize the fgumi sort command across all sort orders: Queryname sort: - Add sub-sort specifiers (queryname::natural, queryname::lex) with SS header tag - Port samtools-compatible natural_compare_nul comparator using pointer-walking - Add EMBEDDED_IN_RECORD optimization to skip writing variable-length keys in temp files - Add comprehensive criterion benchmarks for comparator strategies Coordinate sort: - Enable EMBEDDED_IN_RECORD for RawCoordinateKey — the 8-byte key is trivially re-extracted from BAM bytes, saving 8 bytes per record in temp file I/O Merge pipeline: - Replace manual binary heap with LoserTree in all spill/consolidation merge paths (log2(k) comparisons per element vs 2·log2(k) for heap) - Unify three near-identical merge functions (merge_chunks_keyed, merge_chunks_generic, merge_chunks_with_index) via shared ChunkSource enum and build_chunk_sources helper - merge_chunks_keyed now delegates to merge_chunks_generic Code cleanup: - Remove dead QuerynameRecordBuffer (zero callers, proved to regress when wired in) - Extract shared helpers for queryname key extraction and serialization - Deduplicate format_duration using crate::logging::format_duration - Remove dead digit-skipping loops in natural_compare - Restore proptest oracle tests for MSD radix sort Benchmark results (kapa-umi, 4 threads, 768M/thread): coordinate: 53.6s → 52.0s (-3.0% vs main, -31.8% vs samtools) qn_natural: 71.6s → 62.8s (-12.3% vs main, -37.0% vs samtools) qn_lex: new sort order (59.5s, -39.9% vs samtools) template_coord: 69.6s → 66.4s (-4.5% vs main, -29.8% vs samtools)
nh13
force-pushed
the
nh/sort-optimizations-v3
branch
from
April 3, 2026 02:07
0b3145e to
39404d6
Compare
Merged
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
queryname::natural,queryname::lex) with SS header tag and samtools-compatiblenatural_compare_nulcomparatorEMBEDDED_IN_RECORDfor coordinate and queryname keys, skipping redundant key writes in temp filesQuerynameRecordBuffercode and unify three near-identical merge functions via sharedChunkSourceenumBenchmark (kapa-umi, 4 threads, 768M/thread)
Test plan
cargo ci-test— 2147 tests passcargo ci-lintcleancargo ci-fmtclean