GH-3344: Adaptive compression for v2 page#3368
Conversation
| definitionLevels, | ||
| data, | ||
| statistics, | ||
| 10); |
There was a problem hiding this comment.
It does not make sense to have a threshold greater than 1.0, does it?
In any case, the data of a column full of zeros is extremely compressible (you might want to disable dictionary encoding?), so the default value should work IMHO.
| assertEquals( | ||
| "Data should be stored uncompressed when compression ratio exceeds threshold", | ||
| uncompressedSize, | ||
| compressedSize); |
There was a problem hiding this comment.
Perhaps you want to read the data back and check it's as expected?
| * @param threshold the compression ratio threshold, default is {@value #DEFAULT_V2_PAGE_COMPRESS_THRESHOLD} | ||
| * @return this builder for method chaining | ||
| */ | ||
| public Builder withV2PageCompressThreshold(double threshold) { |
There was a problem hiding this comment.
I'm not sure it makes sense to put "V2" in the API name? For users it doesn't seem be relevant (just keep it mentioned in the docstrings).
There was a problem hiding this comment.
Good point, updated
| converter.getEncoding(headerV2.getEncoding()), | ||
| BytesInput.from(pageLoad), | ||
| headerV2.is_compressed, | ||
| compressed, |
There was a problem hiding this comment.
Fixed a potential bug
| public static final boolean DEFAULT_SIZE_STATISTICS_ENABLED = true; | ||
|
|
||
| public static final boolean DEFAULT_PAGE_WRITE_CHECKSUM_ENABLED = true; | ||
| public static final double DEFAULT_PAGE_COMPRESS_THRESHOLD = 0.98; |
There was a problem hiding this comment.
Why is this magic number? Is it better to use a smaller number like 0.9 or 0.85?
There was a problem hiding this comment.
Per the suggestion linked here: #3344 (comment)
| } | ||
| } | ||
|
|
||
| public static Builder build(BytesInputCompressor compressor, MessageType schema, ByteBufferAllocator allocator) { |
There was a problem hiding this comment.
Is it better to use a separate issue to implement this, though I'm fine to keep it as is.
There was a problem hiding this comment.
Now, the ColumnChunkPageWriteStore implements a similar builder pattern. Just add a new option method.
|
This pull request has been automatically marked as stale because it has had no activity for at least 2 months. If you are still working on this change or plan to move it forward, please leave a comment or push a new commit so we know to keep it open. Otherwise, this PR will be closed automatically in about one month. Thank you for your contribution to Apache Parquet! |
ConeyLiu
force-pushed
the
compress-or-not
branch
from
09e72a0 to
95a5307
Compare
|
Sorry this PR hasn’t been updated for so long due to work. Please review it when you get a chance. @wgtmac |
| compressed = true; | ||
| double compressionRatio = (double) compressedData.size() / data.size(); | ||
| if (compressor.getCodecName() != CompressionCodecName.UNCOMPRESSED | ||
| && compressionRatio > pageCompressThreshold) { |
There was a problem hiding this comment.
This fallback can break for one-shot BytesInput implementations. compressor.compress(data) may consume data (for example stream-backed input), and then the fallback assigns the already-consumed input back to compressedData for CRC/writing. Please materialize/copy the original page bytes before compression, or otherwise make the fallback use a replayable copy. (Reviewed by Codex)
There was a problem hiding this comment.
Materialized data before compression.
| * @param threshold the compression ratio threshold, default is {@value #DEFAULT_PAGE_COMPRESS_THRESHOLD} | ||
| * @return this builder for method chaining | ||
| */ | ||
| public Builder withPageCompressThreshold(double threshold) { |
There was a problem hiding this comment.
Please validate the threshold or document the allowed range here. As written, values like NaN, negative numbers, or 100 are accepted even though the API describes this as a compressed/uncompressed ratio threshold. I would either enforce a finite range (for example 0.0 <= threshold <= 1.0) or explicitly document the special meanings of out-of-range values. (Reviewed by Codex)
ConeyLiu
force-pushed
the
compress-or-not
branch
from
4e4f191 to
0f87e32
Compare
3 participants
Rationale for this change
Closes #3344
What changes are included in this PR?
Are these changes tested?
Yes, added UT.
Are there any user-facing changes?
Yes, add new configuration. No breaking changes.