GH-38271: [C++][Parquet] Support reading parquet files with multiple gzip members by amassalha · Pull Request #38272 · apache/arrow · GitHub

amassalha · 2023-10-15T05:36:06Z

What changes are included in this PR?

Adding support in GZipCodec to decompress concatenated gzip members

Are these changes tested?

test is attached

Are there any user-facing changes?

no

Closes: Support decompressing concatenated gzip members (stream) #38271

github-actions · 2023-10-15T05:36:34Z

⚠️ GitHub issue #38271 has been automatically assigned in GitHub to PR creator.

wgtmac

Thanks for the contribution! Could you please add a round trip test for the GZipCodec, which will fail decoding without you fix?

mapleFU · 2023-10-16T02:45:56Z

@amassalha How does the gzip streaming parquet file generated? This patch looks general ok but I want to know how other system support file like this

amassalha · 2023-10-16T10:35:43Z

@mapleFU to generate gzip streaming we use our internal high-performance gzip implementation which is fully adheres to the gzip RFC.
btw, the file I added can be read successfully using spark (tested with spark-shell)
@wgtmac Roundtrip test added
(patches updated)

mapleFU · 2023-10-17T15:55:11Z

Yeah, I know it's allowed by the protocol. I will investigate arrow-rs and parquet-mr about can we write or read file like this.

Also, do you know what system could generate parquet file with multiple gzip members?

wgtmac · 2023-10-18T01:57:04Z

I agree with @mapleFU. Did you have an example parquet file with multiple gzip members on hand? We need to make sure parquet-mr can decompress it successfully. If you don't mind, please upload one here and I can verify it on my end.

amassalha · 2023-10-18T04:11:10Z

@wgtmac the parquet file is attached to the issue:
#38271

