Support 0 coordinate in BCF

[pd3]

The 0 coordinate is valid in VCF specification, but the round-trip
VCF -> BCF -> VCF turns MT:0 into MT:4294967296. Add a check to
detect this overflow.

See #1475 and samtools/bcftools#1753

[daviesrob]

This need to be rebased on the current develop branch to get the tests to work properly.

The BCF specification says that POS should be an int32_t, so should we actually switch to le_to_i32() for this value? It would put a rather bigger restriction on the longest length supported, but going beyond INT_MAX is likely to cause problems in other areas (notably END tags) so it might not be a problem in practice.

[jkbonfield]

Out of interest, is the BCF spec just pandering to Java here or is there a valid reason for POS being signed? Specifically does BCF permit negative positions? It doesn't particularly make any sense to me and the VCF part of the spec indicates 0 and above only.

I see CHROM is also signed, which again makes no sense. I suspect this is just the same issue we had with BAM where some things were lazily defined.

As such I don't see the issue with our code treating pos as unsigned, as I'd argue it's the spec that is invalid and not the implementation.

Edit: although having said that I see we already use le_to_i32() for rid (chromosome). Can these be negative? AFAIK we don't have a notion of "*" chr as we doing in unmapped SAM, so the offset into the dictionary should always be positive. Putting -1 in it gave me an error, so that seems probable.

[pd3]

I also think it's a result of lazy definition, there is no reason for negative coordinates, even on circular chromosomes. Forced pushed a code that is in sync with the latest develop.

[daviesrob]

Precisely one negative coordinate is allowed in BCF, specifically -1, as already noted. Hence it's signed in the specification.

bcf_record_check() currently checks for negative contig IDs, but not negative positions (< -1). vcf_parse() uses hts_str2uint() for pos so doesn't allow negative values (remember vcf pos is bcf pos + 1). It also sets BCF_IS_64BIT if pos >= INT32_MAX which will cause bcf_write() to reject the record and advise using VCF instead. So it's quite difficult to get a pos value in a BCF file that big - I suspect you'd have to hand craft it.

So this fix will work, but pos > INT32_MAX in BCF isn't really supported.

[jkbonfield]

Ah of course, I'm forgetting bcf subtracts 1 from VCF, so the 0 goes negative.

Definitely in "wouldn't start from here" territory. :-)

[daviesrob]

I've now checked that this fixed the problem reported on bcftools sort, so:
Fixes #1475
Fixes samtools/bcftools#1753