Repository navigation
GH-47752: [C++][Python][Parquet] Add minimum compression savings for V2 pages #52580
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -403,6 +403,7 @@ class PARQUET_EXPORT WriterProperties { | |
| max_rows_per_page_(properties.max_rows_per_page()), | ||
| version_(properties.version()), | ||
| data_page_version_(properties.data_page_version()), | ||
| min_space_savings_(properties.min_space_savings()), | ||
| created_by_(properties.created_by()), | ||
| store_decimal_as_integer_(properties.store_decimal_as_integer()), | ||
| page_checksum_enabled_(properties.page_checksum_enabled()), | ||
|
|
@@ -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) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps we should not allow |
||
| min_space_savings_ = min_space_savings; | ||
| return this; | ||
| } | ||
|
|
||
| /// Specify the Parquet file version. | ||
| /// Default PARQUET_2_6. | ||
| Builder* version(ParquetVersion::type version) { | ||
|
|
@@ -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 | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps we should not allow |
||
| 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]"); | ||
| } | ||
|
Comment on lines
+895
to
+898
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's rather throw in the setter (
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could also |
||
| std::unordered_map<std::string, ColumnProperties> column_properties; | ||
| auto get = [&](const std::string& key) -> ColumnProperties& { | ||
| auto it = column_properties.find(key); | ||
|
|
@@ -918,7 +931,7 @@ class PARQUET_EXPORT WriterProperties { | |
| pagesize_, max_rows_per_page_, version_, created_by_, page_checksum_enabled_, | ||
| size_statistics_level_, std::move(file_encryption_properties_), | ||
| default_column_properties_, column_properties, data_page_version_, | ||
| store_decimal_as_integer_, std::move(sorting_columns_), | ||
| min_space_savings_, store_decimal_as_integer_, std::move(sorting_columns_), | ||
| content_defined_chunking_enabled_, content_defined_chunking_options_)); | ||
| } | ||
|
|
||
|
|
@@ -933,6 +946,7 @@ class PARQUET_EXPORT WriterProperties { | |
| int64_t max_rows_per_page_; | ||
| ParquetVersion::type version_; | ||
| ParquetDataPageVersion data_page_version_; | ||
| std::optional<double> min_space_savings_ = {}; | ||
| std::string created_by_; | ||
| bool store_decimal_as_integer_; | ||
| bool page_checksum_enabled_; | ||
|
|
@@ -973,6 +987,9 @@ class PARQUET_EXPORT WriterProperties { | |
| return parquet_data_page_version_; | ||
| } | ||
|
|
||
|
|
||
| std::optional<double> min_space_savings() const { return min_space_savings_; } | ||
|
|
||
| inline ParquetVersion::type version() const { return parquet_version_; } | ||
|
|
||
| inline std::string created_by() const { return parquet_created_by_; } | ||
|
|
@@ -1100,7 +1117,8 @@ class PARQUET_EXPORT WriterProperties { | |
| std::shared_ptr<FileEncryptionProperties> file_encryption_properties, | ||
| const ColumnProperties& default_column_properties, | ||
| const std::unordered_map<std::string, ColumnProperties>& column_properties, | ||
| ParquetDataPageVersion data_page_version, bool store_short_decimal_as_integer, | ||
| ParquetDataPageVersion data_page_version, std::optional<double> min_space_savings, | ||
| bool store_short_decimal_as_integer, | ||
| std::vector<SortingColumn> sorting_columns, bool content_defined_chunking_enabled, | ||
| CdcOptions content_defined_chunking_options) | ||
| : pool_(pool), | ||
|
|
@@ -1110,6 +1128,7 @@ class PARQUET_EXPORT WriterProperties { | |
| pagesize_(pagesize), | ||
| max_rows_per_page_(max_rows_per_page), | ||
| parquet_data_page_version_(data_page_version), | ||
| min_space_savings_(min_space_savings), | ||
| parquet_version_(version), | ||
| parquet_created_by_(created_by), | ||
| store_decimal_as_integer_(store_short_decimal_as_integer), | ||
|
|
@@ -1129,6 +1148,7 @@ class PARQUET_EXPORT WriterProperties { | |
| int64_t pagesize_; | ||
| int64_t max_rows_per_page_; | ||
| ParquetDataPageVersion parquet_data_page_version_; | ||
| std::optional<double> min_space_savings_; | ||
| ParquetVersion::type parquet_version_; | ||
| std::string parquet_created_by_; | ||
| bool store_decimal_as_integer_; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -41,6 +41,50 @@ | |
| pytestmark = pytest.mark.parquet | ||
|
|
||
|
|
||
| @pytest.mark.parametrize("minimum", [None, 0.0, 0.1, 1.0]) | ||
| @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( | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This test only checks round-trip equality, so it would still pass if 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 |
||
| minimum, page_version, compression, use_writer): | ||
| table = pa.table({ | ||
| "value": pa.array([0.0] * 1000, type=pa.float32()) | ||
| }) | ||
| sink = pa.BufferOutputStream() | ||
| options = dict( | ||
| compression=compression, | ||
| use_dictionary=False, | ||
| data_page_version=page_version, | ||
| min_space_savings=minimum, | ||
| ) | ||
|
|
||
| if use_writer: | ||
| with pq.ParquetWriter(sink, table.schema, **options) as writer: | ||
| writer.write_table(table) | ||
| else: | ||
| pq.write_table(table, sink, **options) | ||
|
|
||
| assert pq.read_table(sink.getvalue()).equals(table) | ||
|
|
||
| # Tests invalid settings | ||
| @pytest.mark.parametrize( | ||
| "minimum", | ||
| [-0.1, 1.1, float("inf"), -float("inf"), float("nan")] | ||
| ) | ||
| @pytest.mark.parametrize("use_writer", [False, True]) | ||
| def test_invalid_min_space_savings(minimum, use_writer): | ||
| table = pa.table({"value": [1.0]}) | ||
| sink = pa.BufferOutputStream() | ||
|
|
||
| with pytest.raises(ValueError, match="min_space_savings"): | ||
| if use_writer: | ||
| pq.ParquetWriter( | ||
| sink, table.schema, min_space_savings=minimum) | ||
| else: | ||
| pq.write_table( | ||
| table, sink, min_space_savings=minimum) | ||
|
|
||
|
|
||
| @pytest.mark.pandas | ||
| def test_parquet_incremental_file_build(tempdir): | ||
| df = _test_dataframe(100) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With
min_space_savings = 0.0,savings >= *minimummarks a page as compressed even when compressed size equals the original.min_space_savings = 0.0would 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: