rok commented on code in PR #52580:
URL: https://github.com/apache/arrow/pull/52580#discussion_r4300059386
##########
cpp/src/parquet/column_writer.cc:
##########
@@ -1057,8 +1057,17 @@ 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;
+
+ if (const auto minimum = properties_->min_space_savings()) {
+
+ //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;
Review 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:
```cpp
const int64_t compressed = compressor_temp_buffer_->size();
page_is_compressed =
compressed < values->size() &&
(!minimum || 1.0 - static_cast<double>(compressed) / values->size() >=
*minimum);
```
##########
cpp/src/parquet/properties.h:
##########
@@ -883,6 +891,11 @@ class PARQUET_EXPORT WriterProperties {
/// \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
+ 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]");
+ }
Review Comment:
Let's rather throw in the setter (`Builder*
min_space_savings(std::optional<double> min_space_savings)`)?
##########
cpp/src/parquet/properties.h:
##########
@@ -883,6 +891,11 @@ class PARQUET_EXPORT WriterProperties {
/// \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
Review Comment:
Perhaps we should not allow `min_space_savings_` is set while
`data_page_version_` is v1?
##########
cpp/src/parquet/properties.h:
##########
@@ -529,6 +530,13 @@ class PARQUET_EXPORT WriterProperties {
return this;
}
+ /// 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) {
Review Comment:
Perhaps we should not allow `min_space_savings_` is set while
`data_page_version_` is v1?
##########
cpp/src/parquet/properties.h:
##########
@@ -883,6 +891,11 @@ class PARQUET_EXPORT WriterProperties {
/// \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
+ 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]");
+ }
Review Comment:
We could also `std::clamp(value, 0.0, 1.0)`, but throwing is probably better.
##########
python/pyarrow/tests/parquet/test_parquet_writer.py:
##########
@@ -41,6 +41,50 @@
pytestmark = pytest.mark.parquet
[email protected]("minimum", [None, 0.0, 0.1, 1.0])
[email protected]("page_version", ["1.0", "2.0"])
[email protected]("compression", ["none", "snappy"])
[email protected]("use_writer", [False, True])
+def test_min_space_savings(
Review 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:
```python
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_size
```
This also makes the V1 "no effect" behavior explicit rather than leaving
`page_version="1.0"` parametrized without an assertion.
--
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]
