spebern commented on code in PR #3847:
URL: https://github.com/apache/arrow-rs/pull/3847#discussion_r1133301171
##########
parquet/src/basic.rs:
##########
@@ -1783,11 +1784,20 @@ mod tests {
fn test_display_compression() {
assert_eq!(Compression::UNCOMPRESSED.to_string(), "UNCOMPRESSED");
assert_eq!(Compression::SNAPPY.to_string(), "SNAPPY");
- assert_eq!(Compression::GZIP.to_string(), "GZIP");
+ assert_eq!(
Review Comment:
I am not so sure about whether the level should get displayed.
##########
parquet/src/basic.rs:
##########
@@ -286,11 +287,11 @@ pub enum Encoding {
pub enum Compression {
Review Comment:
This is a breaking change and is set in
https://github.com/apache/arrow-rs/blob/9ce0ebb06550be943febc226f61bf083016d7652/parquet/src/file/properties.rs#L424
One way to make this not breaking could be to add a new enum
`CompressionOptions` that can also be set via the `WriterPropertiesBuilder`.
##########
parquet/src/compression.rs:
##########
@@ -284,7 +343,7 @@ mod brotli_codec {
let mut encoder = brotli::CompressorWriter::new(
output_buf,
BROTLI_DEFAULT_BUFFER_SIZE,
Review Comment:
Maybe it makes sense to not only make the level configurable for brotli, but
also the buffer and window size.
##########
parquet/src/compression.rs:
##########
@@ -121,6 +121,26 @@ impl CodecOptionsBuilder {
}
}
+/// Defines valid compression levels.
Review Comment:
I just want to mention as I have before that this is copied from `parquet2`,
so the credit should go to those authors of course!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
