-
Notifications
You must be signed in to change notification settings - Fork 8
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
add support for ZStandard & gzip compression, use structure.Compression field #256
Conversation
starting to think about *actually* supporting compressed data, starting with zstandard
add support for reading & writing compressed CBOR,CSV,JSON data via compression package
extract data & compression formats from a filename string by examining file extensions. Assumes that when multiple exentions are present they come in the order: filename.[data_format].[compression_format] BREAKING CHANGE: detect.ExtensionDataFormat -> dectect.FormatFromFilename, now returns 3 args: dataFormat, compressionFormat, error
compression/compression_test.go
Outdated
} | ||
|
||
if result.String() != plainText { | ||
t.Errorf("compression roun trip result mismatch.\nwant: %s\ngot: %s", plainText, result.String()) |
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.
roun -> round
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.
Aside from Kasey's comments, I don't have much to add code wise and generally I'd give it a thumbs up. I Like the maybeWrap(De)Compressor layer to smooth things out.
However the thing I'm slightly concerned is the work this implies further up the stack. While this should not break anything with current usage, I wonder about the product implications and expectations. Specifically the /get
endpoints. Not a 100% sure if they will pick up the correct extension here and even if so, what are the expectations, return the orignal binary, or further, what if it's a compressed file and they request a regular body. Things balloon further with larger body sizes.
Anyways, I think we should make a decision if this should be reserved for hard mode
users or if it should bubble up.
👍
great question. To date we've designed the Qri binary always interprets the data being requested, even if no format conversion is involved (by interpret I mean create a We use a pattern a bunch up in the main qri codebase where we create a new |
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.
Thx for the explainer on that one. I guess I'm good with the PR once those nitpicks are addressed.
ok, ran the qri test suite atop this branch just to be sure, everything looks good so far! |
compression
package to define supported formats, Reader & Writer constructorszstd
&gzip
formatsdsio
Readers & Writers to wrap with Compression / DecompressionBodyFilename
method todataset.Structure
that interprets things likeStructure{ Format: "csv", Compression: "gzip" }
->body.csv.gzip
detect
package to pick up on multi-extension compression formats, converting filenames likebody.csv.gzip
->Structure{ Format: "csv", Compression: "gzip" }