Support VCF v4.3 (read only). #1359

cmnbroad · 2019-04-30T21:17:31Z

Read-only support for VCF v4.3:

ALT header lines are now modeled with a new VCFSimpleHeaderLine-derived VCAltHeaderLine class (previously these were modeled directly with VCFSimpleHeaderLine objects). SAMPLE, META and PEDIGREE header lines are also modeled with VCFSimpleHeaderLine-derived classes (previously these would have been tolerated on read, even for v4.x files, and modeled as VCFHeaderLine). VCFHeaderLine does not allow attribute queries.
Supports reading files with UTF8 character encoding (v4.3).
Supports percent-encoded values (v43).
Adds a VCFHeaderVersion field to VCF header; once a VCFHeader is established as v4.3, it cannot be transitioned to any other version.
Merging a v4.3 header with other headers is only allowed if all headers being merged are v4.3.
VCFWriter throws on any attempt to write out a version v43 header.
Except for the class change of ALT header lines, these changes should be invisible to existing callers.

codecov-io · 2019-04-30T23:03:22Z

Codecov Report

Merging #1359 into master will increase coverage by 0.096%.
The diff coverage is 94.161%.

@@               Coverage Diff               @@
##              master     #1359       +/-   ##
===============================================
+ Coverage     67.863%   67.959%   +0.096%     
- Complexity      8289      8326       +37     
===============================================
  Files            563       570        +7     
  Lines          33718     33797       +79     
  Branches        5659      5660        +1     
===============================================
+ Hits           22882     22968       +86     
+ Misses          8656      8649        -7     
  Partials        2180      2180

Impacted Files	Coverage Δ	Complexity Δ
src/main/java/htsjdk/variant/vcf/VCFEncoder.java	`86.935% <ø> (ø)`	`72 <0> (ø)`	⬇️
src/main/java/htsjdk/variant/vcf/VCFCodec.java	`77.273% <0%> (ø)`	`17 <0> (+1)`	⬆️
...n/java/htsjdk/variant/vcf/VCFSampleHeaderLine.java	`100% <100%> (ø)`	`1 <1> (?)`
src/main/java/htsjdk/variant/vcf/VCFHeader.java	`90.058% <100%> (+1.243%)`	`76 <8> (+10)`	⬆️
...main/java/htsjdk/variant/vcf/VCFHeaderVersion.java	`71.875% <100%> (+7.986%)`	`13 <2> (-5)`	⬇️
...va/htsjdk/variant/vcf/VCFHeaderLineTranslator.java	`93.976% <100%> (+1.383%)`	`3 <1> (ø)`	⬇️
src/main/java/htsjdk/variant/vcf/VCFConstants.java	`87.5% <100%> (+87.5%)`	`1 <0> (+1)`	⬆️
src/main/java/htsjdk/variant/vcf/VCFUtils.java	`44.444% <100%> (+6.391%)`	`17 <6> (+6)`	⬆️
...ava/htsjdk/variant/vcf/VCFStandardHeaderLines.java	`77.632% <100%> (+1.243%)`	`13 <6> (+2)`	⬆️
...main/java/htsjdk/variant/vcf/VCFAltHeaderLine.java	`100% <100%> (ø)`	`2 <2> (?)`
... and 30 more

src/test/java/htsjdk/variant/vcf/VCFCodec43FeaturesTest.java

pshapiro4broad · 2019-05-06T19:05:32Z

src/test/java/htsjdk/variant/vcf/VCFCodec43FeaturesTest.java

