feat(variant): retain source records and carry their columns via htslib (#248) - #252
Open
TimD1-bot wants to merge 2 commits into
Open
feat(variant): retain source records and carry their columns via htslib (#248)#252TimD1-bot wants to merge 2 commits into
TimD1-bot wants to merge 2 commits into
Conversation
…ib (#248) summary.vcf synthesized its site-level columns, so ID, QUAL, FILTER, and INFO were hardcoded placeholders and only CHROM/POS/REF/ALT carried data. Each variant now holds a shared_ptr to the bcf1_t it was parsed from, and htslib copies the columns onto the output. parse_variants bcf_dups each record once, on first use, and shares it across every variant derived from it: the two halves of a split complex variant, the two co-located entries of a het-alt record. Shared provenance is now pointer equality, so rec_idx and rec_idxs are removed. The retained record is never mutated; every output record is a fresh bcf_dup, which is what lets two entries subset one source to different alleles. The parse header's ownership moves into a shared_ptr, since a retained record's tag IDs index its dictionaries, and summary_vcf_header folds both inputs in with bcf_hdr_merge after building vcfdist's own declarations, so the destination wins on a colliding ID. Record construction is ordered around two htslib behaviors. bcf_remove_allele_set does the Number=A/R/G subsetting, but fails on a genotype naming an allele it is removing, so a biallelic placeholder genotype is written first through the record's own 1-sample header. Every carried FORMAT field is then read before any is written, because bcf_update_format_* sets n_sample from the header it is given: a read after the first two-sample write computes its value count against a buffer still holding one sample and returns the next field's bytes. Metrics are unaffected: precision-recall-summary.tsv and phasing-summary.tsv are byte-identical on the committed chr20 fixture.
…skipping it Every FORMAT field on the record has to be rewritten for two samples before it is written out, because the first two-sample write reinterprets any field still holding one sample's values as garbage. A field whose declared type cannot be read therefore cannot be skipped, only refused. The VCF spec allows no such type -- Flag is INFO-only -- so this is reachable only from a header that declares one.
TimD1
reviewed
Aug 11, 2026
| - name: Install build dependencies | ||
| run: | | ||
| sudo apt-get update | ||
| sudo apt-get install -y libbz2-dev liblzma-dev libgtest-dev |
Owner
There was a problem hiding this comment.
rather than installing bcftools, can you use the htslib API to try reading in a gtest unit test?
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
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.
Note
Authorship: the content below was drafted by Claude Opus 5 (an AI coding agent) and
filed via
ghunder @TimD1-bot, a bot account operated by @TimD1. It reflects theagent's analysis, not a statement authored by @TimD1.
Addresses #248. Part of #48. Supersedes the string-based design of #104 and #105, which #239 made
obsolete by switching the writer to htslib.
Based on
devrather than stacked on #247: that PR's onlysrc/change isbed.cpp, this onetouches
variant.{h,cpp},phase.cpp, andcluster.cpp, so there is no overlap — and adev-based branch gets CI. The #246 dependency belongs to the next PR in the chain, which retains
variants vcfdist does not evaluate.
What was wrong
summary.vcfwas synthesized from the internalctgVariantsarrays rather than copied from theinput, so its site-level columns were hardcoded placeholders in
ctgVariants::set_var_record(
src/variant.cpp:544):ID=.,QUAL=.,FILTER=PASS,INFO=.. Only CHROM/POS/REF/ALT carrieddata, and those came from normalized alleles.
Change
Each variant now holds a
std::shared_ptr<bcf1_t>for the record it was parsed from, and htslibcopies the columns onto the output.
parse_variantsbcf_dups each record once, on first use, and shares it across every variantderived from it — the two halves of a split complex variant, the two co-located entries of a
het-alt record. Duplicating lazily means a record every haplotype discards is never copied.
rec_idxandrec_idxsare removed: theyexisted only as the key into a record-indexed store of reconstructed column text.
bcf_dup, which is whatlets two entries subset one source to different alleles without aliasing.
shared_ptr, since a retained record's tag IDs index itsdictionaries and are meaningless without it.
ctgVariantsgainshdrandcallsetalongsiderecs; a second constructor takes all three so a creation site cannot forget them.summary_vcf_headerfolds both inputs in withbcf_hdr_merge, after building vcfdist's owncontigs, FILTER, FORMAT, and samples — that ordering is what makes
bcf_hdr_merge'sdestination-wins-on-collision behavior keep vcfdist's contig lengths and its own FORMAT
cardinalities, both of which its records are written against.
Record construction is ordered around two htslib behaviors
bcf_remove_allele_set(htslib/vcfutils.h) does the allele subsetting, and per its contract"Number=A,R,G INFO and FORMAT fields will be updated accordingly" — which is what removes any
hand-rolled subsetting. Header cardinality needs no rewriting either: the declarations arrive saying
Number=A/R/Gand stay correct, since every output record is biallelic.Two steps fail silently if reordered:
bcf_remove_allele_setfails on a genotype naming an allele it is removing, which is every het-alt record. The
placeholder goes on through the record's own 1-sample header, not the output header, for
reason 2 below;
set_record_samplesoverwrites it with the real genotype afterwards.bcf_update_format_*setsn_samplefrom the header it is given, so the first two-sample write flips the record; a readafter that computes its value count as
nsmpl * nvalsagainst a buffer still holding one sampleand returns the neighbouring field's bytes. This is the
AD=17,7,97corruption. Every readtherefore happens against the 1-sample input header, before
bcf_translate.The re-keying reuses the existing
pad_per_alleleandupdate_formatoverloads, dispatching onbcf_hdr_id2typeforBCF_HT_INT,BCF_HT_REAL, andBCF_HT_STR. A FORMAT ID vcfdist writesitself is not carried — its own declaration wins the header merge, so carrying the input's values
under it would contradict the declaration.
Metrics are unchanged
On the committed chr20 fixture,
precision-recall-summary.tsvandphasing-summary.tsvarebyte-identical before and after, and the record count in
summary.vcfis unchanged at 72,584.bcftools viewandbcftools statsread the output with no complaint, andleaks --atExitreportszero leaks on both the chr20 and synthetic runs.
Verification
Each new test was checked to be load-bearing by mutating the implementation and confirming the right
test failed, since the tests for the writer were written alongside code rather than strictly before
it:
ADreports97,17, floats become4.36e+24bcf_remove_allele_setnever calledAD=17,7,97and un-subsetAC/PLCarriedValuesLandInTheOwningSampleColumnThe placeholder-genotype guard is the exception: it cannot be caught locally, because htslib 1.20
subsets a
1|2genotype without complaint. On the 1.17 that CI pins, removal fails with "Problemupdating genotypes". The guard is kept and the test comment says so. htslib 1.17 has no
osx-arm64build on conda-forge and the release tarball is not reachable from this environment, so CI is the
only place that behavior is exercised.
Two things worth a look
Peak RSS on chr20 rose 142 → 223 MiB (+57%, ~508 bytes per retained input record). The issue
expected retention to be a small fraction of the alignment working set; at this rate a 5M-record WGS
callset would add roughly 2.5 GB. The WGS-scale measurement was explicitly deferred, so this chr20
figure is the only data point — but it is larger than predicted and may deserve action before this
lands.
A matched query/truth pair reports only the query's carried columns. One record is written per
variant, so when both callsets call the same variant the record is built from the query's source and
the truth sample's own ID, QUAL, FILTER, and INFO have no second record to go on; its carried FORMAT
fields report missing. This follows the design's "re-key from 1 sample to 2, non-owning sample
missing", but it is a real gap rather than an obvious consequence, so it is called out here.
Tests
a het-alt, and across both halves of a split complex variant; the retained record keeps every source
allele. Header merge: input INFO/FORMAT/FILTER declarations survive, vcfdist's contig length and
FORMAT declarations win a collision, samples stay TRUTH/QUERY, a null input header is skipped.
Writer: site columns carried, unreported QUAL/FILTER carried as missing,
Number=1/fixed/Flagcarried verbatim,
Number=A/R/Gsubset per allele with theGcase asserted atalt_idx2,both htslib ordering invariants, sample-column ownership, and the retained record unmutated by
writing.
carried_fieldsfixture pair on the synthetic contig, because the chr20fixture cannot cover allele subsetting — every multiallelic record in it is unphased and so dropped
at parse time. The second test reads the output back with
bcftools, which is the only check that acarried field's value count still matches its declared cardinality;
bcftoolsis added to the CIapt-get installfor it.QUAL=50, which now reaches theoutput instead of
..