Skip to content

fix: select VCF output compression by file extension#16

Merged
nh13 merged 1 commit into
mainfrom
nh/mutate-bgzf-vcf-output
Jun 15, 2026
Merged

fix: select VCF output compression by file extension#16
nh13 merged 1 commit into
mainfrom
nh/mutate-bgzf-vcf-output

Conversation

@nh13

@nh13 nh13 commented Jun 15, 2026

Copy link
Copy Markdown
Contributor

Summary

holodeck mutate -o muts.vcf.gz wrote plain uncompressed text into a .gz-named file, so piping it straight into holodeck simulate failed with an opaque invalid BGZF header — you had to re-bgzip the file by hand first. The mirror trap bit methylate -o meth.vcf, which always wrote BGZF, producing a BGZF stream the reader then treated as plain text.

Root cause: noodles' VCF reader (vcf::io::reader::Builder::build_from_path) selects its codec from the file extension alone (.gz/.bgz → BGZF, else plain), never sniffing the magic bytes. The writers didn't honor that convention — mutate never compressed, methylate always did.

Fix

  1. New VcfWriter (src/vcf/writer.rs) picks the codec from the path the same way the reader does: BGZF when (and only when) it ends in .gz/.bgz, plain text otherwise. Modeled on FastqWriter/GoldenBamWriter — a Box<dyn Write> behind new/close, where close drops the inner writer to flush and emit the BGZF EOF block.
  2. Route both mutate and methylate through it.

Output now round-trips through simulate/methylate regardless of name.

Test plan

  • cargo ci-fmt clean
  • cargo ci-lint clean (clippy pedantic, -D warnings)
  • cargo ci-test — full suite passes, including new coverage:
    • writer.rs: extension detection (case-insensitive, final-extension-only); .gz → gzip magic + trailing BGZF EOF block; .vcf → uncompressed text
    • mutate.rs: .vcf.gz output is real BGZF and reads back through parse_variants_by_contig (the simulate path); .vcf output is plain text and also reads back
    • existing test_mutate / test_methylate / test_mutate_then_simulate integration tests unchanged and green
  • Manual CLI repro of the original bug: mutate -o muts.vcf.gzfile reports BGZFsimulate -v muts.vcf.gz succeeds (33 read pairs) with no manual re-bgzip

Summary by CodeRabbit

  • Bug Fixes
    • Fixed VCF output compression handling: mutate and methylate now automatically choose BGZF compression when the output filename ends with .gz or .bgz (case-insensitive), and write plain text otherwise. This restores compatibility for round-tripping between writers and VCF readers for both compressed and uncompressed outputs.
  • Tests
    • Added coverage to verify byte-level encoding and successful read-back for .vcf.gz and .vcf outputs.
  • Documentation
    • Updated the changelog entry and adjusted CLI help to reflect extension-based compression behavior.

@coderabbitai

coderabbitai Bot commented Jun 15, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 14d001ff-4d11-47dd-a12f-902fa1af3f34

📥 Commits

Reviewing files that changed from the base of the PR and between f7d00e8 and 3a3689e.

📒 Files selected for processing (5)
  • CHANGELOG.md
  • src/commands/methylate.rs
  • src/commands/mutate.rs
  • src/vcf/mod.rs
  • src/vcf/writer.rs
✅ Files skipped from review due to trivial changes (1)
  • CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/vcf/mod.rs
  • src/commands/mutate.rs

📝 Walkthrough

Walkthrough

Adds a new VcfWriter enum (Plain/Bgzf variants) in src/vcf/writer.rs that selects compression by output file extension (.gz/.bgz → BGZF; otherwise plain text). The mutate and methylate subcommands are updated to use this abstraction, replacing their manual writer setups. New tests verify round-trip encoding for both formats.

Changes

Extension-aware VcfWriter for mutate and methylate