I also tried pushing it to parquet-testing repo (https://github.com/apache/parquet-testing) but somehow I couldn't creat a pull request (thats what causing the failiures in this run, it can't find the file). any suggestions on that? is 'parquet-testing' also opened for pull requests ?

@mapleFU Im not familier with other system (than ours) curently

mapleFU · 2023-10-18T04:39:36Z

CI failed might related to the lint problem. You can run clang-format -i with your edited files.

How do you generate that parquet-file?

amassalha · 2023-10-18T04:41:44Z

we use our internal high-performance gzip implementation which is fully adheres to the gzip RFC.

As I mentioned, we use our internal high-performance gzip implementation which is fully adheres to the gzip RFC. (I can't give more details, I work in a startup)

mapleFU · 2023-10-18T04:46:28Z

https://github.com/apache/parquet-format/blob/master/src/main/thrift/parquet.thrift#L496-L498

Parquet also has two versions of LZ4 in standard. I've no issue about gzip standard, but maybe I need to make sure this kind of file is supported.

From the standard ( https://github.com/apache/parquet-format/blob/master/Compression.md#gzip ), the link is authority. Let me go through the parquet-mr impl tonight to make it clear.

mapleFU · 2023-10-18T04:54:11Z

@amassalha I've try current implemention, seems that we're able to read the concatenated_gzip_members.zip . Which contains 513 rows in this file. Can you confirm that?

amassalha · 2023-10-18T05:03:17Z

@amassalha I've try current implemention, seems that we're able to read the concatenated_gzip_members.zip . Which contains 513 rows in this file. Can you confirm that?

to read with current values? because when I tried using "ReadBatch" it returned "0" instead of last number (513)

mapleFU · 2023-10-18T05:14:17Z

Ah you're right!

I've check the file, read with arrow-rs:

 arrow-rs git:(master) ✗ .//target/debug/parquet-read /Users/fuxuwei/Downloads/concatenated_gzip_members_2.parquet 
thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: General("Actual decompressed size doesn't match the expected one (4099 vs 4107)")', parquet/src/record/reader.rs:583:36
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

@wgtmac Would you mind also check it using parquet-mr?

wgtmac · 2023-10-18T05:17:42Z

parquet-mr seems to be happy with the file.

➜  Downloads parquet-cli meta concatenated_gzip_members.parquet

File path:  concatenated_gzip_members.parquet
Created by: null
Properties: (none)
Schema:
message root {
  optional int64 long_col (INTEGER(64,false));
}


Row group 0:  count: 513  2.86 B records  start: 4  total(compressed): 1.433 kB total(uncompressed):4.058 kB
--------------------------------------------------------------------------------
          type      encodings count     avg size   nulls   min / max
long_col  INT64     G RB_     513       2.86 B             "1" / "513"

➜  Downloads parquet-cli cat concatenated_gzip_members.parquet
{"long_col": 1}
{"long_col": 2}
{"long_col": 3}
{"long_col": 4}
{"long_col": 5}
...
{"long_col": 511}
{"long_col": 512}
{"long_col": 513}

mapleFU · 2023-10-18T05:18:46Z

Yeah, seems it's a bug we need fix, thanks @amassalha ! I'll checkout why arrow-rs cannot open file like that

amassalha · 2023-10-18T05:20:27Z

Yeah, seems it's a bug we need fix, thanks @amassalha ! I'll checkout why arrow-rs cannot open file like that

great, thank you

wgtmac · 2023-10-18T07:02:19Z

nit: use std::array or std::vector instead of raw array.

wgtmac · 2023-10-18T07:03:11Z

Suggested change

for (int data_half_size : sizes) {

for (int data_half_size : {0, 10000, 100000}) {

Or simply do this.

wgtmac · 2023-10-18T07:04:36Z

Suggested change

std::unique_ptr<Codec> c1;

ASSERT_OK_AND_ASSIGN(c1, Codec::Create(Compression::GZIP));

ASSERT_OK_AND_ASSIGN(auto gzip_codec, Codec::Create(Compression::GZIP));

Let's name it more meaningfully.

wgtmac · 2023-10-18T07:06:41Z

Suggested change

ASSERT_OK_AND_ASSIGN(actual_size_p1, c1->Compress(data_half.size(), data_half.data(),

max_compressed_len_half, compressed.data()));

ASSERT_OK_AND_ASSIGN(actual_size_p2, c1->Compress(data_half.size(), data_half.data(),

max_compressed_len_half, compressed.data()+actual_size_p1));

ASSERT_OK_AND_ASSIGN(int64_t actual_size_p1, gzip_codec->Compress(data_half.size(), data_half.data(),

max_compressed_len_half, compressed.data()));

ASSERT_OK_AND_ASSIGN(int64_t actual_size_p2, gzip_codec->Compress(data_half.size(), data_half.data(),

max_compressed_len_half, compressed.data() + actual_size_p1));

We can simply do this.

wgtmac · 2023-10-18T07:07:32Z

Suggested change

std::vector<uint8_t> decompressed(data_half_size*2);

std::vector<uint8_t> decompressed(data_half_size * 2);

wgtmac · 2023-10-18T07:07:41Z

Suggested change

ASSERT_EQ(data_half.size()*2, actual_decompressed_size);

ASSERT_EQ(data_half.size() * 2, actual_decompressed_size);

mapleFU · 2023-10-18T13:00:07Z

I found arrow-cpp we can read the file because we don't check the decompress size:

  // Decompress the values
  PARQUET_ASSIGN_OR_THROW(auto decompress_len, decompressor_->Decompress(
      compressed_len - levels_byte_len, page_buffer->data() + levels_byte_len,
      uncompressed_len - levels_byte_len,
      decompression_buffer_->mutable_data() + levels_byte_len));
  if (decompress_len != uncompressed_len - levels_byte_len) {
    throw ParquetException("Expected " + std::to_string(uncompressed_len - levels_byte_len) +
                           " bytes but decompressed " + std::to_string(decompress_len));
  }

@wgtmac @pitrou Do you think we need to add the check above?

pitrou · 2023-10-18T13:07:24Z

@mapleFU Yes, I think it would be good regardless of whether we decide to support this feature or not.

mapleFU · 2023-11-15T10:32:15Z

@amassalha

PARQUET-2369: Add new test file concatenated_gzip_members.parquet parquet-testing#41 is merged now, since we can update the testing
Also, I've update the decompression check, previously when you implement this, the final value will be 0, but now, without this patch, reading file like this will throw exception

pitrou

Thanks for the update! Here are two more comments.
Also, can you fix the C++ lint failures (see CI results)?

linting review fixes code review fixes2 code review fixes Update testing Update parquet-testing flint fixes Update compression_test.cc

amassalha · 2023-11-22T10:11:39Z

thanks for the comments all
@wgtmac I think its your call now (all tests passed and I fixed review comments)

amassalha · 2023-11-22T11:30:38Z

I see a timeout fail in 3sfs, I don't see how this is related to gzip, do you have a flaky / non-deterministic bug?
(it passed before the review changes)

mapleFU · 2023-11-22T11:32:25Z

Don't worry I'll rerun it.

amassalha · 2023-11-22T11:34:15Z

Don't worry I'll rerun it.

great, thanks

amassalha · 2023-11-26T04:56:46Z

@wgtmac any comments from your side?

wgtmac · 2023-11-26T14:59:04Z

I don't have any concern. It seems that you need rebase it to get merged.

…members

amassalha · 2023-11-27T04:52:54Z

@wgtmac I rebased, and got same random fale of sf3, should we just rerun or what?

amassalha · 2023-11-27T12:35:00Z

@mapleFU how can I rerun this?

pitrou · 2023-11-27T13:04:54Z

@amassalha There is no need to re-run, these failures are unrelated.

pitrou · 2023-11-27T13:05:43Z

@wgtmac Would you like to review this again? Otherwise, I will just merge this PR.

wgtmac · 2023-11-27T14:40:12Z

@wgtmac Would you like to review this again? Otherwise, I will just merge this PR.

Thanks for notifying me! Feel free to merge this.

amassalha · 2023-11-29T04:59:31Z

@wgtmac @pitrou can we finish this before another rebase is needed?

mapleFU · 2023-11-29T05:05:58Z

I'll merge it

amassalha · 2023-11-29T05:09:19Z

thanks!

conbench-apache-arrow · 2023-11-29T21:04:17Z

After merging your PR, Conbench analyzed the 5 benchmarking runs that have been run so far on merge-commit 1d904d6.

There were 5 benchmark results indicating a performance regression:

Commit Run on ursa-i9-9960x at 2023-11-29 18:52:11Z
- tpch (R) with engine=arrow, format=native, language=R, memory_map=False, query_id=TPCH-11, scale_factor=1
- file-read (Python) with compression=uncompressed, dataset=fanniemae_2016Q4, file_type=feather, output_type=dataframe
and 3 more (see the report linked below)

The full Conbench report has more details. It also includes information about 6 possible false positives for unstable benchmarks that are known to sometimes produce them.

…tiple gzip members (apache#38272) ### What changes are included in this PR? Adding support in GZipCodec to decompress concatenated gzip members ### Are these changes tested? test is attached ### Are there any user-facing changes? no * Closes: apache#38271 Lead-authored-by: amassalha <amassalha@speedata.io> Co-authored-by: Atheel Massalha <amassalha@speedata.io> Signed-off-by: mwish <maplewish117@gmail.com>

amassalha requested a review from wgtmac as a code owner October 15, 2023 05:36

github-actions Bot added Component: Parquet Component: C++ awaiting review Awaiting review labels Oct 15, 2023

wgtmac requested changes Oct 16, 2023

View reviewed changes

Comment thread cpp/src/arrow/util/compression_zlib.cc Outdated

Comment thread cpp/src/arrow/util/compression_zlib.cc Outdated

wgtmac changed the title ~~GH-38271: [C++,Parquet] Support reading parquet files with multiple gzip members~~ GH-38271: [C++][Parquet] Support reading parquet files with multiple gzip members Oct 16, 2023

github-actions Bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Oct 16, 2023

amassalha requested a review from wgtmac October 16, 2023 10:44

wgtmac reviewed Oct 18, 2023

View reviewed changes

amassalha mentioned this pull request Oct 18, 2023

PARQUET-2369: Add new test file concatenated_gzip_members.parquet apache/parquet-testing#41

Merged

Fix decompressing concatenated gzip members

4be8493

pitrou reviewed Nov 20, 2023

View reviewed changes

Comment thread cpp/src/parquet/reader_test.cc Outdated

pitrou reviewed Nov 20, 2023

View reviewed changes

Comment thread cpp/src/parquet/reader_test.cc Outdated

pitrou reviewed Nov 20, 2023

View reviewed changes

Comment thread cpp/src/parquet/reader_test.cc Outdated

amassalha requested a review from pitrou November 21, 2023 15:55

pitrou requested changes Nov 21, 2023

View reviewed changes

Comment thread cpp/src/arrow/util/compression_zlib.cc Outdated

Comment thread cpp/src/parquet/reader_test.cc Outdated

amassalha requested a review from pitrou November 22, 2023 06:08

fix code review comments

a22d6a5

linting review fixes code review fixes2 code review fixes Update testing Update parquet-testing flint fixes Update compression_test.cc

amassalha force-pushed the atheel/decode_gzip_members branch from c7dc64e to a22d6a5 Compare November 22, 2023 10:00

pitrou approved these changes Nov 22, 2023

View reviewed changes

amassalha added 2 commits November 26, 2023 17:33

Merge remote-tracking branch 'upstream/main' into atheel/decode_gzip_…

c34c822

…members

Update parquet-testing

f85cffe

wgtmac approved these changes Nov 27, 2023

View reviewed changes

mapleFU merged commit 1d904d6 into apache:main Nov 29, 2023

mapleFU removed the awaiting committer review Awaiting committer review label Nov 29, 2023

	for (int data_half_size : sizes) {
	for (int data_half_size : {0, 10000, 100000}) {

	std::unique_ptr<Codec> c1;
	ASSERT_OK_AND_ASSIGN(c1, Codec::Create(Compression::GZIP));
	ASSERT_OK_AND_ASSIGN(auto gzip_codec, Codec::Create(Compression::GZIP));

-    ASSERT_OK_AND_ASSIGN(actual_size_p1, c1->Compress(data_half.size(), data_half.data(),
-                                         max_compressed_len_half, compressed.data()));
-    ASSERT_OK_AND_ASSIGN(actual_size_p2, c1->Compress(data_half.size(), data_half.data(),
-                                         max_compressed_len_half, compressed.data()+actual_size_p1));
+    ASSERT_OK_AND_ASSIGN(int64_t actual_size_p1, gzip_codec->Compress(data_half.size(), data_half.data(),
+                                         max_compressed_len_half, compressed.data()));
+    ASSERT_OK_AND_ASSIGN(int64_t actual_size_p2, gzip_codec->Compress(data_half.size(), data_half.data(),
+                                         max_compressed_len_half, compressed.data() + actual_size_p1));

	std::vector<uint8_t> decompressed(data_half_size*2);
	std::vector<uint8_t> decompressed(data_half_size * 2);

	ASSERT_EQ(data_half.size()*2, actual_decompressed_size);
	ASSERT_EQ(data_half.size() * 2, actual_decompressed_size);

Conversation

amassalha commented Oct 15, 2023 • edited by github-actions Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

github-actions Bot commented Oct 15, 2023

Uh oh!

wgtmac left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mapleFU commented Oct 16, 2023

Uh oh!

amassalha commented Oct 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mapleFU commented Oct 17, 2023

Uh oh!

wgtmac commented Oct 18, 2023

Uh oh!

amassalha commented Oct 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mapleFU commented Oct 18, 2023

Uh oh!

amassalha commented Oct 18, 2023

Uh oh!

mapleFU commented Oct 18, 2023

Uh oh!

mapleFU commented Oct 18, 2023

Uh oh!

amassalha commented Oct 18, 2023

Uh oh!

mapleFU commented Oct 18, 2023

Uh oh!

wgtmac commented Oct 18, 2023

Uh oh!

mapleFU commented Oct 18, 2023

Uh oh!

amassalha commented Oct 18, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mapleFU commented Oct 18, 2023

Uh oh!

pitrou commented Oct 18, 2023

Uh oh!

mapleFU commented Nov 15, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

amassalha commented Nov 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amassalha commented Nov 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

amassalha commented Oct 15, 2023 •

edited by github-actions Bot

Loading

amassalha commented Oct 16, 2023 •

edited

Loading

amassalha commented Oct 18, 2023 •

edited

Loading

amassalha commented Nov 22, 2023 •

edited

Loading

amassalha commented Nov 22, 2023 •

edited

Loading

amassalha commented Nov 27, 2023 •

edited

Loading