-
Notifications
You must be signed in to change notification settings - Fork 96
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Go: add support for custom compression #968
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
package mcap | ||
|
||
import "io" | ||
|
||
// ResettableWriteCloser implements io.WriteCloser and adds a Reset method. | ||
type ResettableWriteCloser interface { | ||
io.WriteCloser | ||
Reset(io.Writer) | ||
} | ||
|
||
// ResettableReadCloser implements io.ReadCloser and adds a Reset method. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. io.Reader |
||
type ResettableReader interface { | ||
io.Reader | ||
Reset(io.Reader) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -788,6 +788,10 @@ type WriterOptions struct { | |
// SkipMagic causes the writer to skip writing magic bytes at the start of | ||
// the file. This may be useful for writing a partial section of records. | ||
SkipMagic bool | ||
|
||
// Compressor is a custom compressor. If supplied it will take precedence | ||
// over the built-in ones. | ||
Compressor ResettableWriteCloser | ||
} | ||
|
||
// Convert an MCAP compression level to the corresponding lz4.CompressionLevel. | ||
|
@@ -833,20 +837,26 @@ func NewWriter(w io.Writer, opts *WriterOptions) (*Writer, error) { | |
compressed := bytes.Buffer{} | ||
var compressedWriter *countingCRCWriter | ||
if opts.Chunked { | ||
switch opts.Compression { | ||
case CompressionZSTD: | ||
switch { | ||
case opts.Compressor != nil: // must be top | ||
if opts.Compression == "" { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My first reaction is that the interface feels a little strange with 'compressor' and 'compression' decoupled. I don't see where There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if we used a functional options patter for this it would be better - we could tell the user to pass There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. but we can't make that change here unfortunately |
||
return nil, fmt.Errorf("custom compressor requires compression format") | ||
} | ||
opts.Compressor.Reset(&compressed) | ||
compressedWriter = newCountingCRCWriter(opts.Compressor, opts.IncludeCRC) | ||
case opts.Compression == CompressionZSTD: | ||
level := encoderLevelFromZstd(opts.CompressionLevel) | ||
zw, err := zstd.NewWriter(&compressed, zstd.WithEncoderLevel(level)) | ||
if err != nil { | ||
return nil, err | ||
} | ||
compressedWriter = newCountingCRCWriter(zw, opts.IncludeCRC) | ||
case CompressionLZ4: | ||
case opts.Compression == CompressionLZ4: | ||
level := encoderLevelFromLZ4(opts.CompressionLevel) | ||
lzw := lz4.NewWriter(&compressed) | ||
_ = lzw.Apply(lz4.CompressionLevelOption(level)) | ||
compressedWriter = newCountingCRCWriter(lzw, opts.IncludeCRC) | ||
case CompressionNone: | ||
case opts.Compression == CompressionNone: | ||
compressedWriter = newCountingCRCWriter(bufCloser{&compressed}, opts.IncludeCRC) | ||
default: | ||
return nil, fmt.Errorf("unsupported compression") | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perhaps Reset should return error? lz4's does not, but zstd's does.