pitrou commented on code in PR #52580:
URL: https://github.com/apache/arrow/pull/52580#discussion_r4386922848
##########
docs/source/cpp/parquet.rst:
##########
@@ -271,6 +271,13 @@ with ``data_page_version``. It's V1 by default; setting to
V2 will allow more
optimal compression (skipping compressing pages where there isn't a space
benefit), but not all readers support this data page version.
+For V2 pages, use ``min_compression_savings`` to set how much space
compression must
+save. Use a value from 0.0 to 1.0. For example,
``min_compression_savings(0.1)`` requires
+at least 10% savings. If the space savings do not meet the threshold, it is not
+compressed. With the default ``0.0``, compression is kept as long as compressed
+values are smaller than original values. Setting a nonzero value with V1 or
+using a number outside the allowed range throws ``ParquetException``.
Review Comment:
Just a wording improvement
```suggestion
at least 10% savings. If the space savings for a page do not meet the
threshold,
it is not compressed. With the default ``0.0``, compression is kept as long
as
compressed values are smaller than original values. Setting a nonzero value
with
V1 or using a number outside the allowed range throws ``ParquetException``.
```
##########
cpp/src/parquet/column_writer.cc:
##########
@@ -1057,9 +1057,12 @@ void ColumnWriterImpl::BuildDataPageV2(int64_t
definition_levels_rle_size,
bool page_is_compressed = false;
if (pager_->has_compressor() && values->size() > 0) {
pager_->Compress(*values, compressor_temp_buffer_.get());
- if (compressor_temp_buffer_->size() < values->size()) {
- page_is_compressed = true;
- }
+
+ const int64_t compressed = compressor_temp_buffer_->size();
+ const auto minimum = properties_->min_compression_savings();
+ page_is_compressed =
+ compressed < values->size() &&
+ 1.0 - static_cast<double>(compressed) / values->size() >= minimum;
Review Comment:
I think the `compressed < values->size()` has become superfluous now. If
`compressed` is >= `values->size()` then the compression savings check will
fail.
```suggestion
page_is_compressed =
(1.0 - static_cast<double>(compressed) / values->size()) >= minimum;
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
