Refactor parquet PageReader to use PagedInputStream for decompression #5914

nmahadevuni · 2023-07-28T18:26:48Z

Resolves #5714

netlify · 2023-07-28T18:26:55Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`6bfc6c4`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/64ef015e7aa1840008efc334

velox/common/compression/CMakeLists.txt

nmahadevuni · 2023-08-01T17:11:03Z

New PR for decoupling compression classes into common package is #5946

nmahadevuni · 2023-08-08T16:58:15Z

@yingsu00 @Yuhta Updated this PR. Please review.

velox/dwio/common/compression/Compression.cpp

velox/dwio/common/compression/Compression.h

velox/dwio/parquet/reader/PageReader.cpp

velox/dwio/common/compression/Compression.h

velox/dwio/parquet/reader/PageReader.cpp

velox/dwio/common/compression/PagedInputStream.h

velox/dwio/parquet/reader/PageReader.cpp

velox/dwio/common/compression/Compression.h

nmahadevuni · 2023-08-16T18:04:29Z

@yingsu00 @Yuhta Addressed the comments. Please review again.

nmahadevuni · 2023-08-17T07:54:06Z

@yingsu00 @Yuhta Addressed the latest comments. Please review!

xiaoxmeng

@nmahadevuni thanks for the change. Can we add tests to verify the passed options have been handled properly. Thanks!

velox/dwio/common/compression/Compression.h

xiaoxmeng · 2023-08-21T16:47:41Z

velox/dwio/common/compression/Compression.h

-    uint32_t compressionThreshold,
-    int32_t zlibCompressionLevel,
-    int32_t zstdCompressionLevel,
+    CompressionOptions options,


Can you make CompressionOptions the last input parameter?

const CompressionOptions& options;

xiaoxmeng · 2023-08-21T16:49:10Z

velox/dwio/common/compression/Compression.h

+    const dwio::common::encryption::Decrypter* decryptr = nullptr,
+    bool useRawDecompression = false,
+    size_t compressedLength = 0,
+    CompressionOptions options = getDwrfOrcDecompressionOptions());


Why default is dwrf options? Compressor is also used for presto serializer. We could have a default compression options which is not file format specific.

xiaoxmeng · 2023-08-21T16:51:18Z

velox/dwio/common/compression/Compression.h

+  return options;
+}
+
+static CompressionOptions getParquetDecompressionOptions() {


Can we move this to parquet, dwrf code repo when they create compression options? Thanks!

xiaoxmeng · 2023-08-21T16:53:15Z

velox/dwio/common/compression/Compression.cpp

@@ -137,7 +139,9 @@ class ZlibDecompressor : public Decompressor {

 ZlibDecompressor::ZlibDecompressor(
    uint64_t blockSize,
-    const std::string& streamDebugInfo)
+    const std::string& streamDebugInfo,
+    int windowBits,


Move windowBits and isGzip ahead of streamDebugInfo? Thanks!

Moved windowBits. isGzip has a default value. Cannot move it.

xiaoxmeng · 2023-08-21T16:57:35Z

velox/dwio/common/compression/Compression.cpp

+            streamDebugInfo,
+            options.format.zlib.windowBits,
+            true,
+            useRawDecompression,


useRawDecompression can be zlib compression option? thanks!

No, this is for PagedInputStream, not specific to any compression format.

xiaoxmeng · 2023-08-21T17:00:38Z

velox/dwio/common/compression/PagedInputStream.h

@@ -28,7 +28,9 @@ class PagedInputStream : public dwio::common::SeekableInputStream {
      memory::MemoryPool& memPool,
      std::unique_ptr<Decompressor> decompressor,
      const dwio::common::encryption::Decrypter* decrypter,
-      const std::string& streamDebugInfo)
+      const std::string& streamDebugInfo,
+      bool useRawDecompression = false,


Can we move streamDebugInfo to the last input?

It has default arguments, streamDebugInfo can't be moved to last.

xiaoxmeng · 2023-08-21T17:01:37Z

velox/dwio/common/compression/Compression.h

+}
+
+static CompressionOptions getParquetDecompressionOptions() {
+  CompressionOptions options;


const CompressionOptions& options;

can't declare it const reference. needs initialization.

xiaoxmeng · 2023-08-21T17:01:50Z

velox/dwio/common/compression/Compression.h

+    const dwio::common::encryption::Decrypter* decryptr = nullptr,
+    bool useRawDecompression = false,
+    size_t compressedLength = 0,
+    CompressionOptions options = getDwrfOrcDecompressionOptions());


const CompressionOptions& options;

xiaoxmeng · 2023-08-21T17:03:02Z

velox/dwio/common/compression/Compression.h

-    const dwio::common::encryption::Decrypter* decryptr = nullptr);
+    const dwio::common::encryption::Decrypter* decryptr = nullptr,
+    bool useRawDecompression = false,
+    size_t compressedLength = 0,


const CompressionOptions& options;

nmahadevuni · 2023-08-25T12:11:03Z

@xiaoxmeng Addressed the comments. Please review again. @Yuhta @yingsu00

yingsu00 · 2023-08-29T05:07:23Z

velox/dwio/parquet/reader/PageReader.cpp

    uint32_t compressedSize,
-    uint32_t uncompressedSize,
+    uint32_t decompressedSize,


This should be uncompressedSize, which means the size before the data was compressed

yingsu00 · 2023-08-29T05:08:01Z

velox/dwio/parquet/reader/PageReader.cpp

-          "{} uncompress failed, remainingOutputSize is less then "
-          "uncompressedBlockLength, remainingOutputSize: {}, "
-          "uncompressedBlockLength: {}",
+          "{} decompress failed, remainingOutputSize is less then "


decompression failed

yingsu00 · 2023-08-29T05:08:10Z

velox/dwio/parquet/reader/PageReader.cpp

@@ -165,7 +165,7 @@ const char* FOLLY_NONNULL decompressLz4AndLzo(
      // Check that input length should not be negative.
      if (inputLength < sizeof(uint32_t)) {
        VELOX_FAIL(
-            "{} uncompress failed, input len is to small: {}",
+            "{} decompress failed, input len is to small: {}",


decompression failed

yingsu00 · 2023-08-29T05:31:40Z

velox/dwio/common/compression/Compression.h

 /**
 * Create a decompressor for the given compression kind.
 * @param kind the compression type to implement
 * @param input the input stream that is the underlying source
 * @param bufferSize the maximum size of the buffer
 * @param pool the memory pool
+ * @param useRawDecompression specify whether to perform raw decompression


Can you please make the comments order the same as the real arguments?

yingsu00 · 2023-08-29T05:33:52Z

velox/dwio/common/compression/Compression.h

@@ -87,10 +120,8 @@ std::unique_ptr<BufferedOutputStream> createCompressor(
    facebook::velox::common::CompressionKind kind,


This comment is for line 117: will you please update the comments accordingly?

yingsu00 · 2023-08-29T05:37:35Z

velox/dwio/dwrf/common/Compression.h

 constexpr uint8_t PAGE_HEADER_SIZE = 3;

+FOLLY_ALWAYS_INLINE const CompressionOptions getDwrfOrcCompressionOptions(


Why use FOLLY_ALWAYS_INLINE? This function is just called once when creating the compressor.

yingsu00 · 2023-08-29T05:37:50Z

velox/dwio/dwrf/common/Compression.h

      encrypter);
 }

+FOLLY_ALWAYS_INLINE const CompressionOptions getDwrfOrcDecompressionOptions() {


Why FOLLY_ALWAYS_INLINE?

yingsu00 · 2023-08-29T05:39:32Z

velox/dwio/parquet/reader/CMakeLists.txt

@@ -36,4 +36,5 @@ target_link_libraries(
  arrow
  Snappy::snappy
  thrift
-  zstd::zstd)
+  zstd::zstd
+  ${Protobuf_LIBRARIES})


Is this line related and necessary?

This is necessary since we are now having references to google::protobuf::io::ZeroCopyOutputStream via BufferedOutputStream from Parquet PageReader.h -> dwio/common/compression/Compression.h -> velox/dwio/common/OutputStream.h

undefined reference to `typeinfo for google::protobuf::io::ZeroCopyOutputStream' after this PR

I got the following link issue

/usr/bin/ld: velox/dwio/common/compression/libvelox_dwio_common_compression.a(Compression.cpp.o): in function `google::protobuf::io::ZeroCopyOutputStream::ZeroCopyOutputStream()': /home/chang/SourceCode/velox/cmake-build-debug-system/_deps/protobuf-src/src/google/protobuf/io/zero_copy_stream.h:193: undefined reference to `vtable for google::protobuf::io::ZeroCopyOutputStream' /usr/bin/ld: velox/dwio/common/compression/libvelox_dwio_common_compression.a(Compression.cpp.o): in function `google::protobuf::io::ZeroCopyOutputStream::~ZeroCopyOutputStream()': /velox/cmake-build-debug-system/_deps/protobuf-src/src/google/protobuf/io/zero_copy_stream.h:194: undefined reference to `vtable for google::protobuf::io::ZeroCopyOutputStream' /usr/bin/ld: velox/dwio/common/libvelox_dwio_common.a(OutputStream.cpp.o):(.[data.rel.ro](http://data.rel.ro/)._ZTIN8facebook5velox4dwio6common20BufferedOutputStreamE[_ZTIN8facebook5velox4dwio6common20BufferedOutputStreamE]+0x10): undefined reference to `typeinfo for google::protobuf::io::ZeroCopyOutputStream'

Here are my cmake option

-DVELOX_DEPENDENCY_SOURCE=BUNDLED -DVELOX_ENABLE_EXAMPLES=ON -DVELOX_ENABLE_PARQUET=ON -DVELOX_BUILD_TESTING=ON -DVELOX_ENABLE_BENCHMARKS=ON -DVELOX_ENABLE_BENCHMARKS_BASIC=ON

I checked probobuf cmake file and make sure disbale RTTI is OFF

Not sure how to fix it.

@baibaichen I was trying to do BUNDLED build using your cmake settings buy I'm getting a different error with Arrow build failing with Snappy.h file not found. I will sort that out. Can you move this protobuf link library to velox_dwio_common and check of it fixes the issue?

We should have fixed the protobuf header leak instead of propagating this dependency. I will open a PR to fix this.

yingsu00 · 2023-08-29T05:40:54Z

velox/dwio/parquet/reader/PageReader.h

@@ -28,6 +30,8 @@

 namespace facebook::velox::parquet {

+using namespace dwio::common::compression;


By convention, the header files do not use "using namespace". Can you please just inline these prefixes?

yingsu00

@nmahadevuni Mostly look good! Just a couple of nits...

yingsu00 · 2023-08-29T23:38:25Z

velox/dwio/common/compression/Compression.h

 /**
 * Create a decompressor for the given compression kind.
 * @param kind the compression type to implement
 * @param input the input stream that is the underlying source
 * @param bufferSize the maximum size of the buffer
 * @param pool the memory pool
+ * @param options compression options to use
+ * @param useRawDecompression specify whether to perform raw decompression


This parameter is only for GZIp and ZLIB, can you mention it in the comment?

It is currently for Gzip and Zlib, but can be used for any format. Do we want to change the comment?

Also compressionThreshold is not specific to these formats.

yingsu00 · 2023-08-29T23:40:38Z

velox/dwio/common/compression/Compression.h

+
+  uint32_t compressionThreshold;
+};
+
 /**
 * Create a decompressor for the given compression kind.
 * @param kind the compression type to implement


Can you please use Upper case for the first characters? E.g.
@param kind The compression type to implement

ditto

yingsu00 · 2023-08-29T23:41:30Z

velox/dwio/dwrf/common/Compression.h

+
+/**
+ * Create a decompressor for the given compression kind.
+ * @param kind the compression type to implement


Upper case for the first chars

nmahadevuni · 2023-08-30T11:13:32Z

@ying, can you rerun the failed test? It is not related.

facebook-github-bot · 2023-09-05T13:58:24Z

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

facebook-github-bot · 2023-09-14T13:59:39Z

@Yuhta merged this pull request in 95f608b.

conbench-facebook · 2023-09-14T14:20:44Z

Conbench analyzed the 1 benchmark run on commit 95f608b3.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

…pression (facebookincubator#5914)" This reverts commit 95f608b.

…facebookincubator#5914) Summary: Resolves facebookincubator#5714 Pull Request resolved: facebookincubator#5914 Reviewed By: mbasmanova Differential Revision: D48521298 Pulled By: Yuhta fbshipit-source-id: 1a5f4204f02b556dfa03da125d1a587f9aae70f6

…pression (facebookincubator#5914)" This reverts commit 95f608b.

…facebookincubator#5914) Summary: Resolves facebookincubator#5714 Pull Request resolved: facebookincubator#5914 Reviewed By: mbasmanova Differential Revision: D48521298 Pulled By: Yuhta fbshipit-source-id: 1a5f4204f02b556dfa03da125d1a587f9aae70f6

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 28, 2023

Yuhta reviewed Jul 28, 2023

View reviewed changes

velox/common/compression/CMakeLists.txt Outdated Show resolved Hide resolved

nmahadevuni force-pushed the parquet_refactor_uncompress branch from a88c811 to 6c11c4c Compare July 28, 2023 18:49

nmahadevuni changed the title ~~Refact parquet PageReader to use PagedInputStream for decompression~~ Refactor parquet PageReader to use PagedInputStream for decompression Aug 2, 2023

nmahadevuni mentioned this pull request Aug 2, 2023

Refactor compression classes into common package #5946

Closed

nmahadevuni force-pushed the parquet_refactor_uncompress branch 2 times, most recently from 1bc9754 to 371264e Compare August 8, 2023 12:47

Yuhta self-requested a review August 8, 2023 23:11

Yuhta reviewed Aug 10, 2023

View reviewed changes

velox/dwio/common/compression/Compression.cpp Outdated Show resolved Hide resolved

velox/dwio/common/compression/Compression.h Outdated Show resolved Hide resolved

nmahadevuni force-pushed the parquet_refactor_uncompress branch from 371264e to 67fa4cf Compare August 11, 2023 10:56

nmahadevuni closed this Aug 11, 2023

nmahadevuni force-pushed the parquet_refactor_uncompress branch from 67fa4cf to d26b6de Compare August 11, 2023 12:30

nmahadevuni reopened this Aug 11, 2023

yingsu00 added the parquet label Aug 14, 2023

yingsu00 reviewed Aug 14, 2023

View reviewed changes

velox/dwio/parquet/reader/PageReader.cpp Show resolved Hide resolved

yingsu00 mentioned this pull request Aug 14, 2023

Consolidate LZ4 and LZ0 decompression of Parquet to dwio::common #6105

Closed

yingsu00 reviewed Aug 15, 2023

View reviewed changes

Yuhta reviewed Aug 15, 2023

View reviewed changes

velox/dwio/common/compression/Compression.h Outdated Show resolved Hide resolved

Yuhta reviewed Aug 15, 2023

View reviewed changes

velox/dwio/common/compression/Compression.h Outdated Show resolved Hide resolved

nmahadevuni force-pushed the parquet_refactor_uncompress branch 2 times, most recently from 6e831c9 to 83b4e7b Compare August 16, 2023 16:29

nmahadevuni force-pushed the parquet_refactor_uncompress branch from 83b4e7b to d979be5 Compare August 17, 2023 06:41

Yuhta self-requested a review August 18, 2023 16:05

Yuhta mentioned this pull request Aug 21, 2023

Do pre-decompress gzip with IAA hardware #5718

Open

xiaoxmeng reviewed Aug 21, 2023

View reviewed changes

yaqi-zhao added a commit to yaqi-zhao/velox that referenced this pull request Aug 24, 2023

rebase on facebookincubator#5914

85cf8b8

yaqi-zhao added a commit to yaqi-zhao/velox that referenced this pull request Aug 24, 2023

rebase on facebookincubator#5914

6ff1026

yaqi-zhao added a commit to yaqi-zhao/velox that referenced this pull request Aug 24, 2023

rebase on facebookincubator#5914

842de1b

nmahadevuni force-pushed the parquet_refactor_uncompress branch 2 times, most recently from c139534 to b69418a Compare August 25, 2023 11:01

yingsu00 reviewed Aug 29, 2023

View reviewed changes

nmahadevuni force-pushed the parquet_refactor_uncompress branch from b69418a to db93429 Compare August 29, 2023 07:43

yingsu00 reviewed Aug 29, 2023

View reviewed changes

nmahadevuni added 2 commits August 30, 2023 14:00

Refactor parquet PageReader to use PagedInputStream for decompression

bb85435

Rename uncompress to decompress in parquet PageReader

6bfc6c4

nmahadevuni force-pushed the parquet_refactor_uncompress branch from db93429 to 6bfc6c4 Compare August 30, 2023 08:44

yingsu00 approved these changes Aug 31, 2023

View reviewed changes

yaqi-zhao mentioned this pull request Sep 5, 2023

gzip pre-decompress w/IAA #6176

Closed

yaqi-zhao added a commit to yaqi-zhao/velox that referenced this pull request Sep 7, 2023

rebase on facebookincubator#5914

b6ce906

facebook-github-bot closed this in 95f608b Sep 14, 2023

facebook-github-bot added the Merged label Sep 14, 2023

gaoyangxiaozhu pushed a commit to gayangya/velox that referenced this pull request Sep 25, 2023

Revert "Refactor parquet PageReader to use PagedInputStream for decom…

4b20e5e

…pression (facebookincubator#5914)" This reverts commit 95f608b.

baibaichen mentioned this pull request Sep 26, 2023

undefined reference to `typeinfo for google::protobuf::io::ZeroCopyOutputStream' after #5914 #6744

Closed

gaoyangxiaozhu pushed a commit to gayangya/velox that referenced this pull request Sep 28, 2023

Revert "Refactor parquet PageReader to use PagedInputStream for decom…

099a234

…pression (facebookincubator#5914)" This reverts commit 95f608b.

yingsu00 mentioned this pull request Dec 1, 2023

Unify the compression API in Velox #7471

Open

		@@ -87,10 +120,8 @@ std::unique_ptr<BufferedOutputStream> createCompressor(
		facebook::velox::common::CompressionKind kind,

		constexpr uint8_t PAGE_HEADER_SIZE = 3;

		FOLLY_ALWAYS_INLINE const CompressionOptions getDwrfOrcCompressionOptions(

		@@ -28,6 +30,8 @@

		namespace facebook::velox::parquet {

		using namespace dwio::common::compression;

Refactor parquet PageReader to use PagedInputStream for decompression #5914

Refactor parquet PageReader to use PagedInputStream for decompression #5914

Conversation

nmahadevuni commented Jul 28, 2023 • edited by majetideepak Loading

netlify bot commented Jul 28, 2023 • edited Loading

✅ Deploy Preview for meta-velox canceled.

nmahadevuni commented Aug 1, 2023

nmahadevuni commented Aug 8, 2023

nmahadevuni commented Aug 16, 2023

nmahadevuni commented Aug 17, 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

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

nmahadevuni commented Aug 25, 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

nmahadevuni Aug 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

undefined reference to `typeinfo for google::protobuf::io::ZeroCopyOutputStream' after this PR

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yingsu00 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

nmahadevuni commented Aug 30, 2023

facebook-github-bot commented Sep 5, 2023

facebook-github-bot commented Sep 14, 2023

conbench-facebook bot commented Sep 14, 2023

nmahadevuni commented Jul 28, 2023 •

edited by majetideepak

Loading

netlify bot commented Jul 28, 2023 •

edited

Loading

nmahadevuni Aug 29, 2023 •

edited

Loading