Refactor/simplify - #104
Merged
Merged
Conversation
…ants The error test module was missing #[cfg(test)], so its test-only enum compiled into release builds. Also removes five never-constructed WriteError variants and CbqError::MissingSequenceOnSequencingRecord, folds the one-variant BuilderError into HeaderError::MissingSequenceLength, folds FastxEncodingError into WriteError, and trims tests that only exercised thiserror's derive macro. BREAKING: error::BuilderError and error::FastxEncodingError are removed.
StreamWriter was a pure delegating wrapper around Writer<BufWriter<W>>. Callers may compose directly (see network_streaming example). The write_record/write_paired_record methods were already deprecated in favor of push(SequencingRecord). The write_flag and write_buffer free functions become one private little-endian helper. BREAKING: bq::StreamWriter, bq::StreamWriterBuilder, and the deprecated write_record/write_paired_record methods (bq and vbq) are removed.
The fastx encoder previously called ingest() on every batch, which for CBQ flushes the thread's incomplete block and compresses it under the global lock. Switch to ingest_completed() per batch (moves only pre-compressed bytes) with a final ingest() on thread completion, so blocks stay full across batches. This is what I discovered had large perf improvements with bqtools. This ports the pattern from examples/write.rs, which is now deleted as a redundant 292-line clone of encode_fastx (see examples/auto-write.rs).
Removes API with no callers in-repo or in downstream consumers (bqtools, cyto): the vestigial BlockIndex::from_vbq file scanner, redundant FileHeader constructors and set_bitsize setters, the SequencingRecord getters (fields are now pub), the unused opt_* builder setters, and RefRecord::config/set_id. The from_*_header constructors shrink to struct-update syntax and the inherent BinseqReader::process_parallel_range becomes a one-line delegation to the ParallelReader impl — both kept because bqtools uses them. BREAKING: vbq::BlockIndex::from_vbq, FileHeader::new/new_extended/ set_bitsize (bq and vbq), vbq::FileHeader::with_capacity, and the SequencingRecord getters are removed; SequencingRecord fields are now public.
Removes the unused usage()/bytes_written() writer accessors, the with_compression_level builder setter (only the with_optional_* variant is called), the write-only num_npos block field, and a stale dead_code allow. The BlockIter struct collapses into iter().copied(), and average_block_size (used by bqtools info) is rewritten over windows(2) — same value, but single-block files now report 0.0 instead of NaN.
Every use was fixed-offset LE read/write. Two helpers (read_u64_le / read_u32_le) cover the reads. writes become copy_from_slice/write_all over to_le_bytes.
auto_impl was a proc-macro dep serving one attribute. Generated &R / &mut R impls have no callers here or downstream. memchr is only used by examples/grep.rs, so belongs in dev-dep. BREAKING: &R and &mut R no longer implement BinseqRecord.
The two Encoders were identical except that bq's embedded a FileHeader for fixed-length validation. One crate-private Encoder (parameterized only by BitSize) replaces both. bq::Writer now stores its header directly and validates sequence lengths in push() before encoding. BREAKING: bq::Encoder is no longer part of the public API.
- BatchRecord wraps RefRecord instead of re-implementing it - cbq thread partitioning uses chunks(div_ceil); - the cbq Span struct becomes Range<usize>; - vbq's flush_block becomes a method and push's twin branches merge (a full block now flushes before encoding, so a policy-skipped record may close a block earlier. files remain identical in format; - vbq index constructor pairs collapse and IndexHeader stops storing constant fields; - the fastx encode wrappers inline into one match.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request performs a major cleanup of the internal and public APIs, removing several runtime dependencies (such as byteorder, num_cpus, and auto_impl) and consolidating the nucleotide encoding logic into a shared Encoder struct. It also removes deprecated writer methods in favor of the unified push(SequencingRecord) API and simplifies internal representations like replacing custom spans with standard ranges. The review feedback highlights critical improvement opportunities: three comments point out potential panics from direct slice indexing in the bq, vbq, and cbq readers when handling corrupted files, recommending safe bounds checking with .get(). Additionally, a logical issue in vbq::Writer::push is identified where blocks may be prematurely flushed before checking if a record is skipped by the nucleotide policy.
The push refactor flushed pre-encoding so a policy-skipped record that would have overflowed the block could close it early and fragment output. Encode first then flush via a split-borrow helper. Restores 0.9.x block packing exactly; adds a regression test asserting skipped records leave output identical. Also reattaches push's doc comment, which the earlier refactor had orphaned onto flush_block.
anyhow currently is still in the dependency tree unless sucds can remove it (pr in progress). fixes an error where anyhow was not actually an optional feature since ? was auto-impl with thiserror. removing paraseq from the default build path and updating docs to show under optional features. Also update the example to write to CBQ
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.