-
-
Notifications
You must be signed in to change notification settings - Fork 390
sz: Support S2 (close #429) #431
Conversation
@mholt "next version" - meaning the replacement will :) |
Wow! So, do you think I should wait for that? (Before I make new exported API) |
if sz.S2.MaxBlockSize != 0 { | ||
opts = append(opts, s2.ReaderMaxBlockSize(sz.S2.MaxBlockSize)) | ||
} | ||
return io.NopCloser(s2.NewReader(r, opts...)), nil |
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.
A bit sad the API is truncated to an io.ReadCloser
, but I guess the users can flip back to the org package for more extensive API.
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.
Hmm, yeah, I guess this makes type asserting difficult doesn't it. Sorry :(
I went through all the formats just now to see if the Closer API was really necessary but it does seem a few formats use it, so I think we need it to stay in order to keep the API advantages.
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.
Thank you for picking it so quickly !
The only suggestion is to change file extension .s2
to .sz2
or .s2z
to be on pair with other and to reflect relation to Snappy
And bellow is result of testing a small (140Mb)
|
Co-authored-by: Klaus Post <[email protected]>
@CompuRoot I am not religious about it, but I have used The snappy relation is not too important, except for backwards compatibility of reading snappy data. |
You right, if there already exists CLI tool with this extension, then keeping |
Thanks both for chiming in! Given that S2 is largely compatible with Snappy, I'm keeping it as part of the Snappy format, with the option of writing Snappy-incompatible data if desired. |
@CompuRoot maybe you can take a look at this. I'll likely just merge this and then we can change it later if needed.
@klauspost -- Thank you for yet again, another great package! (One minor request: It could be useful if the compression levels were exported.)