Repository navigation
GH-47752: [C++][Python][Parquet] Add minimum compression savings for V2 pages - #52580
MashrafeeAryan wants to merge 3 commits into
Conversation
|
|
github-actions
Bot
added
Component: Parquet
Component: C++
Component: Python
awaiting review
|
|
Add min_space_savings to control how much space compression must save. V2 pages below the threshold are written uncompressed. Keep the default behavior and V1 pages unchanged.
Add an option to choose how much space compression must save when writing Parquet files from Python. Add tests as well.
Dataset writing ignored min_space_savings. Move the argument to the end of the call so it reaches the writer. Add a test to check that the setting changes the file size.
|
|
65825cc to
89bf1b2
Compare
There was a problem hiding this comment.
This looks great @MashrafeeAryan!
I left some inline comments, noting major.
Besides addressing the linter errors, we should update the Python docs. A new option is added to write_table, ParquetWriter, write_to_dataset and ParquetFileWriteOptions. We should update _parquet_writer_arg_docs in python/pyarrow/parquet/core.py and the data-page-V2 paragraph in docs/source/cpp/parquet.rst.
| /// \brief Build the WriterProperties with the builder parameters. | ||
| /// \return The WriterProperties defined by the builder. | ||
| std::shared_ptr<WriterProperties> build() { | ||
| // checks for invalid input of min_space_savings |
There was a problem hiding this comment.
Perhaps we should not allow min_space_savings_ is set while data_page_version_ is v1?
|
|
||
| /// Used to set minimum threshold for V2 page data values compression | ||
| /// If min_space_savings unset, keep compression if compressed value is smaller. | ||
| Builder* min_space_savings(std::optional<double> min_space_savings) { |
There was a problem hiding this comment.
Perhaps we should not allow min_space_savings_ is set while data_page_version_ is v1?
| //Checks if compressed size meets the minimum threshold | ||
| const double savings = | ||
| 1.0 - static_cast<double>(compressor_temp_buffer_->size()) / values->size(); | ||
| page_is_compressed = savings >= *minimum; |
There was a problem hiding this comment.
With min_space_savings = 0.0, savings >= *minimum marks a page as compressed even when compressed size equals the original. min_space_savings = 0.0 would emit compressed pages with zero benefit, which would mean we'd need to decompress them even though we wouldn't benefit from compression.
Let's rather do something like this and also avoid if branches:
const int64_t compressed = compressor_temp_buffer_->size();
page_is_compressed =
compressed < values->size() &&
(!minimum || 1.0 - static_cast<double>(compressed) / values->size() >= *minimum);| if (min_space_savings_ && | ||
| !(*min_space_savings_ >= 0.0 && *min_space_savings_ <= 1.0)) { | ||
| throw ParquetException("min_space_savings must be in range [0, 1]"); | ||
| } |
There was a problem hiding this comment.
Let's rather throw in the setter (Builder* min_space_savings(std::optional<double> min_space_savings))?
There was a problem hiding this comment.
We could also std::clamp(value, 0.0, 1.0), but throwing is probably better.
| @pytest.mark.parametrize("page_version", ["1.0", "2.0"]) | ||
| @pytest.mark.parametrize("compression", ["none", "snappy"]) | ||
| @pytest.mark.parametrize("use_writer", [False, True]) | ||
| def test_min_space_savings( |
There was a problem hiding this comment.
This test only checks round-trip equality, so it would still pass if min_space_savings were silently dropped on either the write_table or ParquetWriter path. Since the input is all zeros, the expected outcome is fully determined by the parameters. Something like:
column = pq.ParquetFile(buf).metadata.row_group(0).column(0)
expect_uncompressed = (
compression == "none"
or (page_version == "2.0" and minimum == 1.0)
)
if expect_uncompressed:
assert column.total_compressed_size == column.total_uncompressed_size
else:
assert column.total_compressed_size < column.total_uncompressed_sizeThis also makes the V1 "no effect" behavior explicit rather than leaving page_version="1.0" parametrized without an assertion.
2 participants
Fixes #47752.
Rationale for this change
Parquet's V2 pages currently keep compression whenever the compressed values are smaller than the original values. Even if it saves very little space, it still does it. For instance, it can compress 100 KB to 99 KB and save it. Even though that's 1% storage efficient, it might not be worth the decompression overhead while reading the file.
What changes are included in this PR?
In the PR, I added an optional
min_space_savingssetting between0.0and1.0. It gives users an option to select the minimum threshold to compress V2 page data values. For instance, if the original file is100 KBand the user hasmin_space_savings = 0.1, it means the compressed file needs to save at least 10% space. As a result, unless the file is<= 90 KB, it won't get saved.I keep the current behavior when the user does not set
min_space_savings.Added C++ and Python implementation.
Are these changes tested?
Are there any user-facing changes?
C++ and Python users can set
min_space_savingswhen writing V2 pages. For example,0.1requires at least 10% space savings before keeping compression.Was AI used for this PR?
ChatGPT helped to set up the build for PyArrow.
Codex helped find a bug in dataset_writing which ignored min_space_savings.
PR code and description written by:
Reviewed before submission by: