GH-35287: [C++][Parquet] Add CodecOptions to customize the compression parameter by yyang52 · Pull Request #35886 · apache/arrow

yyang52 · 2023-06-02T08:58:39Z

Rationale for this change

Based on #35287, we'd like to add a CodecOptions to make more compression parameters (such as window_bits) customizable when creating the Codec for parquet writer.

Authored-by: Yang Yang yang10.yang@intel.com
Co-authored-by: Rambacher, Mark mark.rambacher@intel.com

What changes are included in this PR?

Add CodecOptions and replace compression_level when creating the Codec. The design is basically based on previous discussions.

Are these changes tested?

Yes

Are there any user-facing changes?

Yes, when user creates the WriterProperties

Closes: [C++][Parquet] Expose the API to fine tune the window_bits parameter of ZLIB compression #35287

github-actions · 2023-06-02T08:58:59Z

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

mapleFU

Seems parquet part change all compression level to options. Do we need to keep api compatible?

cpp/src/parquet/properties.h

mapleFU · 2023-06-05T09:22:16Z

cpp/src/parquet/column_writer.h

This should be put to the end for capability

Also, unless the PageWriter needs to keep ownership of the codec options, this should simply be const CodecOptions& = {}.

In addition to the comments above, should be consolidate Compression::type codec and const std::shared_ptr<CodecOptions>& codec_options? codec_options should know codec.

mapleFU · 2023-06-05T09:24:00Z

cpp/src/parquet/properties.h

Since Codec contains compression_level_, would this be duplicated or useless?

Now we have a compression_level and a CodecOptions. Previously, user can set compression_level, and the compression level would be changed.

After this patch, if user change the compression level, the codec will not be changed.

Can we make CodecOptions mutate it's compression_level, and make set_compression_level set the level in CodecOptions directly?

that makes sense :)

Can we just get it from codec_options_?

+1 on this. compression_level_ is duplicated.

mapleFU · 2023-06-05T09:24:47Z

cc @pitrou @wgtmac

wgtmac · 2023-06-05T09:47:26Z

cpp/src/arrow/util/compression.h

This is a breaking change. It would be better to add a new overload. Then implement this one based on the new overload and label it as deprecated.

Yes, that could solve the compatibility issue of different APIs. Just to make it clear, you suggest marking the newly added API with CodecOptions as deprecated right? Or the previous one?

I mean deprecating the existing one.

Great, that makes sense :)

cpp/src/parquet/types.h

wgtmac · 2023-06-05T09:51:27Z

cpp/src/parquet/column_writer.cc

I am not sure if we are able to move codec into codec_options as well. I don't have a preference here.

pitrou · 2023-06-06T09:43:34Z

cpp/src/arrow/util/compression_lz4.cc

Those changes shouldn't be necessary? (also, why iostream?)

pitrou · 2023-06-06T09:44:53Z

cpp/src/arrow/util/compression_brotli.cc

Let's make the error message respect the macro values:

Suggested change

return Status::Invalid("window_bits should be within 10 ~ 24");

return Status::Invalid("window_bits should be between ", BROTLI_MIN_WINDOW_BITS,

" and ", BROTLI_MAX_WINDOW_BITS);

pitrou · 2023-06-06T09:46:36Z

cpp/src/arrow/util/compression_zlib.cc

Nit, but can you undo the include ordering change? It's a bit gratuitous. Also, it makes things more readable to separate stdlib imports from third-party library imports.

Sure, that might be due to some local format auto-change, will revert those.

cpp/src/arrow/util/compression_zlib.cc

pitrou · 2023-06-06T09:47:48Z

cpp/src/arrow/util/compression_zlib.cc

This is a bit convoluted

Suggested change

