Add support for re-serializing (sharding and reassembling) CRAM containers. #1609
Conversation
cmnbroad
force-pushed
the
cn_rewrite_containers
branch
2 times, most recently
from
cd08c69
to
f0e32c6
Compare
Codecov Report
@@ Coverage Diff @@
## master #1609 +/- ##
===============================================
+ Coverage 69.838% 69.866% +0.028%
- Complexity 9666 9677 +11
===============================================
Files 703 704 +1
Lines 37690 37728 +38
Branches 6121 6124 +3
===============================================
+ Hits 26322 26359 +37
- Misses 8914 8916 +2
+ Partials 2454 2453 -1
|
cmnbroad
force-pushed
the
cn_rewrite_containers
branch
2 times, most recently
from
b1ded6e
to
317bec6
Compare
…iners to a new stream.
cmnbroad
force-pushed
the
cn_rewrite_containers
branch
from
317bec6
to
fd7cb83
Compare
There was a problem hiding this comment.
@cmnbroad Questions / comments about mutability. If this is the only sane way to do this I can accept that it adds some more state.
| private final OutputStream outputStream; | ||
| private final String outputStreamIdentifier; | ||
| private final CramHeader cramHeader; | ||
| private final SAMFileHeader samFileHeader; |
There was a problem hiding this comment.
This field is assigned but never accessed.
| cramIndexer.finish(); | ||
| } | ||
| } catch (final IOException e) { | ||
| throw new RuntimeIOException(String.format("IOException closing stream for %s", outputStreamIdentifier)); |
There was a problem hiding this comment.
Is the output stream actually closed here? It seems like it's just flushed.
| * @param streamByteOffset the new stream byte offset counter for this container | ||
| * @return the updated global record counter | ||
| */ | ||
| public long relocateContainer(final long containerRecordCounter, final long streamByteOffset) { |
There was a problem hiding this comment.
Is it necessary to mutate the container? It was previously effectively immutable wasn't it? It seems like it would be nicer to copy it since I assume the container / slice objects are fairly lightweight and you could just pass through the reference to the heavy data?
Not a blocker if you think this is necessary for efficiency reasons / copying a container being very annoying.
| @@ -249,4 +249,8 @@ public boolean isEOF() { | |||
| return v3 || v2; | |||
| } | |||
|
|
|||
| void setGlobalRecordCounter(final long recordCounter) { | |||
| this.globalRecordCounter = recordCount; | |||
There was a problem hiding this comment.
again, unfortunate to make this mutable.
| @@ -190,6 +190,7 @@ public int write(final CRAMVersion cramVersion, final OutputStream outputStream) | |||
| // landmark 0 = byte length of the compression header | |||
| // landmarks after 0 = byte length of the compression header plus all slices before this one | |||
| landmarks.add(tempOutputStream.size()); | |||
| slice.byteOffsetOfContainer = containerByteOffset; | |||
There was a problem hiding this comment.
Why is this updated again here when it's already set by relocate?
| @@ -78,7 +78,7 @@ public class Slice { | |||
|
|
|||
| private final CompressionHeader compressionHeader; | |||
| private final SliceBlocks sliceBlocks; | |||
| private final long byteOffsetOfContainer; | |||
| public long byteOffsetOfContainer; | |||
There was a problem hiding this comment.
Why make this public? It seems like it's already updated through relocate.
Fixes #1606.