Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/22718 )
Change subject: IMPALA-13923: Support more compression levels for ZSTD and ZLIB ...................................................................... Patch Set 8: (2 comments) http://gerrit.cloudera.org:8080/#/c/22718/3/be/src/util/compress.h File be/src/util/compress.h: http://gerrit.cloudera.org:8080/#/c/22718/3/be/src/util/compress.h@48 PS3, Line 48: int compression_level = Z_DEFAULT_COMPRESSION); > As the implementations still need a default value, this seems to be the cle I think of codec.h/codec.cc as generic connector code. It handles instantiating the compressor/decompressor, but it doesn't have to know much about how those work internally. The way that we're doing it now, we have to convert the std::optional to an int in codec.cc like this: codec_info.compression_level_.value_or(Z_DEFAULT_COMPRESSION) I think that makes more sense in the GzipCompressor's constructor (and same thing for ZstandardCompressor). It would eliminate the need for codec.cc to know the default value. If we push the level validation into the compressor as well, codec.cc would not need to know anything specific about the compression libraries. The details are encapsulated in the compressor class. http://gerrit.cloudera.org:8080/#/c/22718/3/be/src/util/compress.h@66 PS3, Line 66: int compression_level_; > If we approach this, CodecInfo will only have a single attribute "format", We can leave it on the individual compressors for now. Right now, different compressors are using different field names for the compression level. GzipCompressor is using compression_level_ here. ZstandardCompressor is using clevel_. It would be nice to standardize the name of this field. -- To view, visit http://gerrit.cloudera.org:8080/22718 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5b98c735246f08e04598a4e752c8cca04e31a88a Gerrit-Change-Number: 22718 Gerrit-PatchSet: 8 Gerrit-Owner: Surya Hebbar <[email protected]> Gerrit-Reviewer: Abhishek Rawat <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Surya Hebbar <[email protected]> Gerrit-Comment-Date: Tue, 22 Apr 2025 18:59:57 +0000 Gerrit-HasComments: Yes
