Skip to content

D5: ERROR on a coordinate-unsorted input VCF #232

Description

@TimD1-bot

Note

Authorship: the content below was drafted by Claude Opus 5 (an AI coding agent) and
filed via gh under @TimD1-bot, a bot account operated by @TimD1. It reflects the
agent's analysis, not a statement authored by @TimD1.

Part of #47. A behavior change independent of stratification: an unsorted input VCF becomes a hard
failure instead of a quietly wrong result.

Problem

vcfdist does not reject a coordinate-unsorted VCF — it silently degrades on one. Records that go
backwards are dropped one at a time by the overlap filter (src/variant.cpp:905-913) with only a
WARN at verbosity > 1, so the run succeeds, produces a full set of output files, and every
denominator is quietly wrong. Nothing about the current failure mode looks like an error.

Half the guard already exists. #149 restored the contig-re-entry check
(src/variant.cpp:644-659), which errors when a record returns to a contig already left:

Unsorted %s VCF '%s', contig '%s' already parsed

So contigs never interleave. What is missing is the within-contig check.

The other property that currently holds is an accident: accepted records are non-decreasing per
haplotype, because the overlap filter skips a record unless pos >= prev_end[hap] and
prev_end[hap] == prev_pos + prev_rlen >= prev_pos. That is per-haplotype, it holds only for
accepted records, and it is a side effect of a filter written for an unrelated purpose.

Change

In the record loop, track the previous record position within the current contig and ERROR when
rec->pos moves backwards, reusing the existing "Unsorted %s VCF" phrasing
(src/variant.cpp:649) and resetting alongside prev_end/prev_type on a contig change
(src/variant.cpp:656-657). One integer comparison per record.

Why it lands before the code that needs it

#47's per-variant region-membership lookup uses a monotonic cursor per region set, which requires the
query stream to be non-decreasing within a contig. Resting that on the overlap filter's incidental
ordering would make stratified counts silently depend on an unrelated filter's semantics — and the
membership query at src/variant.cpp:882 runs before that filter at :905, so it would not even
be covered by it. Enforcing the precondition first means nothing downstream has to assume it.

Testing

Unit, in test_variant.cpp:

  • a within-contig backwards position ERRORs
  • the existing contig-re-entry ERROR still fires
  • a correctly sorted VCF is unaffected

Integration: a deliberately position-unsorted fixture asserting a non-zero exit. This one is worth an
integration case rather than only a unit test precisely because the current failure mode is a
successful run.

Release notes

Behavior change: runs that previously produced misleading numbers now fail loudly.

Depends on

Nothing. Can start immediately.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions