address comments

facebookincubator · Dec 6, 2023 · 183dc16 · 183dc16
1 parent cc42e19
commit 183dc16
Show file tree

Hide file tree

Showing 10 changed files with 304 additions and 333 deletions.
diff --git a/velox/common/compression/Compression.cpp b/velox/common/compression/Compression.cpp
@@ -76,10 +76,6 @@ std::string compressionKindToString(CompressionKind kind) {
       return "lz4";
     case CompressionKind_GZIP:
       return "gzip";
-    case CompressionKind_LZ4RAW:
-      return "lz4_raw";
-    case CompressionKind_LZ4HADOOP:
-      return "lz4_hadoop";
   }
   return folly::to<std::string>("unknown - ", kind);
 }
@@ -93,9 +89,7 @@ CompressionKind stringToCompressionKind(const std::string& kind) {
           {"lzo", CompressionKind_LZO},
           {"zstd", CompressionKind_ZSTD},
           {"lz4", CompressionKind_LZ4},
-          {"gzip", CompressionKind_GZIP},
-          {"lz4_raw", CompressionKind_LZ4RAW},
-          {"lz4_hadoop", CompressionKind_LZ4HADOOP}};
+          {"gzip", CompressionKind_GZIP}};
   auto iter = stringToCompressionKindMap.find(kind);
   if (iter != stringToCompressionKindMap.end()) {
     return iter->second;

diff --git a/velox/common/compression/Compression.h b/velox/common/compression/Compression.h
@@ -29,8 +29,6 @@ enum CompressionKind {
   CompressionKind_ZSTD = 4,
   CompressionKind_LZ4 = 5,
   CompressionKind_GZIP = 6,
-  CompressionKind_LZ4RAW = 7,
-  CompressionKind_LZ4HADOOP = 8,
   CompressionKind_MAX = INT64_MAX
 };
 

diff --git a/velox/common/compression/tests/CompressionTest.cpp b/velox/common/compression/tests/CompressionTest.cpp
@@ -32,8 +32,6 @@ TEST_F(CompressionTest, testCompressionNames) {
   EXPECT_EQ("lz4", compressionKindToString(CompressionKind_LZ4));
   EXPECT_EQ("zstd", compressionKindToString(CompressionKind_ZSTD));
   EXPECT_EQ("gzip", compressionKindToString(CompressionKind_GZIP));
-  EXPECT_EQ("lz4_raw", compressionKindToString(CompressionKind_LZ4RAW));
-  EXPECT_EQ("lz4_hadoop", compressionKindToString(CompressionKind_LZ4HADOOP));
   EXPECT_EQ(
       "unknown - 99",
       compressionKindToString(static_cast<CompressionKind>(99)));
@@ -59,8 +57,6 @@ TEST_F(CompressionTest, stringToCompressionKind) {
   EXPECT_EQ(stringToCompressionKind("lz4"), CompressionKind_LZ4);
   EXPECT_EQ(stringToCompressionKind("zstd"), CompressionKind_ZSTD);
   EXPECT_EQ(stringToCompressionKind("gzip"), CompressionKind_GZIP);
-  EXPECT_EQ(stringToCompressionKind("lz4_raw"), CompressionKind_LZ4RAW);
-  EXPECT_EQ(stringToCompressionKind("lz4_hadoop"), CompressionKind_LZ4HADOOP);
   VELOX_ASSERT_THROW(
       stringToCompressionKind("bz2"), "Not support compression kind bz2");
 }

diff --git a/velox/common/compression/v2/Compression.cpp b/velox/common/compression/v2/Compression.cpp
@@ -49,7 +49,6 @@ bool Codec::supportsGetUncompressedLength(CompressionKind kind) {
 bool Codec::supportsCompressionLevel(CompressionKind kind) {
   switch (kind) {
     case CompressionKind::CompressionKind_LZ4:
-    case CompressionKind::CompressionKind_LZ4RAW:
       return true;
     default:
       return false;
@@ -59,14 +58,19 @@ bool Codec::supportsCompressionLevel(CompressionKind kind) {
 bool Codec::supportsStreamingCompression(CompressionKind kind) {
   switch (kind) {
     case CompressionKind::CompressionKind_LZ4:
-    case CompressionKind::CompressionKind_GZIP:
-    case CompressionKind::CompressionKind_ZLIB:
       return true;
     default:
       return false;
   }
 }
 
+bool Codec::supportsCompressFixedLength(CompressionKind kind) {
+  switch (kind) {
+    default:
+      return false;
+  }
+}
+
 int32_t Codec::maximumCompressionLevel(CompressionKind kind) {
   checkSupportsCompressionLevel(kind);
   auto codec = Codec::create(kind);
@@ -104,20 +108,29 @@ std::unique_ptr<Codec> Codec::create(
   std::unique_ptr<Codec> codec;
   switch (kind) {
     case CompressionKind::CompressionKind_LZ4:
+      if (auto options = dynamic_cast<const Lz4CodecOptions*>(&codecOptions)) {
+        switch (options->type) {
+          case Lz4CodecOptions::kLz4Frame:
+            codec = makeLz4FrameCodec(compressionLevel);
+            break;
+          case Lz4CodecOptions::kLz4Raw:
+            codec = makeLz4RawCodec(compressionLevel);
+            break;
+          case Lz4CodecOptions::kLz4Hadoop:
+            codec = makeLz4HadoopRawCodec(compressionLevel);
+            break;
+        }
+      }
+      // By default, create LZ4 Frame codec.
       codec = makeLz4FrameCodec(compressionLevel);
       break;
-    case CompressionKind::CompressionKind_LZ4RAW:
-      codec = makeLz4RawCodec(compressionLevel);
-      break;
-    case CompressionKind::CompressionKind_LZ4HADOOP:
-      codec = makeLz4HadoopRawCodec();
-      break;
     default:
       break;
   }
 
   if (codec == nullptr) {
-    VELOX_UNSUPPORTED("LZO codec not implemented");
+    VELOX_UNSUPPORTED(
+        "{} codec not implemented", compressionKindToString(kind));
   }
 
   codec->init();
@@ -135,8 +148,6 @@ bool Codec::isAvailable(CompressionKind kind) {
   switch (kind) {
     case CompressionKind::CompressionKind_NONE:
     case CompressionKind::CompressionKind_LZ4:
-    case CompressionKind::CompressionKind_LZ4RAW:
-    case CompressionKind::CompressionKind_LZ4HADOOP:
       return true;
     case CompressionKind::CompressionKind_SNAPPY:
     case CompressionKind::CompressionKind_GZIP:
@@ -150,45 +161,15 @@ bool Codec::isAvailable(CompressionKind kind) {
 
 std::optional<uint64_t> Codec::getUncompressedLength(
     uint64_t inputLength,
-    const uint8_t* input,
-    std::optional<uint64_t> uncompressedLength) const {
-  if (inputLength == 0) {
-    if (uncompressedLength.value_or(0) != 0) {
-      VELOX_USER_CHECK_EQ(
-          uncompressedLength.value_or(0),
-          0,
-          "Invalid uncompressed length: {}.",
-          *uncompressedLength);
-    }
-    return 0;
-  }
-  auto actualLength =
-      doGetUncompressedLength(inputLength, input, uncompressedLength);
-  if (actualLength) {
-    if (uncompressedLength) {
-      VELOX_USER_CHECK_EQ(
-          *actualLength,
-          *uncompressedLength,
-          "Invalid uncompressed length: {}.",
-          *uncompressedLength);
-    }
-    return actualLength;
-  }
-  return uncompressedLength;
+    const uint8_t* input) const {
+  return std::nullopt;
 }
 
-std::optional<uint64_t> Codec::doGetUncompressedLength(
-    uint64_t inputLength,
+uint64_t Codec::compressFixedLength(
     const uint8_t* input,
-    std::optional<uint64_t> uncompressedLength) const {
-  return uncompressedLength;
-}
-
-uint64_t Codec::compressPartial(
     uint64_t inputLength,
-    const uint8_t* input,
-    uint64_t outputLength,
-    uint8_t* output) {
+    uint8_t* output,
+    uint64_t outputLength) {
   VELOX_UNSUPPORTED("'{}' doesn't support partial compression", name());
 }
 } // namespace facebook::velox::common