Add support to compress data in PrestoVectorSerializer #5544

jinchengchenghh · 2023-07-06T07:57:45Z

Compress the data by folly when flush to OutputStream. And decompress the data under deserialization.
Will use in spill compression.

Resolve:#5313

netlify · 2023-07-06T07:57:50Z

✅ Deploy Preview for meta-velox ready!

Name	Link
🔨 Latest commit	`c48af6b`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/64bdd330b522500008b081a5
😎 Deploy Preview	https://deploy-preview-5544--meta-velox.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

xiaoxmeng

Thanks for the change % minors!

xiaoxmeng · 2023-07-06T23:57:50Z

velox/dwio/common/Common.cpp

+    case CompressionKind_LZ4:
+      return getCodec(folly::io::CodecType::LZ4);
+  }
+  VELOX_UNREACHABLE();


Do we want to log out the unsupported kind in default case to help debugging? Thanks!

velox/dwio/common/Common.h

velox/common/memory/ByteStream.cpp

velox/serializers/PrestoSerializer.cpp

xiaoxmeng · 2023-07-07T02:23:08Z

velox/serializers/PrestoSerializer.cpp

  auto children = &(*result)->children();
  auto childTypes = type->as<TypeKind::ROW>().children();
-  readColumns(source, pool, childTypes, children, useLosslessTimestamp);
+  if (codec->type() == folly::io::CodecType::NO_COMPRESSION) {
+    VELOX_CHECK(!isCompressedBitSet(pageCodecMarker));


Shall we move the check above at l1845?

VELOX_CHECK_EQ(codec->type() == folly::io::CodecType::NO_COMPRESSION, !isCompressedBitSet(pageCodecMarker), "error message");

xiaoxmeng · 2023-07-07T02:31:08Z

velox/serializers/PrestoSerializer.cpp

+    for (auto& stream : streams_) {
+      stream->flush(&out);
+    }
+    int32_t uncompressedSize = out.tellp();


velox/serializers/PrestoSerializer.cpp

xiaoxmeng · 2023-07-07T02:37:45Z

velox/serializers/PrestoSerializer.cpp

+      crc = computeChecksum(listener, codec, numRows, compressedSize);
+    }
+    output->seekp(crcOffset);
+    writeInt64(output, crc); // Write zero checksum


The comment is not valid?

velox/serializers/tests/PrestoSerializerTest.cpp

jinchengchenghh · 2023-07-10T06:48:47Z

Fixed all the comments, can you help review again? @xiaoxmeng

xiaoxmeng

Thanks for the update. LGTM. Could you draw a diagram to show the serialized page layout with and without compression in header?

velox/dwio/common/Common.cpp

velox/serializers/tests/PrestoSerializerTest.cpp

xiaoxmeng · 2023-07-12T07:17:11Z

velox/serializers/tests/PrestoSerializerTest.cpp

