Go: add support for custom compression #968
Conversation
wkalt
force-pushed
the
ticket/fg-4938/byo-compression
branch
from
ef5399b
to
af98e5a
Compare
go/mcap/Makefile
Outdated
| @@ -1,5 +1,5 @@ | |||
| test: | |||
| go test ./... | |||
| go test -cover ./... | |||
wkalt
force-pushed
the
ticket/fg-4938/byo-compression
branch
from
98710a6
to
cb07c24
Compare
Adds support for bringing custom compressors and decompressors to the go lexer and writer. For the writer, a ResettableWriteCloser is accepted. For the reader, a map of compression format to ResettableReader is accepted. If the reader implements io.Closer we'll call that on file close.
wkalt
force-pushed
the
ticket/fg-4938/byo-compression
branch
from
cb07c24
to
f0597db
Compare
wkalt
force-pushed
the
ticket/fg-4938/byo-compression
branch
from
d0e63c1
to
9d68a25
Compare
| Reset(io.Writer) | ||
| } | ||
|
|
||
| // ResettableReadCloser implements io.ReadCloser and adds a Reset method. |
| // ResettableWriteCloser implements io.WriteCloser and adds a Reset method. | ||
| type ResettableWriteCloser interface { | ||
| io.WriteCloser | ||
| Reset(io.Writer) |
There was a problem hiding this comment.
perhaps Reset should return error? lz4's does not, but zstd's does.
go/mcap/writer.go
Outdated
| case CompressionZSTD: | ||
| switch { | ||
| case opts.Compressor != nil: // must be top | ||
| if opts.Compression == "" { |
There was a problem hiding this comment.
My first reaction is that the interface feels a little strange with 'compressor' and 'compression' decoupled. I don't see where Compression is used in the case of a custom compressor, but presumably an lz4 writer should always be accompanied by type LZ4. Is Compression required if a compressor is passed? If so, would it make sense to have Compressor contain the compression, and embed the ResettableWriteCloser type?
There was a problem hiding this comment.
it's an option, but what I don't really like about it is it forces all consumers of this to implement their own Compressor type (to expose a Compression() string method), whereas the current interface has a chance of matching the compressors out of the box - although I think this will only be true of one of lz4 and zstd, depending on whether or not Reset() is chosen to send an error out.
There was a problem hiding this comment.
if we used a functional options patter for this it would be better - we could tell the user to pass WithCompressor(compression string, compressor ...).
There was a problem hiding this comment.
but we can't make that change here unfortunately
Adds support for bringing custom compressors and decompressors to the go lexer and writer.
For the writer, a ResettableWriteCloser is accepted. For the reader, a map of compression format to ResettableReader is accepted. If the reader implements io.Closer we'll call that on file close.