+    public void testVCF43ReadUTF8Attributes() {
+        final Tuple<VCFHeader, List<VariantContext>> entireVCF = VariantContextTestUtils.readEntireVCFIntoMemory(
+                Paths.get(TEST_PATH + TEST_43_UTF8_FILE));
+        final List<VCFHeaderLine> headerLines = getIDHeaderLinesWithKey(entireVCF.a,"COMMENT");


whitespace

Suggested change

final List<VCFHeaderLine> headerLines = getIDHeaderLinesWithKey(entireVCF.a,"COMMENT");

final List<VCFHeaderLine> headerLines = getIDHeaderLinesWithKey(entireVCF.a, "COMMENT");

src/test/java/htsjdk/variant/vcf/VCFCodec43FeaturesTest.java

src/main/java/htsjdk/variant/vcf/VCFTextTransformer.java

pshapiro4broad · 2019-05-06T19:15:30Z

src/main/java/htsjdk/variant/vcf/VCFTextTransformer.java

+ * needs to be done.
+ */
+public class VCFTextTransformer {
+    final static private String ENCODING_SENTNEL_STRING = "%";


typo

Suggested change

final static private String ENCODING_SENTNEL_STRING = "%";

final static private String ENCODING_SENTINEL_STRING = "%";

src/main/java/htsjdk/variant/vcf/VCFHeaderVersion.java

pshapiro4broad · 2019-05-06T19:21:35Z

src/main/java/htsjdk/variant/vcf/VCFHeaderLineTranslator.java

@@ -44,6 +44,7 @@
        mapping.put(VCFHeaderVersion.VCF4_0,new VCF4Parser());
        mapping.put(VCFHeaderVersion.VCF4_1,new VCF4Parser());
        mapping.put(VCFHeaderVersion.VCF4_2,new VCF4Parser());
+        mapping.put(VCFHeaderVersion.VCF4_3,new VCF4Parser());


Suggested change

mapping.put(VCFHeaderVersion.VCF4_3,new VCF4Parser());

mapping.put(VCFHeaderVersion.VCF4_3, new VCF4Parser());

src/main/java/htsjdk/variant/vcf/VCFHeaderLineTranslator.java

lbergelson

@cmnbroad Looks sane to me. I have some worries about baking utf-8 characters into our source files, we've seen problems with them before. I think we should try to set the source file encoding properly so we no longer have issues around that before we include them.

lbergelson · 2019-05-10T17:41:21Z

src/main/java/htsjdk/variant/vcf/VCFHeader.java

@@ -146,6 +167,52 @@ public VCFHeader(final Set<VCFHeaderLine> metaData, final List<String> genotypeS
        buildVCFReaderMaps(genotypeSampleNames);
    }

+    /**
+     * Establish the header version for this header. If the header version has already been established
+     * for this header, the new version will be subject to version transition vaidation.


Suggested change

* for this header, the new version will be subject to version transition vaidation.

* for this header, the new version will be subject to version transition validation.

lbergelson · 2019-05-10T17:43:05Z

src/main/java/htsjdk/variant/vcf/VCFHeader.java

+     * Throw if {@code fromVersion} is not compatible with a {@code toVersion}. Generally, any version before
+     * version 4.2 can be up-converted to version 4.2, but not to version 4.3. Once a header is established as
+     * version 4.3, it cannot be up or down converted, and it must remain at version 4.3.
+     * @param fromVersion version


maybe mention something about the behavior of null fromVersion here?

lbergelson · 2019-05-10T17:43:39Z

src/main/java/htsjdk/variant/vcf/VCFHeader.java

+        if (fromVersion != null) {
+            if (toVersion.isAtLeastAsRecentAs(VCFHeaderVersion.VCF4_3)) {
+                if (!fromVersion.isAtLeastAsRecentAs(VCFHeaderVersion.VCF4_3)) {
+                    // we're trying to go from pre-v43 to v43+


I'd write it v4.3 for slightly more clarity.

lbergelson · 2019-05-10T17:46:22Z

src/main/java/htsjdk/variant/vcf/VCFHeaderLineTranslator.java

+        map.put(VCFHeaderVersion.VCF4_3, new VCF4Parser());
+        map.put(VCFHeaderVersion.VCF3_3, new VCF3Parser());
+        map.put(VCFHeaderVersion.VCF3_2, new VCF3Parser());
+        mapping = Collections.unmodifiableMap(map);


lbergelson · 2019-05-10T18:30:02Z

src/test/java/htsjdk/variant/variantcontext/VariantContextTestUtils.java

+import java.util.ArrayList;
+import java.util.List;
+
+public class VariantContextTestUtils {


Instead of adding this new class I would just push this down into VariantBaseTest. Unless you think it's generally useful for downstream users.. in which case we should make this a final class with a private constructor.

lbergelson · 2019-05-10T19:36:23Z

src/main/java/htsjdk/variant/vcf/VCFSampleHeaderLine.java

+    private static final long serialVersionUID = 1L;
+
+    public VCFSampleHeaderLine(String line, VCFHeaderVersion version) {
+        super(VCFConstants.SAMPLE_HEADER_KEY, new VCF4Parser().parseLine(line, null));


The VCF4Parser is stateless so maybe there should be a static instance of it instead of creating new ones all over the place.

Yes, VCFHeaderLineTranslator already keeps static ones. Done.

lbergelson · 2019-05-10T19:43:05Z

src/main/java/htsjdk/variant/vcf/VCFHeaderVersion.java

@@ -114,10 +115,12 @@ private static String clean(String s) {
     */
    public boolean isAtLeastAsRecentAs(final VCFHeaderVersion target) {


Am I being dumb, or can this method be replaced by a >= sign on the ordinal value?

As long as the enum is kept in order, yes. That seems less error prone too, as I almost missed making this change at all.

Done, with tests to keep it from regressing.

lbergelson · 2019-05-15T16:52:44Z

src/main/java/htsjdk/variant/vcf/VCFTextTransformer.java

+     * @return a string with all encoded characters replaced with the corresponding character
+     * @throws TribbleException if the the encoding is uninterpretable
+     */
+    protected static String decodePercentEncodedChars(final String rawText) {


It's not totally clear to me if general percent encoding is allowed by the vcf 4.3 spec or only percent encoding of specific characters.

using the capitalized percent encoding: %3A : (colon) %3B ; (semicolon) %3D = (equal sign) %25 % (percent sign) %2C , (comma) %0D CR %0A LF %09 TAB

lbergelson · 2019-05-15T16:53:30Z

src/main/java/htsjdk/variant/vcf/VCFTextTransformer.java

+     * @return the decoded string
+     * @throws TribbleException if the the encoding is uninterpretable
+     */
+    public String transformEncodedText(final String rawPart) {


this might be more clearly named decode?

lbergelson · 2019-05-15T17:38:22Z

src/test/java/htsjdk/variant/vcf/VCFCodec43FeaturesTest.java

+
+/*
+ **************************************************************************************************
+ * IMPORTANT NOTE: this class contains string constants that contain embedded non-ASCII characters


I'm worried that this will cause problems similar to broadinstitute/gatk#5934. I think we might want to figure out how to fix the encoding in the build.gradle so it doesn't use system specific encodings during build.

I think we can probably fix it with something like

tasks.withType(JavaCompile) { options.encoding = "UTF-8" } tasks.withType(Javadoc) { options.addStringOption('encoding', 'UTF-8') }

added to build.gradle but I haven't reproduced the issue locally to test yet.

…nd build.gradle change).

cmnbroad · 2019-05-23T19:48:23Z

Back to you @lbergelson. I relaxed the percent encoding to be more permissive and only transfor encodings if the % is followed by two legitimate hex-digit values, otherwise the raw characters are passed through untranslated.

droazen · 2019-05-30T19:00:18Z

@lbergelson Waiting for a final 👍 from you on this one.

lbergelson

👍

pshapiro4broad

Just noticed I had some comments from a review I didn't post. Here they are, up to you if you want to do anything with them.

pshapiro4broad · 2019-05-07T13:07:21Z

src/main/java/htsjdk/variant/variantcontext/writer/VCFWriter.java

@@ -258,10 +260,21 @@ public void add(final VariantContext context) {

    @Override
    public void setHeader(final VCFHeader header) {
+        rejectVCFV43Headers(header);


Why check here as well as in write? Is it illegal to create a VCFWriter in this state, even if it's never used to write a VCF?

pshapiro4broad · 2019-05-07T13:09:58Z

src/main/java/htsjdk/variant/variantcontext/writer/VCFWriter.java

        if (outputHasBeenWritten) {
            throw new IllegalStateException("The header cannot be modified after the header or variants have been written to the output stream.");
        }
        this.mHeader = doNotWriteGenotypes ? new VCFHeader(header.getMetaDataInSortedOrder()) : header;
        this.vcfEncoder = new VCFEncoder(this.mHeader, this.allowMissingFieldsInHeader, this.writeFullFormatField);
    }
+
+    // writing vcf v4.3 is not implemented
+    private static void rejectVCFV43Headers(final VCFHeader targetHeader) {


In the interest of adding future checks, how about calling this ensureValidToWrite() or something like that? Personally, I use 'ensure' for (non-testing) methods that assert a state and throw an error if the state isn't valid. 'check' is for methods that assert but return a result code instead of throwing. Then the specific comment about checking for 4.3 can be moved below, to the if.

pshapiro4broad · 2019-05-07T13:12:38Z

src/main/java/htsjdk/variant/vcf/VCFConstants.java

+
+    public static final String PEDIGREE_HEADER_KEY = "PEDIGREE";
+    public static final String PEDIGREE_HEADER_START = VCFHeader.METADATA_INDICATOR + PEDIGREE_HEADER_KEY;
+    public static final int PEDIGREE_HEADER_OFFSET = PEDIGREE_HEADER_START.length() + 1;


Thank you for not continuing the use of magic numbers here!

pshapiro4broad · 2019-05-07T13:25:09Z

src/main/java/htsjdk/variant/vcf/AbstractVCFCodec.java

+     * @param targetVersion the version for which a transformer is bing requested
+     * @return a {@link VCFTextTransformer} suitable for the targetVersion
+     */
+    private static VCFTextTransformer getTextTransformerForVCFVersion(final VCFHeaderVersion targetVersion) {


Since this is not overridable, what about moving it to VCFTextTransformer and make it a factory method, e.g. VCFTextTransformer.fromVersion().

Another possible place for this is VCFHeaderVersion, e.g. VCFHeaderVersion.createTextTransformer().

cmnbroad force-pushed the cn_vcf43_scratch branch 2 times, most recently from 5a4e48b to a9dae56 Compare April 30, 2019 23:03

cmnbroad force-pushed the cn_vcf43_scratch branch 4 times, most recently from 4ed4850 to d28da51 Compare May 6, 2019 14:36

Support VCF v4.3 (read only).

d28da51

droazen assigned lbergelson May 6, 2019

droazen requested a review from lbergelson May 6, 2019 16:20

pshapiro4broad reviewed May 6, 2019

View reviewed changes

Respond to Phil's review comments.

25552cb

lbergelson requested changes May 15, 2019

View reviewed changes

cmnbroad added 3 commits May 17, 2019 10:46

Address most code review comments (except percent encoding question a…

dc7ecd3

…nd build.gradle change).

Add UTF-8 source declarations for compile and javadoc tasks.

7579dcb

Make percent encoding extremely lenient.

97489a3

lbergelson approved these changes May 31, 2019

View reviewed changes

lbergelson merged commit d5ac863 into samtools:master May 31, 2019

pshapiro4broad reviewed Jun 5, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support VCF v4.3 (read only). #1359

Support VCF v4.3 (read only). #1359

cmnbroad commented Apr 30, 2019 •

edited

Loading

codecov-io commented Apr 30, 2019 •

edited

Loading

pshapiro4broad May 6, 2019

cmnbroad May 7, 2019

pshapiro4broad May 6, 2019

pshapiro4broad May 6, 2019

lbergelson left a comment

lbergelson May 10, 2019

cmnbroad May 16, 2019

lbergelson May 10, 2019

cmnbroad May 16, 2019

lbergelson May 10, 2019

cmnbroad May 16, 2019

lbergelson May 10, 2019

lbergelson May 10, 2019

cmnbroad May 16, 2019

lbergelson May 10, 2019

cmnbroad May 16, 2019

lbergelson May 10, 2019

cmnbroad May 16, 2019 •

edited

Loading

cmnbroad May 17, 2019

lbergelson May 15, 2019

lbergelson May 15, 2019

cmnbroad May 16, 2019

lbergelson May 15, 2019

cmnbroad commented May 23, 2019 •

edited

Loading

droazen commented May 30, 2019

lbergelson left a comment

pshapiro4broad left a comment

pshapiro4broad May 7, 2019

pshapiro4broad May 7, 2019

pshapiro4broad May 7, 2019

pshapiro4broad May 7, 2019

	final List<VCFHeaderLine> headerLines = getIDHeaderLinesWithKey(entireVCF.a,"COMMENT");
	final List<VCFHeaderLine> headerLines = getIDHeaderLinesWithKey(entireVCF.a, "COMMENT");

	final static private String ENCODING_SENTNEL_STRING = "%";
	final static private String ENCODING_SENTINEL_STRING = "%";

	mapping.put(VCFHeaderVersion.VCF4_3,new VCF4Parser());
	mapping.put(VCFHeaderVersion.VCF4_3, new VCF4Parser());

	* for this header, the new version will be subject to version transition vaidation.
	* for this header, the new version will be subject to version transition validation.

		@@ -114,10 +115,12 @@ private static String clean(String s) {
		*/
		public boolean isAtLeastAsRecentAs(final VCFHeaderVersion target) {

Support VCF v4.3 (read only). #1359

Support VCF v4.3 (read only). #1359

Conversation

cmnbroad commented Apr 30, 2019 • edited Loading

codecov-io commented Apr 30, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lbergelson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmnbroad May 16, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmnbroad commented May 23, 2019 • edited Loading

droazen commented May 30, 2019

lbergelson left a comment

Choose a reason for hiding this comment

pshapiro4broad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmnbroad commented Apr 30, 2019 •

edited

Loading

codecov-io commented Apr 30, 2019 •

edited

Loading

cmnbroad May 16, 2019 •

edited

Loading

cmnbroad commented May 23, 2019 •

edited

Loading