-TEST_F(PrestoSerializerTest, ioBufRoundTrip) {
-  serializer::presto::PrestoVectorSerde::registerVectorSerde();
+TEST_P(PrestoSerializerTest, ioBufRoundTrip) {
+  if (!isRegisteredVectorSerde()) {


Can you register once in SetUpTestCase? Thanks!

xiaoxmeng · 2023-07-12T07:18:26Z

velox/serializers/tests/PrestoSerializerTest.cpp

@@ -275,27 +297,31 @@ TEST_F(PrestoSerializerTest, multiPage) {
  auto byteStream = toByteStream(bytes);

  RowVectorPtr deserialized;
+  dwio::common::CompressionKind kind = GetParam();


You can make a method in test class: getSerdeOptions() which construct an options based on the test parameter?

xiaoxmeng · 2023-07-12T08:03:02Z

velox/serializers/PrestoSerializer.h

 #include "velox/vector/VectorStream.h"

 namespace facebook::velox::serializer::presto {
 class PrestoVectorSerde : public VectorSerde {
 public:
  // Input options that the serializer recognizes.
  struct PrestoOptions : VectorSerde::Options {
-    explicit PrestoOptions(bool useLosslessTimestamp)
-        : useLosslessTimestamp(useLosslessTimestamp) {}
+    explicit PrestoOptions() {}


PrestoOptions() = default;

Do we need this?

Yes, then we can simplify the function, otherwise, we get nullptr Options most of the case, we should construct the PrestoOptions with its default value.

const PrestoVectorSerde::PrestoOptions toPrestoOptions( const VectorSerde::Options* options) { if (options == nullptr) { return PrestoVectorSerde::PrestoOptions(); } return *(static_cast<const PrestoVectorSerde::PrestoOptions*>(options)); }

xiaoxmeng · 2023-07-12T08:03:46Z

velox/serializers/PrestoSerializer.h

+    explicit PrestoOptions() {}
+
+    explicit PrestoOptions(
+        bool useLosslessTimestamp,


s/useLosslessTimestamp/_useLosslessTimestamp/ s/compressionKind/_compressionKind/

xiaoxmeng · 2023-07-12T08:05:12Z

velox/serializers/PrestoSerializer.cpp

@@ -136,6 +136,18 @@ std::string typeToEncodingName(const TypePtr& type) {
  }
 }

+const PrestoVectorSerde::PrestoOptions toPrestoOptions(


s/const PrestoVectorSerde::PrestoOptions/PrestoVectorSerde::PrestoOptions/

Drop the const as return by value?

xiaoxmeng · 2023-07-12T08:07:51Z

velox/serializers/PrestoSerializer.cpp

 private:
  static const int32_t kSizeInBytesOffset{4 + 1};
  static const int32_t kHeaderSize{kSizeInBytesOffset + 4 + 4 + 8};

+  const StreamArena* streamArena_;


StreamArena* const streamArena_; const std::unique_ptr<folly::io::Codec> codec_;

velox/serializers/PrestoSerializer.cpp

xiaoxmeng · 2023-07-12T08:08:28Z

velox/serializers/PrestoSerializer.cpp

-      bool useLosslessTimestamp) {
+      bool useLosslessTimestamp,
+      dwio::common::CompressionKind compressionKind) {
+    streamArena_ = streamArena;


The same for the streamArena_ init

facebook-github-bot · 2023-07-12T21:22:12Z

@xiaoxmeng has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

jinchengchenghh · 2023-07-13T01:45:08Z

Thanks for the update. LGTM. Could you draw a diagram to show the serialized page layout with and without compression in header?

The serialized page layout is same with and without compression in header.
Just change the data layout.

xiaoxmeng

@jinchengchenghh Looks great. Thanks for iterations!

velox/dwio/common/Common.cpp

xiaoxmeng · 2023-07-14T05:29:16Z

velox/serializers/PrestoSerializer.cpp

-  readColumns(source, pool, childTypes, children, useLosslessTimestamp);
+  if (!needCompression(*codec)) {
+    // skip number of columns
+    source->skip(4);


Shall we add a check if the number of columns match? We currently see some serialized pages corruption in meta internal test. Thanks!

xiaoxmeng · 2023-07-14T05:29:24Z

velox/serializers/PrestoSerializer.cpp

+    ByteStream uncompressedSource;
+    uncompressedSource.resetInput({byteRange});
+    // skip number of columns
+    uncompressedSource.skip(4);


facebook-github-bot · 2023-07-14T05:30:40Z

@xiaoxmeng has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

xiaoxmeng · 2023-07-14T05:33:53Z

Thanks for the update. LGTM. Could you draw a diagram to show the serialized page layout with and without compression in header?

The serialized page layout is same with and without compression in header. Just change the data layout.

Thanks. I meant to add this diagram in the code. But we can do this followup if you'd like to help. Thanks!

mbasmanova · 2023-07-14T11:03:42Z

velox/dwio/common/tests/CommonTest.cpp

+  ASSERT_EQ(
+      folly::io::CodecType::LZ4,
+      compressionKindToCodec(CompressionKind::CompressionKind_LZ4)->type());
+  EXPECT_THROW(


Use VELOX_ASSERT_THROW

mbasmanova · 2023-07-14T11:05:23Z

velox/serializers/CMakeLists.txt

@@ -14,7 +14,7 @@
 add_library(velox_presto_serializer PrestoSerializer.cpp
                                    UnsafeRowSerializer.cpp)

-target_link_libraries(velox_presto_serializer velox_vector)
+target_link_libraries(velox_presto_serializer velox_dwio_common velox_vector)


I'm not sure if it is a good idea to add this dependency. What do we need from dwio::common? Can we refactor to extract what we need?

We need this file https://github.com/facebookincubator/velox/blob/main/velox/dwio/common/Common.h, I would suggest to rename this file to Compression.h, and extract it to velox_compression.

facebook-github-bot · 2023-07-17T05:51:16Z

@xiaoxmeng has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

mbasmanova · 2023-07-17T17:57:30Z

velox/dwio/common/compression/CMakeLists.txt

+  add_subdirectory(tests)
+endif()
+
+add_library(velox_dwio_common_compression Compression.cpp LzoDecompressor.cpp)


Does this need to be under dwio? Looks like it will be used for non-DWIO code. Maybe move it somewhere else. It would be nice to do this refactoring in a separate PR.

Where would you recommend to put it? Maybe velox/common/compression or velox/compression? I can help refactor it in a separate PR.

velox/common/compression sounds good. Thanks.

@jinchengchenghh I think you have moved to the wrong location: it should be velox/common/compression/Compression.h but not velox/dwio/common/compression/Compression.h? @mbasmanova

I will refactor in a separate PR as this discussion suggested. #5544 (comment)

@jinchengchenghh Thank you. Just to clarify, we'll wait for a refactoring PR to proceed.

xiaoxmeng

@jinchengchenghh could you rebase the PR to clear the ci test failure? Thanks!

facebook-github-bot · 2023-07-18T02:52:16Z

@xiaoxmeng has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

tanjialiang · 2023-07-18T23:08:10Z

velox/dwio/common/compression/Compression.cpp

+    case folly::io::CodecType::LZ4:
+      return CompressionKind_LZ4;
+    default:
+      VELOX_UNSUPPORTED("Not support folly codec type {}", type);


type.toString()?

This is a enum class folly::io::CodecType, it does not have toString function

tanjialiang · 2023-07-19T17:51:21Z

velox/dwio/common/compression/Compression.cpp

+    case CompressionKind_LZ4:
+      return "lz4";
+  }
+  return folly::to<std::string>("unknown - ", kind);


Why not put inside switch default?

This is old code, I just move it to another directory.

tanjialiang · 2023-07-19T17:51:47Z

velox/serializers/PrestoSerializer.h

@@ -15,20 +15,29 @@
 */
 #pragma once
 #include "velox/common/base/Crc.h"
+#include "velox/dwio/common/compression/Compression.h"
 #include "velox/vector/VectorStream.h"

 namespace facebook::velox::serializer::presto {
 class PrestoVectorSerde : public VectorSerde {
 public:
  // Input options that the serializer recognizes.


I understand this is not within this PR but can we make the comments triple slash as it's public? THanks

You could open a new PR to do that since it is no relevant change.

tanjialiang · 2023-07-19T17:52:01Z

velox/serializers/PrestoSerializer.h

+        dwio::common::CompressionKind _compressionKind)
+        : useLosslessTimestamp(_useLosslessTimestamp),
+          compressionKind(_compressionKind) {}
+
    // Currently presto only supports millisecond precision and the serializer


tanjialiang · 2023-07-19T17:53:12Z

velox/serializers/PrestoSerializer.cpp

+PrestoVectorSerde::PrestoOptions toPrestoOptions(
+    const VectorSerde::Options* options) {
+  if (options == nullptr) {
+    return PrestoVectorSerde::PrestoOptions();


Does returning NONE mean no compression or something wrong? I see a needCompression() method down there already can tell if compression is needed.

+1
@jinchengchenghh I know this is due to current code behavior but could you help either fix all the code sites to pass non-null VectorSerde::Options or have a unit test to make sure PrestoVectorSerde::PrestoOptions() returns common::CompressionKind::CompressionKind_NONE.

tanjialiang · 2023-07-19T17:53:42Z

velox/serializers/PrestoSerializer.cpp

@@ -1580,6 +1595,9 @@ class PrestoVectorSerializer : public VectorSerializer {
    }
  }

+  // The SerializedPage layout is:
+  // numRows(4) | codec(1) | uncompressedSize(4) | compressedSize(4) |
+  // checksum(8) | data


Nice comments. Can we make them triple slash /// as it's public?

velox/serializers/PrestoSerializer.cpp

xiaoxmeng

@jinchengchenghh file move PR is now committed. Can you rebase? Thanks!

facebook-github-bot · 2023-07-24T16:30:13Z

@xiaoxmeng has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2023-07-25T14:28:51Z

@xiaoxmeng merged this pull request in e80df93.

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jul 6, 2023

mbasmanova requested review from xiaoxmeng, bikramSingh91 and Yuhta July 6, 2023 14:51

jinchengchenghh force-pushed the compressup branch from 5fa9ca8 to 9c0d9ec Compare July 7, 2023 01:33

jinchengchenghh changed the title ~~Add support to compress buffer in VectorSerializer~~ Add support to compress data in VectorSerializer Jul 7, 2023

xiaoxmeng reviewed Jul 7, 2023

View reviewed changes

jinchengchenghh changed the title ~~Add support to compress data in VectorSerializer~~ Add support to compress data in PrestoVectorSerializer Jul 10, 2023

xiaoxmeng reviewed Jul 12, 2023

View reviewed changes

xiaoxmeng approved these changes Jul 14, 2023

View reviewed changes

jinchengchenghh force-pushed the compressup branch from 10a6fc1 to 3c374dc Compare July 14, 2023 06:47

mbasmanova reviewed Jul 14, 2023

View reviewed changes

jinchengchenghh force-pushed the compressup branch 2 times, most recently from d2cc78a to 0370416 Compare July 17, 2023 05:17

mbasmanova reviewed Jul 17, 2023

View reviewed changes

xiaoxmeng reviewed Jul 17, 2023

View reviewed changes

jinchengchenghh force-pushed the compressup branch from 0370416 to a6ac219 Compare July 18, 2023 01:06

tanjialiang reviewed Jul 18, 2023

View reviewed changes

tanjialiang reviewed Jul 19, 2023

View reviewed changes

xiaoxmeng reviewed Jul 21, 2023

View reviewed changes

jinchengchenghh force-pushed the compressup branch from b436e64 to c48af6b Compare July 24, 2023 01:26

jinchengchenghh added 2 commits July 24, 2023 08:25

Support compression in VectorSerializer

124efa0

Fix conflict

c48af6b

xiaoxmeng mentioned this pull request Jul 24, 2023

Introduce compressionKind in HiveInsertTableHandle #5795

Closed

facebook-github-bot closed this in e80df93 Jul 25, 2023

facebook-github-bot added the Merged label Jul 25, 2023

Add support to compress data in PrestoVectorSerializer #5544

Add support to compress data in PrestoVectorSerializer #5544

Conversation

jinchengchenghh commented Jul 6, 2023 • edited

netlify bot commented Jul 6, 2023 • edited

✅ Deploy Preview for meta-velox ready!

xiaoxmeng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jinchengchenghh commented Jul 10, 2023

xiaoxmeng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Jul 12, 2023

jinchengchenghh commented Jul 13, 2023 • edited

xiaoxmeng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Jul 14, 2023

xiaoxmeng commented Jul 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Jul 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiaoxmeng left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Jul 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiaoxmeng left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Jul 24, 2023

facebook-github-bot commented Jul 25, 2023

jinchengchenghh commented Jul 6, 2023 •

edited

netlify bot commented Jul 6, 2023 •

edited

jinchengchenghh commented Jul 13, 2023 •

edited