int CompressionWindowBitsForFormat(GZipFormat::type format, int input_window_bits) {

int window_bits = input_window_bits;

int CompressionWindowBitsForFormat(GZipFormat::type format, int window_bits) {

cpp/src/parquet/column_writer.h

pitrou · 2023-06-06T10:05:56Z

cpp/src/parquet/types.h

const CodecOptions&

pitrou · 2023-06-06T10:06:16Z

cpp/src/parquet/types.h

Again, I'm not sure it's useful to deprecate this one.

cpp/src/parquet/types.cc

pitrou · 2023-06-06T10:07:37Z

r/src/compression.cpp

Can probably revert this.

pitrou · 2023-06-06T10:10:30Z

@yyang52 There are Python test failures on CI.

Also, is it possible to pass a base CodecOptions when a derived class exists? (e.g. for gzip). Ideally it should be supported.

yyang52 · 2023-06-08T08:39:20Z

@yyang52 There are Python test failures on CI.

Also, is it possible to pass a base CodecOptions when a derived class exists? (e.g. for gzip). Ideally it should be supported.

Yes, I think it supports that (as checked in TEST(TestCodecMisc, SpecifyCompressionLevel) to pass base CodecOptions to different Codecs)

mapleFU

I'm general ok with the parquet side changes now, there are some comments.

mapleFU · 2023-06-09T06:12:16Z

cpp/src/arrow/util/compression.h

Can it use BROTLI_DEFAULT_WINDOW?

yes, but then we need to include the brotli header here as well?

Yes you're right. I'm not familiar with compression algorithms, so maybe I'm asking a stupid question. but I'm afraid that brorli might change it's default bits.

Would it be ok for a optional window_bits, if it's set, use it, otherwise using default value?

yes, I think that's just how it is implemented now in GZip/BrotliCodecOptions?

Sure, I notice that this patch hard code the encoder's internal into compression.h, so I'm afraid if they're changed in the 3rd libraries, we'll get confused here.

yeah it seems better to use their BROTLI_DEFAULT_WINDOW

Same with my previous comment. We can use BROTLI_DEFAULT_WINDOW in the .cc file

mapleFU · 2023-06-09T06:14:11Z

cpp/src/parquet/column_io_benchmark.cc

mapleFU · 2023-06-09T06:14:48Z

cpp/src/parquet/column_writer.cc

I think just *codec_options is ok?

cpp/src/arrow/util/compression.h

mapleFU · 2023-06-09T06:21:04Z

cpp/src/parquet/properties.h

Now we have a compression_level and a CodecOptions. Previously, user can set compression_level, and the compression level would be changed.

After this patch, if user change the compression level, the codec will not be changed.

Can we make CodecOptions mutate it's compression_level, and make set_compression_level set the level in CodecOptions directly?

yyang52 · 2023-07-06T07:06:12Z

May you help to review the code change as well as trigger the remaining CI checks?

May you help to trigger the remaining CI checks again? Thanks! @pitrou

pitrou · 2023-07-06T14:16:21Z

cpp/src/parquet/column_writer.h

+      OffsetIndexBuilder* offset_index_builder = NULLPTR,
+      const CodecOptions& codec_options = CodecOptions{});
+
+  ARROW_DEPRECATED("Use the one with CodecOptions instead")


Suggested change

ARROW_DEPRECATED("Use the one with CodecOptions instead")

ARROW_DEPRECATED("Deprecated in 13.0.0. Use CodecOptions-taking overload instead.")

pitrou · 2023-07-06T14:26:19Z

cpp/src/arrow/util/compression.h

+  }
+  virtual ~CodecOptions() = default;
+
+  int compression_level_ = kUseDefaultCompressionLevel;


Forgot this, but style nit (this is a public member, it's weird to add a trailing underscore):

Suggested change

int compression_level_ = kUseDefaultCompressionLevel;

int compression_level = kUseDefaultCompressionLevel;

pitrou

@yyang52 There are a couple of comments left to address, can you take a look?

mapleFU

Parquet part looks ok to me

yyang52 · 2023-07-07T01:26:44Z

@yyang52 There are a couple of comments left to address, can you take a look?

Doing some fix, may you help to take a look and trigger remaining CI checks? Thanks! @pitrou

wgtmac

LGTM. Nice work!

…ions

pitrou · 2023-07-11T14:01:38Z

@github-actions crossbow submit -g cpp

github-actions · 2023-07-11T14:04:21Z

Revision: 73b4018

Submitted crossbow builds: ursacomputing/crossbow @ actions-e0a8b0150b

Task	Status
test-alpine-linux-cpp
test-build-cpp-fuzz
test-conda-cpp
test-conda-cpp-valgrind
test-cuda-cpp
test-debian-11-cpp-amd64
test-debian-11-cpp-i386
test-fedora-35-cpp
test-ubuntu-20.04-cpp
test-ubuntu-20.04-cpp-bundled
test-ubuntu-20.04-cpp-minimal-with-formats
test-ubuntu-20.04-cpp-thread-sanitizer
test-ubuntu-22.04-cpp
test-ubuntu-22.04-cpp-20

pitrou · 2023-07-11T15:19:12Z

Thanks a lot for your contribution @yyang52 ! I hope you didn't mind the delays.

yyang52 · 2023-07-12T01:37:12Z

Thanks for your kind support! It's a pleasure to make contribution to Arrow community! @pitrou @mapleFU @wgtmac

conbench-apache-arrow · 2023-07-21T18:47:40Z

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

There were no benchmark performance regressions. 🎉

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

mapleFU · 2023-07-23T10:47:05Z

cpp/src/parquet/file_writer.cc

+      if (!codec_options) {
+        pager = PageWriter::Open(sink_, properties_->compression(path), col_meta,
+                                 row_group_ordinal_, static_cast<int16_t>(column_ordinal),
+                                 properties_->memory_pool(), false, meta_encryptor,


This part lost buffered_row_group_, and set it to false. Sorry that I didn't found it at first time

Sorry for this missing and thanks for the fix!

… Parquet column (#38025) ### Rationale for this change After the changes in #35886, getting the compression level for a Parquet column segfaults if the compression level or other options weren't previously set ### What changes are included in this PR? Adds a null check on the codec options of the column properties before trying to access the compression level. ### Are these changes tested? Yes, I added a unit test. ### Are there any user-facing changes? This fixes a regression added after 13.0.0 so isn't a user-facing fix * Closes: #38039 Authored-by: Adam Reeve <adreeve@gmail.com> Signed-off-by: Antoine Pitrou <antoine@python.org>

… for a Parquet column (apache#38025) ### Rationale for this change After the changes in apache#35886, getting the compression level for a Parquet column segfaults if the compression level or other options weren't previously set ### What changes are included in this PR? Adds a null check on the codec options of the column properties before trying to access the compression level. ### Are these changes tested? Yes, I added a unit test. ### Are there any user-facing changes? This fixes a regression added after 13.0.0 so isn't a user-facing fix * Closes: apache#38039 Authored-by: Adam Reeve <adreeve@gmail.com> Signed-off-by: Antoine Pitrou <antoine@python.org>

yyang52 requested review from lidavidm, paleolimbot, thisisnic and wjones127 as code owners June 2, 2023 08:58

github-actions bot added Component: C++ Component: FlightRPC Component: Parquet Component: R awaiting review Awaiting review labels Jun 2, 2023

yyang52 force-pushed the codec_options branch from 0837917 to 07b92c2 Compare June 2, 2023 09:12

mapleFU reviewed Jun 2, 2023

View reviewed changes

cpp/src/parquet/properties.h Outdated Show resolved Hide resolved

github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Jun 2, 2023

yyang52 force-pushed the codec_options branch 2 times, most recently from 1e5c232 to fdfb1e8 Compare June 5, 2023 02:55

mapleFU reviewed Jun 5, 2023

View reviewed changes

wgtmac reviewed Jun 5, 2023

View reviewed changes

yyang52 force-pushed the codec_options branch from a8748d8 to ea0d84c Compare June 6, 2023 09:05

pitrou requested changes Jun 6, 2023

View reviewed changes

github-actions bot removed Component: R Component: FlightRPC labels Jun 8, 2023

yyang52 force-pushed the codec_options branch from 8771d6b to dc7718f Compare June 8, 2023 08:34

yyang52 force-pushed the codec_options branch from 4623eb8 to 369d045 Compare June 8, 2023 08:42

mapleFU reviewed Jun 9, 2023

View reviewed changes

yyang52 force-pushed the codec_options branch from 921d197 to d66e60c Compare June 9, 2023 07:28

add CodecOptions to customize the compression parameter

d66e60c

pitrou reviewed Jul 6, 2023

View reviewed changes

pitrou requested changes Jul 6, 2023

View reviewed changes

mapleFU approved these changes Jul 6, 2023

View reviewed changes

address comments to fix style

8f8e4c5

wgtmac approved these changes Jul 7, 2023

View reviewed changes

yyang52 and others added 4 commits July 10, 2023 15:19

quick fix

7fd9bea

Merge branch 'main' of https://github.com/apache/arrow into codec_opt…

86e025b

…ions

bug fix

b5b9e24

Merge branch 'main' into codec_options

73b4018

pitrou approved these changes Jul 11, 2023

View reviewed changes

pitrou merged commit a9035cb into apache:main Jul 11, 2023

pitrou removed the awaiting committer review Awaiting committer review label Jul 11, 2023

mapleFU reviewed Jul 23, 2023

View reviewed changes

github-actions bot added the awaiting committer review Awaiting committer review label Jul 23, 2023

mapleFU mentioned this pull request Jul 23, 2023

[C++][Parquet] Parquet: buffered column writer uses unbuffered page writer #36828

Closed

adamreeve mentioned this pull request Oct 4, 2023

GH-38039: [C++][Parquet] Fix segfault getting compression level for a Parquet column #38025

Merged

pitrou mentioned this pull request Oct 5, 2023

[C++][Parquet] Segfault getting compression level for a Parquet column #38039

Closed

	return Status::Invalid("window_bits should be within 10 ~ 24");
	return Status::Invalid("window_bits should be between ", BROTLI_MIN_WINDOW_BITS,
	" and ", BROTLI_MAX_WINDOW_BITS);

	int CompressionWindowBitsForFormat(GZipFormat::type format, int input_window_bits) {
	int window_bits = input_window_bits;
	int CompressionWindowBitsForFormat(GZipFormat::type format, int window_bits) {

	ARROW_DEPRECATED("Use the one with CodecOptions instead")
	ARROW_DEPRECATED("Deprecated in 13.0.0. Use CodecOptions-taking overload instead.")

	int compression_level_ = kUseDefaultCompressionLevel;
	int compression_level = kUseDefaultCompressionLevel;

Conversation

yyang52 commented Jun 2, 2023 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

github-actions bot commented Jun 2, 2023

Uh oh!

mapleFU left a comment

Choose a reason for hiding this comment

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

mapleFU commented Jun 5, 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!

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pitrou commented Jun 6, 2023

Uh oh!

yyang52 commented Jun 8, 2023

Uh oh!

mapleFU left a comment

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

yyang52 commented Jun 2, 2023 •

edited by github-actions bot

Loading