CRAM: Immutable Block #1231

jmthibault79 · 2018-11-28T21:28:35Z

Description

Refactoring CRAM code for future maintainability

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)

jmthibault79 · 2018-11-28T21:29:13Z

src/main/java/htsjdk/samtools/cram/build/ContainerFactory.java

@@ -169,21 +168,13 @@ private static Slice buildSlice(final List<CramCompressionRecord> records,
        writer.writeCramCompressionRecords(records, slice.alignmentStart);

        bitOutputStream.close();
-        slice.coreBlock = Block.buildNewCore(bitBAOS.toByteArray());
+        slice.coreBlock = Block.buildNewCoreBlock(bitBAOS.toByteArray());


renamed for consistency with other static methods

jmthibault79 · 2018-11-28T21:30:24Z

src/main/java/htsjdk/samtools/cram/io/ExternalCompression.java

+            switch (method) {
+                case RAW:
+                    return compressedContent;
+                case GZIP:


There's an opportunity for refactoring the compression classes here. Leaving alone for this PR.

Yeah, definitely should do that in a subsequent round. Especially since ExternalCompressor already has compress methods for each compression type, and the compress/decompress implementations should live together. Maybe BlockCompressionMethod should just have compress/uncompress methods directly, and this method and ExternalCompressor can go away.

jmthibault79 · 2018-11-28T21:30:53Z

src/main/java/htsjdk/samtools/cram/structure/Block.java

@@ -1,323 +0,0 @@
-/**


Moved to htsjdk.samtools.cram.structure.block.Block

jmthibault79 · 2018-11-28T21:36:12Z

src/main/java/htsjdk/samtools/cram/structure/block/Block.java

+ * limitations under the License.
+ * ****************************************************************************
+ */
+package htsjdk.samtools.cram.structure.block;


moved from package htsjdk.samtools.cram.structure

jmthibault79 · 2018-11-28T21:36:57Z

src/main/java/htsjdk/samtools/cram/structure/block/Block.java

@@ -0,0 +1,234 @@
+/**


Block is now the abstract base class for the 2 concrete implementations RawBlock and CompressibleBlock

jmthibault79 · 2018-11-28T21:41:01Z

src/main/java/htsjdk/samtools/cram/structure/block/Block.java

+     * @param rawContent the uncompressed content of the block
+     * @return a new file header {@link RawBlock} object
+     */
+    public static RawBlock buildNewFileHeaderBlock(final byte[] rawContent) {


alternate implementation: FileHeaderBlock, CoreBlock, etc classes

I was kind of thinking the same thing, but are there any other differences in their behavior (besides the raw/compressed) type of block and the content type) - would there be other useful methods with different implementations ?

If we do keep these, maybe the subclass constructors should be protected so this is basically the block factory and is the only way to create blocks ?

I found a way to make this work well (including the static factory) and I'll be committing in a moment.

src/main/java/htsjdk/samtools/cram/structure/block/Block.java

src/main/java/htsjdk/samtools/cram/structure/block/CompressibleBlock.java

src/main/java/htsjdk/samtools/cram/structure/block/RawBlock.java

codecov-io · 2018-11-28T21:55:27Z

Codecov Report

Merging #1231 into master will increase coverage by 0.055%.
The diff coverage is 76.282%.

@@               Coverage Diff               @@
##              master     #1231       +/-   ##
===============================================
+ Coverage     69.323%   69.378%   +0.055%     
- Complexity      8105      8107        +2     
===============================================
  Files            543       544        +1     
  Lines          32480     32437       -43     
  Branches        5501      5496        -5     
===============================================
- Hits           22516     22504       -12     
+ Misses          7755      7732       -23     
+ Partials        2209      2201        -8

Impacted Files	Coverage Δ	Complexity Δ
...java/htsjdk/samtools/cram/structure/Container.java	`81.25% <ø> (ø)`	`7 <0> (ø)`	⬇️
.../samtools/cram/compression/ExternalCompressor.java	`40% <ø> (ø)`	`4 <0> (ø)`	⬇️
...amtools/cram/encoding/writer/CramRecordWriter.java	`89.063% <ø> (ø)`	`26 <0> (ø)`	⬇️
...va/htsjdk/samtools/cram/structure/ContainerIO.java	`65.385% <100%> (+4.779%)`	`13 <0> (ø)`	⬇️
...amtools/cram/structure/block/BlockContentType.java	`100% <100%> (ø)`	`4 <1> (?)`
...s/cram/structure/block/BlockCompressionMethod.java	`100% <100%> (ø)`	`4 <1> (?)`
...k/samtools/cram/structure/block/ExternalBlock.java	`100% <100%> (ø)`	`2 <2> (?)`
...c/main/java/htsjdk/samtools/cram/build/CramIO.java	`83.721% <100%> (ø)`	`20 <0> (ø)`	⬇️
...ain/java/htsjdk/samtools/cram/structure/Slice.java	`44.545% <100%> (ø)`	`20 <2> (ø)`	⬇️
...samtools/cram/compression/ExternalCompression.java	`77.273% <33.333%> (-11.299%)`	`14 <3> (+3)`
... and 9 more

jmthibault79 · 2018-11-28T22:22:11Z

src/main/java/htsjdk/samtools/cram/structure/block/CompressibleBlock.java

+                      final byte[] compressedContent) {
+        super(type);
+
+        if (type == BlockContentType.EXTERNAL && contentId == Block.NO_CONTENT_ID) {


This check is causing many test failures - I'm investigating.

Removed this check and opened #1232

cmnbroad

A few stream of consciousness questions and comments since - see what you think.

cmnbroad · 2018-11-30T20:18:00Z

src/main/java/htsjdk/samtools/cram/io/ExternalCompression.java

+            switch (method) {
+                case RAW:
+                    return compressedContent;
+                case GZIP:


Yeah, definitely should do that in a subsequent round. Especially since ExternalCompressor already has compress methods for each compression type, and the compress/decompress implementations should live together. Maybe BlockCompressionMethod should just have compress/uncompress methods directly, and this method and ExternalCompressor can go away.

src/main/java/htsjdk/samtools/cram/structure/block/Block.java

cmnbroad · 2018-11-30T20:34:31Z

src/main/java/htsjdk/samtools/cram/structure/block/Block.java

+     * @param rawContent the uncompressed content of the block
+     * @return a new file header {@link RawBlock} object
+     */
+    public static RawBlock buildNewFileHeaderBlock(final byte[] rawContent) {


I was kind of thinking the same thing, but are there any other differences in their behavior (besides the raw/compressed) type of block and the content type) - would there be other useful methods with different implementations ?

cmnbroad · 2018-11-30T20:38:13Z

src/main/java/htsjdk/samtools/cram/structure/block/Block.java

+     * @param rawContent the uncompressed content of the block
+     * @return a new file header {@link RawBlock} object
+     */
+    public static RawBlock buildNewFileHeaderBlock(final byte[] rawContent) {


If we do keep these, maybe the subclass constructors should be protected so this is basically the block factory and is the only way to create blocks ?

src/main/java/htsjdk/samtools/cram/structure/block/Block.java

src/main/java/htsjdk/samtools/cram/structure/block/RawBlock.java

src/main/java/htsjdk/samtools/cram/structure/block/Block.java

src/main/java/htsjdk/samtools/cram/structure/block/CompressibleBlock.java

cmnbroad · 2018-12-03T19:21:56Z

@jmthibault79 Was just looking at this again - is it ready for review (wasn't sure if you were still making changes) ?

jmthibault79 · 2018-12-03T20:05:19Z

@cmnbroad ready for re-review now, after the subclasses commit

cmnbroad · 2018-12-04T20:47:55Z

@jmthibault79 I still feel like we don't have the Block hierarchy factored quite correctly yet. It doesn't seem like any useful polymorphism results from having the class hierarchy based on content type, type casting is still required, and the actual "content type" of a given block is redundantly represented three different ways now (in the class hierarchy itself; in the base class BlockContentType instance field; and in the static BlockContentType constant maintained by each subclass). Since the call sites all seem to know what content type they're reading or writing (i.e., SliceIO knows it wants to extract, say, a Slice header), most of this seems to be about enforcing/validating the compression/content-type combination for each block type.

What do you think about this proposal: go back to just Block (possibly with protected subclass CompressibleBlock since there is some useful polymorphism there wrt/compressed vs. raw content), where Block has factory methods like it does now for creating/validating each of type of block from either raw content or from a stream. The factory methods would enforce or validate the correct combination of "Block" params (compressible, etc.), and possibly substitute Compressible block as appropriate - but the only class type callers would only ever deal with is Block.

Then in subsequent PRs, we can create classes that model things like SliceHeader, with fields and methods specific to it, and constructors for those classes read/write from/to plain old Block. (Edit: or maybe those constructors should take a input stream or raw content, and themselves call the static factories in Block).

src/main/java/htsjdk/samtools/cram/structure/block/Block.java

- see also #1232

jmthibault79 · 2018-12-04T22:57:13Z

I see your point about the hierarchy. Here's another attempt, with ExternalDataBlock as the only subclass, because it has the meaningful distinction of being associated with a Content ID.

cmnbroad

@jmthibault79 I think the hierarchy is a lot better now, but I still have some suggestions for potential changes. Hopefully they make sense - if not let me know what you think since I don't want to hold this up indefinitely.

src/main/java/htsjdk/samtools/cram/build/ContainerFactory.java

src/main/java/htsjdk/samtools/cram/build/CramIO.java

src/main/java/htsjdk/samtools/cram/structure/ContainerIO.java

src/main/java/htsjdk/samtools/cram/build/CramIO.java

src/main/java/htsjdk/samtools/cram/structure/SliceIO.java

src/main/java/htsjdk/samtools/cram/structure/block/Block.java

…ock()

…nMethod/contentType

cmnbroad

Mostly there now, a few more minor comments and this should be good to merge.

src/main/java/htsjdk/samtools/cram/build/ContainerFactory.java

src/main/java/htsjdk/samtools/cram/structure/CompressionHeader.java

src/main/java/htsjdk/samtools/cram/structure/block/Block.java

src/main/java/htsjdk/samtools/cram/structure/block/ExternalBlock.java

src/test/java/htsjdk/samtools/cram/structure/BlockTest.java

cmnbroad

A couple more suggestions, for either this PR or for the next one, since we've beat this one to a pulp. @lbergelson let us know if you also want to review this.

src/test/java/htsjdk/samtools/cram/structure/BlockTest.java

lbergelson

👍

jmthibault79 commented Nov 28, 2018

View reviewed changes

src/main/java/htsjdk/samtools/cram/structure/block/Block.java Outdated Show resolved Hide resolved

jmthibault79 commented Nov 28, 2018

View reviewed changes

src/main/java/htsjdk/samtools/cram/structure/block/CompressibleBlock.java Outdated Show resolved Hide resolved

jmthibault79 commented Nov 28, 2018

View reviewed changes

src/main/java/htsjdk/samtools/cram/structure/block/CompressibleBlock.java Outdated Show resolved Hide resolved

jmthibault79 commented Nov 28, 2018

View reviewed changes

src/main/java/htsjdk/samtools/cram/structure/block/RawBlock.java Outdated Show resolved Hide resolved

jmthibault79 added cram Waiting for Review This PR is waiting for a reviewer to respond labels Nov 28, 2018

jmthibault79 requested review from cmnbroad and lbergelson November 28, 2018 21:56

jmthibault79 commented Nov 28, 2018

View reviewed changes

jmthibault79 force-pushed the jt_Block branch 2 times, most recently from 31ed5a8 to 8095ea2 Compare November 30, 2018 21:45

cmnbroad reviewed Nov 30, 2018

View reviewed changes

cmnbroad reviewed Dec 4, 2018

View reviewed changes

src/main/java/htsjdk/samtools/cram/structure/block/Block.java Outdated Show resolved Hide resolved

jmthibault79 added 7 commits December 4, 2018 17:18

Immutable Block, RawBlock, and CompressibleBlock

4c264fc

updates

f47ec66

public

bbf950c

better content/type check

5760b28

- see also #1232

Content ID comment

531792f

rename Block.readFromInputStream to read() and move down in file

1863f58

parameterized

080e332

jmthibault79 added 3 commits December 4, 2018 17:18

more consistent ExternalDataBlock

7f56657

store uncompressed length and check on demand

3bcc8f3

rm subclasses besides ExternalDataBlock

8f9e6af

jmthibault79 force-pushed the jt_Block branch from f59abd0 to 8f9e6af Compare December 4, 2018 22:53

Change static factory method to constructor

4bdd368

cmnbroad reviewed Dec 5, 2018

View reviewed changes

jmthibault79 added 12 commits December 5, 2018 11:33

Block.createRawFooBlock

eb54128

renaming in buildSlice

b7c814c

rm unused getByteSize

fa383bf

ContainerIO.readAsCompressionHeader() -> CompressionHeader.readFromBl…

a854435

…ock()

CompressionHeader.toBlock()

8d44c9b

Be more explicit in block type check errors

1e5975f

don't need uncompress again

62b1fde

better names for compressionMethod and contentType

740df1a

reorder ExternalDataBlock constructor and catch a few more compressio…

a4aa0f9

…nMethod/contentType

new Block constructor specialized for RAW

ef408d4

change CompressionHeader public interface to read/write

b89eed6

rename to ExternalBlock, and use plain Block in Slice

60a7232

cmnbroad reviewed Dec 7, 2018

View reviewed changes

jmthibault79 added 5 commits December 7, 2018 12:49

better exception handling

a3cf698

this can be a simple OutputStream

a2d13aa

javadocs

3c29139

tests can just throw

ccadb9a

removed redundant ExternalBlock constructor and streamlined tests

bfc5bb0

cmnbroad approved these changes Dec 7, 2018

View reviewed changes

src/test/java/htsjdk/samtools/cram/structure/BlockTest.java Outdated Show resolved Hide resolved

src/test/java/htsjdk/samtools/cram/structure/BlockTest.java Outdated Show resolved Hide resolved

DataProvider smarter

5389b11

lbergelson approved these changes Dec 10, 2018

View reviewed changes

cmnbroad merged commit 1126e5c into master Dec 10, 2018

cmnbroad deleted the jt_Block branch December 10, 2018 19:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CRAM: Immutable Block #1231

CRAM: Immutable Block #1231

jmthibault79 commented Nov 28, 2018 •

edited

Loading

jmthibault79 Nov 28, 2018

jmthibault79 Nov 28, 2018

cmnbroad Nov 30, 2018

jmthibault79 Nov 28, 2018

jmthibault79 Nov 28, 2018

jmthibault79 Nov 28, 2018

jmthibault79 Nov 28, 2018 •

edited

Loading

cmnbroad Nov 30, 2018

cmnbroad Nov 30, 2018

jmthibault79 Dec 3, 2018

codecov-io commented Nov 28, 2018 •

edited

Loading

jmthibault79 Nov 28, 2018

jmthibault79 Nov 29, 2018

cmnbroad left a comment

cmnbroad Nov 30, 2018

cmnbroad Nov 30, 2018

cmnbroad Nov 30, 2018

cmnbroad commented Dec 3, 2018

jmthibault79 commented Dec 3, 2018

cmnbroad commented Dec 4, 2018 •

edited

Loading

jmthibault79 commented Dec 4, 2018

cmnbroad left a comment

cmnbroad left a comment

cmnbroad left a comment

lbergelson left a comment

CRAM: Immutable Block #1231

CRAM: Immutable Block #1231

Conversation

jmthibault79 commented Nov 28, 2018 • edited Loading

Description

Checklist

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

jmthibault79 Nov 28, 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

codecov-io commented Nov 28, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmnbroad 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

cmnbroad commented Dec 3, 2018

jmthibault79 commented Dec 3, 2018

cmnbroad commented Dec 4, 2018 • edited Loading

jmthibault79 commented Dec 4, 2018

cmnbroad left a comment

Choose a reason for hiding this comment

cmnbroad left a comment

Choose a reason for hiding this comment

cmnbroad left a comment

Choose a reason for hiding this comment

lbergelson left a comment

Choose a reason for hiding this comment

jmthibault79 commented Nov 28, 2018 •

edited

Loading

jmthibault79 Nov 28, 2018 •

edited

Loading

codecov-io commented Nov 28, 2018 •

edited

Loading

cmnbroad commented Dec 4, 2018 •

edited

Loading