Je patch g zipped index stream bug - #1079
Conversation
| // Gzipped -- we need to buffer the GZIPInputStream methods as this class makes read() calls, | ||
| // and seekableStream does not support single byte reads | ||
| final InputStream is = new GZIPInputStream(new BufferedInputStream(inputStream, 512000)); | ||
| final InputStream is = new BlockCompressedInputStream(new BufferedInputStream(inputStream, 512000)); |
There was a problem hiding this comment.
But tribble can index gzip files too, not only bgzip. This limits the usage of tribble to only bgzip...
There was a problem hiding this comment.
@magicDGS What's the use of tribble indexing a gvcf that isn't block compressed? You'll have to decompress the entire file up to the point you want to access in order to use the index. Doesn't that defeat the purpose of having the index?
There was a problem hiding this comment.
For example, a file compressed with gzip (for whatever reason) and used by GATK, which requires index for every file...
There was a problem hiding this comment.
The every file thing is going to go away as soon as we rev htsjdk. It was just to work around the nasty bug that you provided the fix for. I think if people are compressing files with gzip that should be bgzipped they're probably doing it accidentally and would like to be aware of it, because it will cause terrible performance problems.
There was a problem hiding this comment.
I had experiences of colleagues sharing with me BED files ending in .gz that I tried to tabix-index and it turns out to be gzip; when I asked them, they even didn't know about the existance of bgzip/tabix. In addition, htsjdk is using the .gz extension for both bgzip and gzip compression (e.g., IOUtils the creation of a stream to write from a file ending in .gz is in gzip, with VCFs it is block-compressed). This inconsistency might cause problems if someone writing with htsjdk a .gz file is also trying to read it with tribble.
The TribbleIndexedFeature is not only for indexed files; thus, a non-indexed gzip file can be a proper input for reading features, and this PR removes that support. I think that to maintain compatibility with gzip/bgzip files, the way to go is to detect if the file/stream is really block-compressed. If it has the bgzip signature, then this code will wrap the stream in a BlockCompressedInputStream; otherwise, it should be a GZIPInputStream. If the problem is that with an index the positions are screw (and performance reduced), then if it is indexed it should be forced to be a block-compressed (I can live with it). Does this makes sense to you?
Finally, if I remember correctly, the GATK used to require an index always if intervals where requested; I am not sure (I haven't look at the FeatureSource for the longest time in GATK4) if this is still a requirement even.
There was a problem hiding this comment.
This was a mistake, (I had not intended to include this line change). Though i don't agree entirely with your reasoning as to why it was a mistake. The big issue I have with using a BlockCompressedInputStream is that it doesn't handle large unblocked GZIP files and will crash with unhelpful error messages. At this stage either input stream should be interchangeable as there is no demand for an index to iterate over the entire file. Either way I have added another layer of protection here and a test that asserts there isn't a crash over a large (unblocked) GZIP vcf file.
| * number of bytes remaining in the stream otherwise. | ||
| */ | ||
| @Override | ||
| public int available() throws IOException { |
There was a problem hiding this comment.
Sorry, I think that I submitted the review from the phone without reviewing. What I meant is that this was already implemented in the master branch (I guess that at that time it was a PR)
| return 0; | ||
| } | ||
| final long remaining = length() - position(); | ||
| // the remaining might be negative if the length is not available (0) |
|
closing this in favor of #1101 |
|
we wanted to review this but couldn't do the rebase on this branch |
4 participants
Description
Added a check and a helpful error message for the case where a user requests an AbstractFeatureReader over a non-BGZF file but has a tabix index. Also added some tests for the FeatureReader codepath/
Checklist