Parquet: Do not compress v2 data page when compress is bad quality #8257
Conversation
| cmpr.compress(&values_data.buf, &mut buffer)?; | ||
| if uncompressed_size <= buffer.len() - buffer_len { |
There was a problem hiding this comment.
This can also be a "score", like if uncompressed_size <= (buffer.len() - buffer_len) * 0.9 {
There was a problem hiding this comment.
I'd suggest maybe going the opposite direction. If compression doesn't give say a 10% size reduction, then don't compress...it will be faster to read. 10% might be too much...maybe we could make it configurable.
Anyway, that's probably out of scope for this PR. Let's just go with the simple test for now...it's still an improvement.
There was a problem hiding this comment.
Yeah, I do think that would need to configurable as I think some people would treat a 10% size increase as a regression, even if they got a faster reader
A good follow on ticket for sure
|
This need add some test, I found a |
This is caused by another bug in user side, it's not related to this problem :-( |
mapleFU
force-pushed
the
page-v2-no-compress-if-size-too-lage
branch
from
c35ae85 to
0e198e9
Compare
|
I've added a test for this, would you mind take a look again? |
| cmpr.compress(&values_data.buf, &mut buffer)?; | ||
| if uncompressed_size <= buffer.len() - buffer_len { |
There was a problem hiding this comment.
Yeah, I do think that would need to configurable as I think some people would treat a 10% size increase as a regression, even if they got a faster reader
A good follow on ticket for sure
| get_test_column_writer::<ByteArrayType>(Box::new(page_writer), 0, 0, Arc::new(props)); | ||
|
|
||
| // Create small, simple data that Snappy compression will likely increase in size | ||
| // due to compression overhead for very small data |
Which issue does this PR close?
Rationale for this change
Do not compress v2 data page when compress is bad quality ( compressed size is greater or equal to uncompressed_size )
What changes are included in this PR?
Discard compression when it's too large
Are these changes tested?
Covered by existing
Are there any user-facing changes?
No