bam: remove BamRecord, some other prototype stuff - #45
Merged
Conversation
The read-path BamRecord struct had no production callers — every reader in the crate decodes via RecordStore::push_raw straight into slabs. Its only uses were as a round-trip oracle in tests and a separate fuzz target that fuzzed a code path production never takes. Drop the struct, decode helper, and accessors. Keep parse_header, ParsedHeader, compute_end_pos_from_raw, read2, and read4 — those are the genuinely-shared primitives still used by RecordStore::push_raw, the pre-push CIGAR scan, and OwnedBamRecord::from_raw_bam. Migrate the round-trip tests in owned_record.rs and writer.rs to push into a RecordStore and assert against SlimRecord/store accessors — which is what production runs anyway. Retarget the fuzz target the same way. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The method had zero callers outside its own two tests — its docstring already steered everyone to RecordStore::push_raw for bulk decode, and no production caller in seqair or rastair2 used it for the "edit one record" flow either. Keeping it meant maintaining a third copy of the "parse 32-byte header → walk qname/cigar/seq/qual/aux" walk (after RecordStore::push_raw and the now-deleted BamRecord::decode). Drop the method, the OwnedRecordError::Decode variant it raised, and the pos_from_bam_i32 helper that nothing else used. Drop the two tests that exercised it as a to_bam_bytes round-trip oracle — the writer's round-trip coverage already exercises that path through a RecordStore. After this, "decode raw BAM bytes" has exactly one entry point: RecordStore::push_raw. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Previously two writer paths each packed the BAM record fixed header inline: OwnedBamRecord::to_bam_bytes (the typed-record path) and BamWriter::write_store_record_inner (the slab-resident path). Both duplicated the bin_mq_nl / flag_nc bit-packing and the eight LE-byte appends. A field added to one site without the other (e.g. the bin recompute, or the n_cigar_op casts) was a bug class waiting to happen. Extract `bam::record::encode_fixed_header` as the inverse of the existing `parse_header`, taking a `FixedHeaderFields` struct in BAM wire types (pos/next_pos already resolved to -1 for unmapped, and l_read_name as u8 to encode the qname ≤ 254 invariant in the type). Both call sites now construct the struct with their own validated casts and call the shared encoder. The resulting BAM bytes are unchanged — verified by the existing writer round-trip tests and the broader suite. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Both write paths (write -> write_inner via OwnedBamRecord::to_bam_bytes, and write_store_record -> write_store_record_inner via slab + the shared encode_fixed_header) ended with the same dance: size-cap check, pre-write index validation (mapped-without-reference rejection), BGZF block-size + bytes write, then index dispatch with the placed-unmapped vs mapped vs fully-unmapped fork. Two copies of the index dispatch meant a fix in one site silently diverged from the other — already happened once with the beg.saturating_add(1) clamp. Extract `finalize_record(ref_id, is_unmapped, beg, end_pos)` for the shared tail. Each writer entry now: fill self.buf with record bytes, compute (beg, end_pos), call finalize_record. Net -35 lines in writer.rs and one place to fix any future change to size limits, index pushing, or BGZF write order. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Member
Author
|
still not very clean. will have to make some examples and proof/refine the APIs |
4 examples rewritten, `SegmentOptions::default()` added Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Centralizes SAM CIGAR formatting so callers stop hand-rolling the op-code-to-char match. Drops the inline fmt_cigar helper from the realignment example. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Wraps the four-line MM/ML/seq/is_reverse extraction in a typed constructor returning Result<Option<Self>, FromRecordError>: Ok(None) when MM is absent, typed errors for wrong-type tags or malformed MM/ML. Forgetting to thread `is_reverse` through `parse` silently produces wrong methylation calls — the constructor pulls it from rec.flags itself. Drops the manual extraction from examples/base_mods.rs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Drops the opaque `build_index: bool` and positional `level: i32` from `BamWriter::from_path` / `from_path_with_level` / `from_stdout` / `new_inner` in favor of a `BamWriterBuilder` reached via `BamWriter::builder(path, header)` (file target) or `BamWriter::build_over(writer, header)` (arbitrary `Write` sink). Setters: `write_index(bool)` and `compression_level(i32)`. The header is required positionally; everything else has a default. Header is still written eagerly during `build()`. Spec rules and all in-tree call sites (bench, example, tests, internal tests) updated. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Re-attach the spec rules orphaned by the BamRecord -> SlimRecord
migration to their new code homes (BamFlags predicates, parse_header,
decode_bases_into, RecordStore::{seq_at,aux}, push_raw SIMD path,
matching/indel pre-compute fields). Rename remaining BamRecord prose
references to OwnedBamRecord across the BAM-record-builder/writer/flags
specs, point r[bam_writer.index_coproduction] at the new
BamWriterBuilder::write_index API, and replace the BamRecord mention in
r[perf.precompute_matches_indels] / base-quality field-usage with
SlimRecord. Drop the contradictory "no universally correct value"
paragraph from SegmentOptions::new now that Default exists, and note on
BaseModState::from_record that we reject MM-with-deltas records lacking
ML (stricter than SAM 1.6). Extract the duplicated push-raw test
scaffold into bam::test_util.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The &[u8] FromAuxValue impl previously accepted both Z and H tags, returning the raw stored bytes for either. This silently misinterpreted H tags: their wire encoding is ASCII hex digits, so a caller fetching &[u8] would get the digits rather than the underlying byte array. Restrict the &[u8] impl to Z, and add a HexBytes wrapper with its own FromAuxValue impl plus a decode() method that materializes the byte array. H tags now require an explicit fetch as HexBytes; Z tags do not coerce into HexBytes. Searched the workspace — no production caller was relying on H acceptance via &[u8]. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The builder types are part of the writer's public API (BamWriter::builder and BamWriter::build_over both return one), but were only reachable as seqair::bam::writer::BamWriterBuilder. Surface them at seqair::bam so they line up with BamWriter and BamWriteError. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
When build_over(writer, header).write_index(true).build() is called, there is no sidecar location for a .bai file and BAI virtual offsets only make sense against a seekable BAM file — building an IndexBuilder would just waste work and hand the caller something that points into a stream they have already streamed away. Accept write_index(true) on the ToWriter path for API uniformity, but drop the request: log info! and skip IndexBuilder construction. The ToPath path is unchanged. Migrate the existing index tests that used build_over(&mut Vec) to tempfile-backed BamWriter::builder(path) targets, and add a new write_index_on_writer_target_is_soft_noop test that pins down the new behavior. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
BamWriter::builder(path, header) and BamWriter::build_over(writer, header) hung off the writer type itself even though both returned a builder, not a writer. Move them to BamWriterBuilder::to_path and BamWriterBuilder::to_writer so the configure-then-build surface lives in one place: BamWriterBuilder is the configuration type, BamWriter is what .build() produces. Update every test/bench/example caller and the bam_writer.create_* spec rules. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
IndexedBamReader's pre-filter unconditionally dropped placed-unmapped reads (flag 0x4 with a valid tid) before they reached the customize layer. Downstream tools that want htslib-`view`-equivalent semantics (e.g. perbase `only-depth -x`) had no way to see them and worked around it by injecting `-F 4` at the call site. Add a builder-style `keep_unmapped(bool)` setter (and `keeps_unmapped()` inspector). Default `false` preserves htslib-pileup-correct behavior; `true` lets `filter_raw` / `filter` decide. `fork()` propagates the flag. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replaces the `for idx in 0..store.len() as u32 { let rec = store.record(idx); ... }`
boilerplate with `for (idx, rec) in store.iter() { ... }`. Yielded `idx`
matches the `u32` returned by `push_raw`/`push_fields` so callers can still
reach the variable-length slabs via `store.cigar(idx)` etc., or via the
`SlimRecord::cigar(&store)` getters.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The yielded `u32` was redundant — `SlimRecord` already carries `cigar(&store)` / `qname(&store)` / `seq(&store)` / `qual(&store)` / `aux(&store)` getters that cover every shared-reference access pattern, and the only operations that need the index are mutable (`set_alignment`, `extra_mut`, …) which can't run during iteration anyway. Callers that genuinely need a position can use `records().enumerate()`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Measured ~8× wall-clock speedup on this repo (58.9s → 7.0s) because nextest parallelizes test binaries instead of running them serially. Repo already ships a nextest config at .config/nextest.toml. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Mapped records with SEQ=* (seq_len=0) were silently dropped from every pileup column because the qual slab lookup returned None. htslib's pileup keeps them — bam_seqi reads beyond the empty SEQ buffer, the 0xFF qual sentinel decodes to N, and the alignment counts toward depth(). Any BAM with secondary alignments lacking sequence (a common SAM convention) saw a different coverage profile under seqair than htslib; perbase #107 had to ignore an empty-SEQ parity test waiting for this fix. Match/Insertion ops on a seq_len=0 record now emit Base::Unknown + BaseQuality::UNAVAILABLE, matching htslib's depth() and match_depth(). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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.
No description provided.