[GitHub] [arrow] pitrou commented on a change in pull request #11810: ARROW-9648: Added compression level parameter to LZ4_FRAME compression codec.

[GitBox <gi...@apache.org>]

pitrou commented on a change in pull request #11810:
URL: https://github.com/apache/arrow/pull/11810#discussion_r761130558



##########
File path: cpp/src/arrow/util/compression_lz4.cc
##########
@@ -41,6 +41,9 @@ namespace util {
 
 namespace {
 
+constexpr int kLZ4MinCompressionLevel = 1;
+constexpr int kLZ4DefaultCompressionLevel = 1;

Review comment:
       The same constant is already defined in the `.h`, why redefine it here?

##########
File path: ci/docker/linux-apt-jni.dockerfile
##########
@@ -83,5 +82,6 @@ ENV ARROW_BUILD_TESTS=OFF \
     CC=gcc \
     CXX=g++ \
     ORC_SOURCE=BUNDLED \
+    LZ4_SOURCE=BUNDLED \

Review comment:
       Nit: keep those entries in alphabetical order.

##########
File path: cpp/src/arrow/util/compression_internal.h
##########
@@ -65,7 +65,10 @@ std::unique_ptr<Codec> MakeLz4RawCodec();
 std::unique_ptr<Codec> MakeLz4HadoopRawCodec();
 
 // Lz4 frame format codec.
-std::unique_ptr<Codec> MakeLz4FrameCodec();
+constexpr int kLZ4DefaultCompressionLevel = 1;

Review comment:
       Nit: "Lz4" (not capitalized)

##########
File path: cpp/src/arrow/util/compression_lz4.cc
##########
@@ -300,9 +316,14 @@ class Lz4FrameCodec : public Codec {
   }
 
   Compression::type compression_type() const override { return Compression::LZ4_FRAME; }
-  int minimum_compression_level() const override { return kUseDefaultCompressionLevel; }
-  int maximum_compression_level() const override { return kUseDefaultCompressionLevel; }
-  int default_compression_level() const override { return kUseDefaultCompressionLevel; }
+  int minimum_compression_level() const override { return kLZ4MinCompressionLevel; }
+  int maximum_compression_level() const override { return LZ4F_compressionLevel_max(); }
+  int default_compression_level() const override { return kLZ4DefaultCompressionLevel; }
+
+  int compression_level() const override { return compression_level_; }
+
+ private:
+  const int compression_level_;

Review comment:
       Just put this in the `protected` section below?




-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org