wrap both path and index #785

Merged
merged 4 commits into from Jan 20, 2017
@@ -132,7 +132,13 @@ public SamInputResource index(final File file) {
/** Updates the index to point at the provided resource, then returns itself. */
public SamInputResource index(final Path path) {
- this.index = new PathInputResource(path, Function.identity());
+ this.index = new PathInputResource(path);
+ return this;
+ }
+
+ /** Updates the index to point at the provided resource, with the provided wrapper, then returns itself. */
+ public SamInputResource index(final Path path, Function<SeekableByteChannel, SeekableByteChannel> wrapper) {
+ this.index = new PathInputResource(path, wrapper);
return this;
}
@@ -81,10 +81,28 @@
abstract public SamReader open(final File file);
+ /**
+ * Open the specified path (without using any wrappers).
+ *
+ * @param path the SAM or BAM file to open.
+ */
public SamReader open(final Path path) {
- final SamInputResource r = SamInputResource.of(path, getPathWrapper());
+ return open(path, null, null);
+ }
+
+ /**
+ * Open the specified path, using the specified wrappers for prefetching/caching.
+ *
+ * @param path the SAM or BAM file to open
+ * @param dataWrapper the wrapper for the data (or null for none)
+ * @param indexWrapper the wrapper for the index (or null for none)
+ */
+ public SamReader open(final Path path,
+ Function<SeekableByteChannel, SeekableByteChannel> dataWrapper,
+ Function<SeekableByteChannel, SeekableByteChannel> indexWrapper) {
+ final SamInputResource r = SamInputResource.of(path, dataWrapper);
final Path indexMaybe = SamFiles.findIndex(path);
- if (indexMaybe != null) r.index(indexMaybe);
+ if (indexMaybe != null) r.index(indexMaybe, indexWrapper);
return open(r);
}
@droazen

droazen Jan 13, 2017 edited

Contributor

Is it possible to add a simple unit test for this method that verifies that the provided wrappers are applied (perhaps using a dummy wrapper of some kind)?

@jean-philippe-martin

jean-philippe-martin Jan 19, 2017

Contributor

One way would be to copy the xor wrapper from GATK and check that we can read an XOR'd index.

@jean-philippe-martin

jean-philippe-martin Jan 19, 2017

Contributor

Actually that wouldn't work because the factory only uses the wrappers for things that are not files. Instead I'm doing something simpler and directly calling "asUnbufferedSeekableStream" and checking that the wrapper was accessed.

@@ -106,25 +124,6 @@ public SamReader open(final Path path) {
/** Sets a specific Option to a boolean value. * */
abstract public SamReaderFactory setOption(final Option option, boolean value);
- /** Sets a wrapper to modify the SeekableByteChannel from an opened Path, e.g. to add
- * buffering or prefetching. This only works on Path inputs since we need a SeekableByteChannel.
- *
- * @param wrapper how to modify the SeekableByteChannel (Function.identity to unset)
- * @return this
- */
- public SamReaderFactory setPathWrapper(Function<SeekableByteChannel, SeekableByteChannel> wrapper) {
- this.pathWrapper = wrapper;
- return this;
- }
-
- /** Gets the wrapper previously set via setPathWrapper.
- *
- * @return the wrapper.
- */
- public Function<SeekableByteChannel, SeekableByteChannel> getPathWrapper() {
- return pathWrapper;
- }
-
/** Sets the specified reference sequence * */
abstract public SamReaderFactory referenceSequence(File referenceSequence);
@@ -178,15 +177,10 @@ public static SamReaderFactory make() {
private CRAMReferenceSource referenceSource;
private SamReaderFactoryImpl(final EnumSet<Option> enabledOptions, final ValidationStringency validationStringency, final SAMRecordFactory samRecordFactory) {
- this(enabledOptions, validationStringency, samRecordFactory, Function.identity());
- }
-
- private SamReaderFactoryImpl(final EnumSet<Option> enabledOptions, final ValidationStringency validationStringency, final SAMRecordFactory samRecordFactory, final Function<SeekableByteChannel, SeekableByteChannel> wrapper) {
this.enabledOptions = EnumSet.copyOf(enabledOptions);
this.samRecordFactory = samRecordFactory;
this.validationStringency = validationStringency;
this.customReaderFactory = CustomReaderFactory.getInstance();
- setPathWrapper(wrapper);
}
@Override
@@ -417,7 +411,7 @@ private boolean isSra(final File sourceFile) {
}
public static SamReaderFactory copyOf(final SamReaderFactoryImpl target) {
- return new SamReaderFactoryImpl(target.enabledOptions, target.validationStringency, target.samRecordFactory, target.getPathWrapper());
+ return new SamReaderFactoryImpl(target.enabledOptions, target.validationStringency, target.samRecordFactory);
}
}
@@ -0,0 +1,46 @@
+package htsjdk.samtools;
+
+import java.nio.channels.SeekableByteChannel;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.HashMap;
+import java.util.function.Function;
+import org.testng.Assert;
+import org.testng.annotations.Test;
+
+public class PathInputResourceTest {
+ final String localBam = "src/test/resources/htsjdk/samtools/BAMFileIndexTest/index_test.bam";
+
+ @Test
+ public void testWrappersAreAccessed() throws Exception {
+ Path path = Paths.get(localBam);
+ Path indexPath = Paths.get(localBam + ".bai");
+ HashMap<String, Boolean> fired = new HashMap<>();
+ Function<SeekableByteChannel, SeekableByteChannel> wrapData = (SeekableByteChannel in) -> {
+ fired.put("data", true);
+ return in;
+ };
+ Function<SeekableByteChannel, SeekableByteChannel> wrapIndex = (SeekableByteChannel in) -> {
+ fired.put("index", true);
+ return in;
+ };
+ SamInputResource in = SamInputResource.of(path, wrapData);
+ in.index(indexPath, wrapIndex);
+ InputResource indexResource = in.indexMaybe();
+ Assert.assertNotNull(indexResource);
+
+ Assert.assertFalse(fired.containsKey("data"));
+ Assert.assertFalse(fired.containsKey("index"));
+
+ indexResource.asUnbufferedSeekableStream();
+
+ Assert.assertFalse(fired.containsKey("data"));
+ Assert.assertTrue(fired.containsKey("index"));
+
+ in.data().asUnbufferedSeekableStream();
+
+ Assert.assertTrue(fired.containsKey("data"));
+ Assert.assertTrue(fired.containsKey("index"));
+ }
+
+}
@@ -6,6 +6,7 @@
import htsjdk.samtools.seekablestream.SeekableHTTPStream;
import htsjdk.samtools.seekablestream.SeekableStreamFactory;
import htsjdk.samtools.util.*;
+import java.net.URI;
import java.nio.ByteBuffer;
import java.nio.channels.SeekableByteChannel;
import java.nio.file.Paths;
@@ -92,8 +93,7 @@ public void testWrap() throws IOException {
final SamReader wrappedReader =
SamReaderFactory
.makeDefault()
- .setPathWrapper(SamReaderFactoryTest::addHeader)
- .open(input);
+ .open(input, SamReaderFactoryTest::addHeader, null);
int records = countRecords(wrappedReader);
Assert.assertEquals(10, records);
}
@@ -289,7 +289,6 @@ public void openPath() throws IOException {
}
}
-
final Set<List<SAMRecord>> observedRecordOrdering1 = new HashSet<List<SAMRecord>>();
final Set<List<SAMRecord>> observedRecordOrdering3 = new HashSet<List<SAMRecord>>();
final Set<List<SAMRecord>> observedRecordOrdering20 = new HashSet<List<SAMRecord>>();
@@ -23,7 +23,6 @@
*/
package htsjdk.samtools;
-import htsjdk.samtools.cram.CRAMException;
import htsjdk.samtools.util.CloseableIterator;
import htsjdk.samtools.util.CloserUtil;
import org.testng.Assert;