Layer / File(s) Summary
VcfWriter abstraction and module export
src/vcf/mod.rs, src/vcf/writer.rs
Publicly exports a new writer submodule. Defines VcfWriter with Plain(BufWriter<File>) and Bgzf(bgzf::io::Writer<File>) variants, new to select codec by path extension, close to flush or call BGZF finish(), Write trait forwarding, is_bgzf_path helper, and unit tests for path detection and gzip/BGZF magic byte validation.
Integration into mutate and methylate, plus changelog
src/commands/mutate.rs, src/commands/methylate.rs, CHANGELOG.md
Replaces manual File/BGZF setup in mutate with VcfWriter::new and generalizes write_vcf_header to W: Write. Adds two integration tests verifying .vcf.gz produces BGZF bytes and .vcf produces plain text, both round-tripping through parse_variants_by_contig. Methylate is updated similarly: imports and wiring are changed, CLI help text describes extension-based compression, and output finalization uses vcf_out.close(). Changelog documents the fix.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐇 A .gz or a .vcf plain,
The rabbit checks the extension's name.
BGZF or text, the choice is neat,
No more mismatches—mutate's complete!
close() writes the EOF with care,
Round-trips succeed beyond compare. ✨

Suggested reviewers

  • tfenne
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'fix: select VCF output compression by file extension' directly and clearly describes the main fix: addressing VCF output compression behavior by introducing extension-based selection logic. It matches the core objective and primary change across the modified files.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@nh13
nh13 force-pushed the nh/mutate-bgzf-vcf-output branch from 861c217 to f7d00e8 Compare June 15, 2026 09:13
@nh13

nh13 commented Jun 15, 2026

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jun 15, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/commands/methylate.rs (1)

79-81: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Update the output flag help text to match extension-aware compression.

Line 79 still states BGZF-only output, but this command now writes plain text for non-.gz/.bgz paths. Please align the doc string with current behavior.

Suggested patch
-    /// Output methylation-annotated VCF (BGZF-compressed).
+    /// Output methylation-annotated VCF.
+    /// Compression follows the output extension:
+    /// `.gz`/`.bgz` => BGZF, otherwise plain text.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/commands/methylate.rs` around lines 79 - 81, The doc comment for the
output field in the methylate command currently states that output will be
BGZF-compressed, but the actual implementation now conditionally compresses
based on file extension (.gz or .bgz extensions result in BGZF compression,
while other extensions produce plain text output). Update the doc string for the
output field to accurately reflect this extension-aware compression behavior
instead of unconditionally stating BGZF compression.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@src/commands/methylate.rs`:
- Around line 79-81: The doc comment for the output field in the methylate
command currently states that output will be BGZF-compressed, but the actual
implementation now conditionally compresses based on file extension (.gz or .bgz
extensions result in BGZF compression, while other extensions produce plain text
output). Update the doc string for the output field to accurately reflect this
extension-aware compression behavior instead of unconditionally stating BGZF
compression.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 4ea3914a-b537-495c-9a13-fdc7cafab18c

📥 Commits

Reviewing files that changed from the base of the PR and between 07444d0 and f7d00e8.

📒 Files selected for processing (5)
  • CHANGELOG.md
  • src/commands/methylate.rs
  • src/commands/mutate.rs
  • src/vcf/mod.rs
  • src/vcf/writer.rs

@tfenne tfenne left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing this. LGTM. I merged your other PR first, so I think this one will need a quick rebase/merge and then we can merge it.

`mutate` always wrote uncompressed text and `methylate` always wrote BGZF,
regardless of the output path. noodles' VCF reader picks its codec from the
file extension alone (`.gz`/`.bgz` -> BGZF, else plain), never sniffing the
magic bytes. So `mutate -o muts.vcf.gz` produced plain text that `simulate`
rejected with an opaque BGZF error (you had to re-bgzip the file by hand), and
the mirror trap bit `methylate -o meth.vcf`, which wrote a BGZF stream the
reader then treated as plain text.

Add a `VcfWriter` that picks the codec from the path the same way the reader
does -- BGZF when (and only when) it ends in `.gz`/`.bgz`, plain text
otherwise -- and route both subcommands through it. The two codecs are enum
variants so `close` can call BGZF's consuming `finish` and surface a failed
EOF-block write, rather than swallowing it in `Drop`. Output now round-trips
through `simulate`/`methylate` regardless of name.
@nh13
nh13 force-pushed the nh/mutate-bgzf-vcf-output branch from f7d00e8 to 3a3689e Compare June 15, 2026 16:27
@nh13
nh13 merged commit 04632a3 into main Jun 15, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants