Repository navigation
GH-47752: [C++][Python][Parquet] Add minimum compression savings for V2 pages - #52580
Conversation
|
|
|
|
1 similar comment
|
|
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.
|
Doc issue appears to be from wrong ordering in
|
|
@MashrafeeAryan this is looking good, some minor suggestions from me, but I think it's ready for another reviewer. Feel free to take it out of draft. @pitrou could we ask you for a review here? |
|
Ping @pitrou :) |
|
I've submitted #55272 to write V2 data pages by default. |
@pitrou that's good. I don't think it changes anthing for this PR? Also, please note @MashrafeeAryan is my intern and will work on fast read/write path for fixed size lists. (As proposed by hardwood). |
No. |
I think lint issue was fixed on main, so rebase should fix it. |
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.
Throw an error when users set min_space_savings for V1 pages. Update C++ and Python tests.
Require compressed data to be smaller, even when min_space_savings is 0.0. Simplify the compression check and update the test.
Check min_space_savings when users set it. Update the test and Python error handling.
Check compressed and original sizes in the existing test. Keep checking that the data reads back correctly.
Explain min_space_savings in the Python and C++ docs.
Fix spacing and line wrapping in the C++ changes.
Allow zero with V1 and reject nonzero values. Combine dataset tests and keep threshold checks in C++. Update docs to match.
Co-authored-by: Antoine Pitrou <pitrou@free.fr>
4dcf26f to
f39dc5e
Compare
I have fixed the lint issue. Thank you so much! |
|
Yes, I think so. Thank you @MashrafeeAryan ! |
|
Merged. Thanks for working on this @MashrafeeAryan ! |
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_compression_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_compression_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_compression_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: