Adding support for 0-length B arrays in SAM files to conform to 1.6 spec #1194

lbergelson · 2018-10-10T21:59:35Z

Making changes to allow 0-length arrays in SAM because this is allowed in SAM 1.6

Checklist

Code compiles correctly
New tests covering changes and new functionality
All tests passing
Extended the README / documentation, if necessary
Is not backward compatible (breaks binary or source compatibility)

lbergelson · 2018-10-10T22:03:31Z

@yfarjoun Could you take a look at this to make sure I haven't done anything crazy... I removed all the checks for 0 lengths that I could find and the test passes for bam/sam/cram but it's possible there's something else relying on it that I haven't though of.

codecov-io · 2018-10-10T22:37:51Z

Codecov Report

Merging #1194 into master will increase coverage by 0.302%.
The diff coverage is 88.889%.

@@               Coverage Diff               @@
##              master     #1194       +/-   ##
===============================================
+ Coverage     68.414%   68.715%   +0.302%     
- Complexity      8019      8065       +46     
===============================================
  Files            542       543        +1     
  Lines          32701     32751       +50     
  Branches        5530      5541       +11     
===============================================
+ Hits           22372     22505      +133     
+ Misses          8125      8043       -82     
+ Partials        2204      2203        -1

Impacted Files	Coverage Δ	Complexity Δ
src/main/java/htsjdk/samtools/SAMRecord.java	`67.678% <ø> (+0.274%)`	`314 <0> (-1)`	⬇️
src/main/java/htsjdk/samtools/TextTagCodec.java	`77.922% <100%> (+24.374%)`	`54 <1> (+21)`	⬆️
src/main/java/htsjdk/samtools/SAMLineParser.java	`62.36% <50%> (+2.583%)`	`41 <0> (+3)`	⬆️
...sjdk/samtools/util/Md5CalculatingOutputStream.java	`66.667% <0%> (-7.692%)`	`7% <0%> (-1%)`
...ava/htsjdk/variant/vcf/VCFStandardHeaderLines.java	`75.581% <0%> (-0.475%)`	`11% <0%> (ø)`
...ain/java/htsjdk/samtools/cram/io/CramIntArray.java	`88.889% <0%> (ø)`	`4% <0%> (?)`
...rc/main/java/htsjdk/samtools/SamReaderFactory.java	`67.464% <0%> (+0.957%)`	`7% <0%> (ø)`	⬇️
...n/java/htsjdk/samtools/cram/structure/ReadTag.java	`71.806% <0%> (+3.524%)`	`65% <0%> (+3%)`	⬆️
... and 8 more

pshapiro4broad

minor comments

pshapiro4broad · 2018-10-11T14:30:33Z

src/main/java/htsjdk/samtools/TextTagCodec.java

        final int length = Array.getLength(value);
+        if(length == 0){


missing space after if

pshapiro4broad · 2018-10-11T14:32:36Z

src/main/java/htsjdk/samtools/TextTagCodec.java

@@ -316,6 +326,16 @@ private Object covertStringArrayToObject(final String stringVal) {
        }
    }

+    private static Object createEmptyArray(char elementType) {
+        switch( Character.toLowerCase(elementType) ) {
+            case 'c': { return new byte[0]; }


Don't need { } around case or default body. Also, usually case body statements are formatted across lines.

reformatted

pshapiro4broad · 2018-10-11T14:34:00Z

src/main/java/htsjdk/samtools/TextTagCodec.java

@@ -316,6 +326,16 @@ private Object covertStringArrayToObject(final String stringVal) {
        }
    }

+    private static Object createEmptyArray(char elementType) {
+        switch( Character.toLowerCase(elementType) ) {


odd whitespace here, I would expect to see switch (Character.toLowerCase(elementType)) {

src/test/java/htsjdk/samtools/SAMRecordUnitTest.java

pshapiro4broad · 2018-10-11T15:24:02Z

src/test/java/htsjdk/samtools/SAMRecordUnitTest.java

+                .setCreateMd5File(false)
+                .setCreateIndex(false);
+        final Path reference = IOUtil.getPath("src/test/resources/htsjdk/samtools/one-contig.fasta");
+        try(final SAMFileWriter samFileWriter = writerFactory.makeWriter(samRecords.getHeader(), false, tmp, reference)) {


missing space after try here and on line 1237 below

src/test/java/htsjdk/samtools/SAMRecordUnitTest.java

pshapiro4broad · 2018-10-11T15:34:06Z

src/test/java/htsjdk/samtools/SAMRecordUnitTest.java

+
+    @DataProvider
+    public Object[][] getEmptyArrays(){
+        final String BAM = BamFileIoUtils.BAM_FILE_EXTENSION;


I think this can be replaced with streaming operations, e.g.

public Iterator<Object[]> getEmptyArrays() { return Stream.of(BamFileIoUtils.BAM_FILE_EXTENSION, IOUtil.SAM_FILE_EXTENSION, CramIO.CRAM_FILE_EXTENSION) .flatMap(ext -> Stream.of(new int[0], new short[0], new byte[0], new float[0]) .map(array -> new Object[]{array, array.getClass(), ext})).iterator(); }

personally, I do not find that to be more clear...less line? yes...but not more clear.

I agree with the sentiment but not this particular stream operation. Instead I've added a new cartesianProduct method which allows you to combine dataproviders in a nice way.

yfarjoun · 2018-10-15T18:13:18Z

src/main/java/htsjdk/samtools/TextTagCodec.java

+        if(length == 0){
+            return "";
+        }
+        final StringBuilder ret = new StringBuilder(Array.get(value, 0).toString());


should be tested with non-zero length.

lbergelson · 2018-10-15T18:16:17Z

src/main/java/htsjdk/samtools/TextTagCodec.java

+            case 'c': { return new byte[0]; }
+            case 's': { return new short[0]; }
+            case 'i': { return new int[0]; }
+            case 'f': { return new float[0]; }


what to do with F

do not accept F

not accepting F

yfarjoun · 2018-10-15T18:23:24Z

src/main/java/htsjdk/samtools/TextTagCodec.java

+
+        final int numberOfTokens = StringUtil.splitConcatenateExcessTokens(stringVal, elementTypeAndValue, ',');
+        if (!(numberOfTokens == 1 || numberOfTokens == 2)) {
+            throw new SAMFormatException("Tag of type B requires an element type");


is there a test-case that will fall into this hole?

I couldn't come up with one... removed this...

yfarjoun · 2018-10-15T18:24:02Z

src/test/java/htsjdk/samtools/SAMRecordUnitTest.java

+        Assert.assertEquals(record.getAttribute(arrayTag), new char[0]);
+        record.setAttribute(arrayTag, null);
+        Assert.assertNull(record.getStringAttribute(arrayTag));
+


remove excess nls

src/test/java/htsjdk/samtools/SAMRecordUnitTest.java

yfarjoun · 2018-10-15T18:30:27Z

src/test/java/htsjdk/samtools/SAMRecordUnitTest.java

+        Assert.assertEquals(attribute.getClass(), expectedClass);
+        Assert.assertEquals(Array.getLength(attribute), 0);
+    }
+


yfarjoun

thanks for doing this.

src/main/java/htsjdk/samtools/TextTagCodec.java

yfarjoun

I agree that the suggestions are a nice addition.

…pec.

* adding explicit test dependency on a recent version of guava, we already have a transitive test dependency on an older version so this shouldn't be a big deal (unless of course it breaks something unexpected) * adding new method for combining dataproviders to TestNGUtils

lbergelson · 2018-10-17T21:35:04Z

@yfarjoun Could you take a further quick look when you get a chance, I made some additional changes to things including changing our test dependencies slightly and added additional tests.

The way that arrays and particularly unsigned arrays are retrofitted into the tag encoder is really gross and I want to replace it.

src/main/java/htsjdk/samtools/TextTagCodec.java

pshapiro4broad · 2018-10-18T18:05:39Z

src/test/java/htsjdk/utils/TestNGUtilsTest.java

+    }
+
+    @Test(dataProvider = "getArraysAndLists")
+    public void testObjectsToLists(Object[][] objects, List<List<Object>> lists){


Many cases here where the code is missing a space before a {, and missing a space after a comma above (e.g. line 36). It might be easier to reformat the file completely in the IDE.

done... I'm not sure it helps much though, this is a gross block of cases

pshapiro4broad · 2018-10-18T18:08:59Z

src/test/java/htsjdk/utils/TestNGUtils.java

+    /**
+     * @return the cartesian product of two or more DataProviders than can be used as a new dataprovider
+     */
+    public static Object[][] cartesianProduct(Object[][] ... dataProviders){


It is possible to write this and preserve types (e.g. avoid SuppressWarnings). I'm pretty sure it can also be written using generics (vs Object) but didn't have a chance to finish that.

public static Object[][] cartesianProduct(Object[][]... dataProviders) { List<List<List<Object>>> lists = Arrays.stream(dataProviders) .map(Main::nestedArraysToNestedLists) .collect(Collectors.toList()); final List<List<List<Object>>> product = Lists.cartesianProduct(lists); final List<List<Object>> mergeProduct = product.stream() .map(list -> { List<Object> result = new ArrayList<>(); list.forEach(result::addAll); return result; }).collect(Collectors.toList()); return nestedListsToNestedArrays(mergeProduct); } /** * @param dataProvider a nested Object array * @return an equivalent nested List */ public static<T> List<List<T>> nestedArraysToNestedLists(T[][] dataProvider) { return Arrays.stream(dataProvider) .map(Arrays::asList) .collect(Collectors.toList()); } /** * @param lists a nested List * @return an equivalent nested array */ public static Object[][] nestedListsToNestedArrays(List<List<Object>> lists) { return lists.stream().map(List::toArray).toArray(Object[][]::new); }

Also I think stream().map(... list.forEach().. return) is really flatMap. something like

final List<List<Object>> mergeProduct = product.stream().flatMap(list -> list.stream()).collect(Collectors.toList());

👍 to the first comment, thats better.

I don't think this can be done with generics in any sane way, since the It's an Object[][] of totally unrelated types. There's no shared type other than Object. Also, it's what dataproviders expect.

That flatMap flattens the wrong level of list, this works but I'm not sure it's any clearer

final List<List<List<Object>>> product = Lists.cartesianProduct(lists); final List<List<Object>> mergeProduct = product.stream() .map( l -> l.stream() .flatMap(Collection::stream) .collect(Collectors.toList())) .collect(Collectors.toList());

Yeah, not sure much can be done here to make the code clearer. Regarding generics you can definitely make cartesianProduct generic. I ran into trouble with nestedListsToNestedArrays though. But as you say, the caller is always passing Object so not much is gained there.

lbergelson · 2018-10-22T15:41:29Z

@yfarjoun Did you have any final comments on this one? I'd like to get it in soon.

yfarjoun · 2018-10-22T18:06:47Z

src/main/java/htsjdk/samtools/TextTagCodec.java

        final int length = Array.getLength(value);
-        for (int i = 1; i < length; ++i) {
+        final StringBuilder ret = new StringBuilder();
+        for (int i = 0; i < length; ++i) {
            ret.append(',');
            ret.append(Array.get(value, i).toString());
        }
        return ret.toString();



yfarjoun · 2018-10-22T18:07:48Z

src/main/java/htsjdk/samtools/TextTagCodec.java

-        if (StringUtil.splitConcatenateExcessTokens(stringVal, elementTypeAndValue, ',') != 2) {
-            throw new SAMFormatException("Tag of type B should have an element type followed by comma");
-        }
+


yfarjoun · 2018-10-22T18:13:36Z

src/main/java/htsjdk/samtools/TextTagCodec.java

        final char elementType = elementTypeAndValue[0].charAt(0);
+        if (numberOfTokens == 1) {


can this be removed and fall through with length==0?

yfarjoun · 2018-10-22T18:19:38Z

src/test/java/htsjdk/samtools/SAMTextWriterTest.java

+    public void testEmptyArrayAttributeHasNoCommaWhenWrittenToSAM(){
+        final SAMFileHeader header = new SAMFileHeader();
+        final SAMRecord record = new SAMRecord(header);
+        record.setAttribute("xa", new int[0]);


yfarjoun · 2018-10-22T18:20:31Z

src/test/java/htsjdk/samtools/TextTagCodecTest.java

+                ? textTagCodec.encode(tagName, array)
+                : textTagCodec.encodeUnsignedArray(tagName, array);
+        Assert.assertEquals(encodedTag, expectedTag);
+


yfarjoun · 2018-10-22T18:21:23Z

src/test/java/htsjdk/tribble/TestUtils.java

@@ -57,4 +64,6 @@ public static Path getTribbleFileInJimfs(String vcf, String index, FileSystem fi
        }
        return Files.copy(vcfPath, vcfDestination);
    }
+


remove extra NLs

yfarjoun · 2018-10-22T18:27:37Z

src/test/java/htsjdk/utils/TestNGUtilsTest.java

+    @DataProvider
+    public Object[][] getArraysAndLists() {
+        return new Object[][]{
+                {


add empty set,
add actual products of more than 2 lists (i.e. with more than one element each)
add a prodcut with an empty set
add a product with a empty set object.

yfarjoun

thanks for keeping us compliant!

pshapiro4broad · 2018-10-23T13:24:38Z

src/test/java/htsjdk/utils/TestNGUtils.java

+        List<List<List<Object>>> lists = Arrays.stream(dataProviders)
+                .map(TestNGUtils::nestedArraysToNestedLists)
+                .collect(Collectors.toList());
+        final List<List<List<Object>>> product = Lists.cartesianProduct(lists);


Ugh, didn't realize this required adding a guava dependency. ~~It seems like there should be a way to do this using jdk 8 streams directly, by using map(...flatMap())~~ I can see how to do this for 2, 3, etc, but not sure how to do it for N.

We're not actually adding a guava dependency. We have a it as a transient test dependency already through jimfs. I'm just making it an explicit dependency and also bumping to the most recent version.

Note that it's still a test dependency.

lbergelson · 2018-10-24T18:20:40Z

@yfarjoun Any further comments? I'd like to get this merged soon.

yfarjoun

looks good. thanks!

src/test/java/htsjdk/utils/TestNGUtils.java

src/test/java/htsjdk/utils/TestNGUtilsTest.java

lbergelson · 2018-10-24T20:49:47Z

@yfarjoun Some day I'll write a commit without a typo... but today is not that day.

jmarshall · 2023-06-10T10:58:38Z

I removed all the checks for 0 lengths that I could find and the test passes for bam/sam/cram but it's possible there's something else

Unfortunately there was (at least) one 0-length check that was missed, in setUnsignedArrayAttribute(). This is the proximate cause of samtools/hts-specs#728 and this GATK forum posting linked from there.

Now addressed by PR #1669.

This function was omitted from PR samtools#1194. Zero-length 'B' arrays are valid, so remove this check too and add a test case.

* Remove setUnsignedArrayAttribute zero-length-array check This function was omitted from PR #1194. Zero-length 'B' arrays are valid, so remove this check too and add a test case. * Remove Slice setAttribute zero-length-array check Slices' optional tags follow the rules for BAM tagged fields, so zero-length 'B' arrays are valid. Slice tags are currently unused, so the question is somewhat moot and there are no test cases. * removed unused setAttribute methods --------- Co-authored-by: John Marshall <[email protected]>

lbergelson requested a review from yfarjoun October 10, 2018 21:59

lbergelson assigned yfarjoun Oct 10, 2018

pshapiro4broad reviewed Oct 11, 2018

View reviewed changes

yfarjoun reviewed Oct 15, 2018

View reviewed changes

lbergelson commented Oct 15, 2018

View reviewed changes

yfarjoun reviewed Oct 15, 2018

View reviewed changes

src/test/java/htsjdk/samtools/SAMRecordUnitTest.java Outdated Show resolved Hide resolved

yfarjoun reviewed Oct 15, 2018

View reviewed changes

yfarjoun requested changes Oct 15, 2018

View reviewed changes

lbergelson added the Waiting for revisions This PR has received comments from reviewers and is waiting for the Author to respond label Oct 15, 2018

This was referenced Oct 15, 2018

Support SAM v1.6 in GATK broadinstitute/gatk#5282

Closed

add SAM 1.6 to supported versions #1186

Open

lbergelson commented Oct 16, 2018

View reviewed changes

src/main/java/htsjdk/samtools/TextTagCodec.java Outdated Show resolved Hide resolved

yfarjoun reviewed Oct 17, 2018

View reviewed changes

lbergelson and others added 9 commits October 17, 2018 14:25

Adding support for 0-length B arrays in SAM files to conform to 1.6 s…

06dd1c1

…pec.

forgot fai

744d7c6

add space

306c309

space

8aabba8

space

14ff9e4

lowercase tag

ef8c7a2

lowercase

133370a

adding more tests and further changes

f7c46fb

lbergelson force-pushed the lb_binary_array_0_len branch from 8147e99 to f7c46fb Compare October 17, 2018 21:31

fix space

ebf496f

pshapiro4broad reviewed Oct 18, 2018

View reviewed changes

pshapiro4broad and others added 4 commits October 18, 2018 14:32

Update src/main/java/htsjdk/samtools/TextTagCodec.java

98ceb53

Update src/main/java/htsjdk/samtools/TextTagCodec.java

5d591c3

Update src/main/java/htsjdk/samtools/TextTagCodec.java

1d41109

more comments

2306e3f

yfarjoun reviewed Oct 22, 2018

View reviewed changes

lbergelson added 2 commits October 22, 2018 14:56

working on comments

fbfdd79

more test

c1e19ec

pshapiro4broad reviewed Oct 23, 2018

View reviewed changes

yfarjoun approved these changes Oct 24, 2018

View reviewed changes

src/test/java/htsjdk/utils/TestNGUtils.java Outdated Show resolved Hide resolved

src/test/java/htsjdk/utils/TestNGUtilsTest.java Outdated Show resolved Hide resolved

lbergelson added 2 commits October 24, 2018 16:48

Update src/test/java/htsjdk/utils/TestNGUtils.java

8bd2258

Update src/test/java/htsjdk/utils/TestNGUtilsTest.java

31cc9ac

lbergelson merged commit 44baddf into master Oct 24, 2018

lbergelson deleted the lb_binary_array_0_len branch October 24, 2018 21:17

jmarshall mentioned this pull request Jun 10, 2023

Can ML Tags be zero length arrays? samtools/hts-specs#728

Closed

jmarshall added a commit to jmarshall/htsjdk that referenced this pull request Jun 13, 2023

Remove setUnsignedArrayAttribute zero-length-array check

1cc8a2c

This function was omitted from PR samtools#1194. Zero-length 'B' arrays are valid, so remove this check too and add a test case.

jmarshall mentioned this pull request Jun 13, 2023

Remove remaining incorrect zero-length-B-array checks and exception throwing #1669

Closed

gileshall mentioned this pull request Aug 3, 2023

Zero len array #1674

Merged

		final char elementType = elementTypeAndValue[0].charAt(0);
		if (numberOfTokens == 1) {

Adding support for 0-length B arrays in SAM files to conform to 1.6 spec #1194

Adding support for 0-length B arrays in SAM files to conform to 1.6 spec #1194

Conversation

lbergelson commented Oct 10, 2018

Checklist

lbergelson commented Oct 10, 2018

codecov-io commented Oct 10, 2018 • edited Loading

Codecov Report

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

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

yfarjoun left a comment

Choose a reason for hiding this comment

yfarjoun left a comment

Choose a reason for hiding this comment

lbergelson commented Oct 17, 2018

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

lbergelson commented Oct 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yfarjoun Oct 22, 2018 • 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

yfarjoun left a comment

Choose a reason for hiding this comment

pshapiro4broad Oct 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lbergelson commented Oct 24, 2018

yfarjoun left a comment

Choose a reason for hiding this comment

lbergelson commented Oct 24, 2018

jmarshall commented Jun 10, 2023 • edited Loading

codecov-io commented Oct 10, 2018 •

edited

Loading

yfarjoun Oct 22, 2018 •

edited

Loading

pshapiro4broad Oct 23, 2018 •

edited

Loading

jmarshall commented Jun 10, 2023 •

edited

Loading