Feat: Add zstd support #9
Conversation
src/io/mod.rs
Outdated
| pub fn new_writer<P>(&self, p: &P) -> Result<BufWriter<Box<dyn Write + Send>>> | ||
| where | ||
| P: AsRef<Path>, | ||
| { | ||
| let file = File::create(p).map_err(FgError::IoError)?; | ||
| let write: Box<dyn Write + Send> = if Io::is_gzip_path(p) { | ||
| Box::new(GzEncoder::new(file, self.compression)) | ||
| } else if Io::is_zstd_path(p) { | ||
| Box::new(ZstdEncoder::new(file, 0).unwrap().auto_finish()) |
There was a problem hiding this comment.
Same comment here about the unwrap().
There was a problem hiding this comment.
I don't know enough about zstd compression, but I'm not sure just specifying 0 here (which appears to then use the default, level 3, compression) is the right thing to do, as it gives the user zero control.
On the other hand, re-using the compression passed in above is problematic as gzip and zstd have very different compression level ranges (0-9 vs. -17 to 22), and introducing a new parameter to Io::new() for zstd_level would be a breaking change.
Thoughts @nh13?
There was a problem hiding this comment.
How about something like:
Not sure if it's the proper way to go about it but it's one possibility if it's preferred that the Io struct absolutely doesn't change.
There was a problem hiding this comment.
I could see having a member per level, but then what about bzip2 and future compression levels? And perhaps we have compression that requires not just a level? And certainly we don't want to add translation layer that goes from 0-10 levels for each compression type.
What about instead of storing compression as a member of Io, we have a Map/Vec that stores a compression-specific struct (can impl a trait). You could even make is_gzip_path specific to the compression as a trait-level method (is_path_for), so a new compression need only impl the base trait to be added. Then we avoid the if/else if/else chain there and it can just be a loop (with a default if it exits the loop)? Does that make sense?
There was a problem hiding this comment.
Makes sense. The Io struct probably will have to change at some point if further compression schemes are to be supported as far as I can see. #10 might also be a good solution to this. Haven't checked it out thoroughly yet, but it does feel like good idea to actually check the file format instead of just the extension, although might still want to have a way to compare the found format vs. extension to detect any inconsistencies.
An attempt at adding zstd support in a similar fashion to how gzip is handled.
Once (if) approved, would be used for: fulcrumgenomics/fqgrep#20 in a subsequent PR to the relevant repo.