You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "mapleFU (via GitHub)" <gi...@apache.org> on 2025/09/01 06:51:24 UTC
[PR] Parquet: Do not compress v2 data page when compress is bad quality [arrow-rs]
mapleFU opened a new pull request, #8257:
URL: https://github.com/apache/arrow-rs/pull/8257
# Which issue does this PR close?
- Closes #8256 .
# 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
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Parquet: Do not compress v2 data page when compress is bad quality [arrow-rs]
Posted by "JigaoLuo (via GitHub)" <gi...@apache.org>.
JigaoLuo commented on PR #8257:
URL: https://github.com/apache/arrow-rs/pull/8257#issuecomment-3297847281
Thanks. I will continue in #8358
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Parquet: Do not compress v2 data page when compress is bad quality [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #8257:
URL: https://github.com/apache/arrow-rs/pull/8257
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Parquet: Do not compress v2 data page when compress is bad quality [arrow-rs]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on PR #8257:
URL: https://github.com/apache/arrow-rs/pull/8257#issuecomment-3241960162
This need add some test, I found a `"missing required field PageHeader.uncompressed_page_size"` bug here
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Parquet: Do not compress v2 data page when compress is bad quality [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #8257:
URL: https://github.com/apache/arrow-rs/pull/8257#discussion_r2351939223
##########
parquet/src/column/writer/mod.rs:
##########
@@ -1104,12 +1104,23 @@ impl<'a, E: ColumnValueEncoder> GenericColumnWriter<'a, E> {
rep_levels_byte_len + def_levels_byte_len + values_data.buf.len();
// Data Page v2 compresses values only.
- match self.compressor {
+ let is_compressed = match self.compressor {
Some(ref mut cmpr) => {
+ let buffer_len = buffer.len();
cmpr.compress(&values_data.buf, &mut buffer)?;
+ if uncompressed_size <= buffer.len() - buffer_len {
Review Comment:
In my opinion, given there is a tradeoff here (file size / decode speed) we can't hard code some heuristic that changes the default behavior.
However, I think it would be a good improvement to add some sort of tuning knob for people who wanted to make that tradeoff more explicitly as long as the default setting of the knob leaves the existing behavior
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Parquet: Do not compress v2 data page when compress is bad quality [arrow-rs]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on PR #8257:
URL: https://github.com/apache/arrow-rs/pull/8257#issuecomment-3243571369
> This need add some test, I found a "missing required field PageHeader.uncompressed_page_size" bug here
This is caused by another bug in user side, it's not related to this problem :-(
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Parquet: Do not compress v2 data page when compress is bad quality [arrow-rs]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on PR #8257:
URL: https://github.com/apache/arrow-rs/pull/8257#issuecomment-3258608832
I've added a test for this, would you mind take a look again?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Parquet: Do not compress v2 data page when compress is bad quality [arrow-rs]
Posted by "etseidl (via GitHub)" <gi...@apache.org>.
etseidl commented on code in PR #8257:
URL: https://github.com/apache/arrow-rs/pull/8257#discussion_r2322757407
##########
parquet/src/column/writer/mod.rs:
##########
@@ -1104,12 +1104,23 @@ impl<'a, E: ColumnValueEncoder> GenericColumnWriter<'a, E> {
rep_levels_byte_len + def_levels_byte_len + values_data.buf.len();
// Data Page v2 compresses values only.
- match self.compressor {
+ let is_compressed = match self.compressor {
Some(ref mut cmpr) => {
+ let buffer_len = buffer.len();
cmpr.compress(&values_data.buf, &mut buffer)?;
+ if uncompressed_size <= buffer.len() - buffer_len {
Review 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.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Parquet: Do not compress v2 data page when compress is bad quality [arrow-rs]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on PR #8257:
URL: https://github.com/apache/arrow-rs/pull/8257#issuecomment-3266991890
Generally different level of data would have different distribution, and like what query-optimizer meets, data changes ( like frequently insertion or insert overwrite ) might need to re-sampling the data. So I may think runtime config would be different from others
And z-ordering clustering or other cluserting might also changes the distribution score. So currently I may think: a user config can set the own score, maybe different score for just ingested data (which might need fast write) or well clustered data ( which might need well compressed ). 10% is a good intuition but it's hard to define it's good. When compressed size > uncompressed size it's 100% worse.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Parquet: Do not compress v2 data page when compress is bad quality [arrow-rs]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on code in PR #8257:
URL: https://github.com/apache/arrow-rs/pull/8257#discussion_r2313061274
##########
parquet/src/column/writer/mod.rs:
##########
@@ -1104,12 +1104,23 @@ impl<'a, E: ColumnValueEncoder> GenericColumnWriter<'a, E> {
rep_levels_byte_len + def_levels_byte_len + values_data.buf.len();
// Data Page v2 compresses values only.
- match self.compressor {
+ let is_compressed = match self.compressor {
Some(ref mut cmpr) => {
+ let buffer_len = buffer.len();
cmpr.compress(&values_data.buf, &mut buffer)?;
+ if uncompressed_size <= buffer.len() - buffer_len {
Review Comment:
This can also be a "score", like `if uncompressed_size <= (buffer.len() - buffer_len) * 0.9 {`
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Parquet: Do not compress v2 data page when compress is bad quality [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #8257:
URL: https://github.com/apache/arrow-rs/pull/8257#issuecomment-3265735894
Thanks again @mapleFU and @etseidl
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Parquet: Do not compress v2 data page when compress is bad quality [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #8257:
URL: https://github.com/apache/arrow-rs/pull/8257#issuecomment-3297735001
> I agree. I’ve been thinking more about this, especially since my focus is primarily on cuDF rather than DataFusion.
>
> At a high level, it’s a trade-off between computation (specifically decompression) and I/O (file size reduction). In CPU scenarios like datafusion, I believe reading compressed Parquet files tends to be computation-bound
Yes I 100% agree with this conclusion -- and I think allowing users to have more control over this tradeoff via a tuning knob would make a lot of sense.
I filed a ticket to try and capture the discussion
- https://github.com/apache/arrow-rs/issues/8358
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Parquet: Do not compress v2 data page when compress is bad quality [arrow-rs]
Posted by "JigaoLuo (via GitHub)" <gi...@apache.org>.
JigaoLuo commented on code in PR #8257:
URL: https://github.com/apache/arrow-rs/pull/8257#discussion_r2333288803
##########
parquet/src/column/writer/mod.rs:
##########
@@ -1104,12 +1104,23 @@ impl<'a, E: ColumnValueEncoder> GenericColumnWriter<'a, E> {
rep_levels_byte_len + def_levels_byte_len + values_data.buf.len();
// Data Page v2 compresses values only.
- match self.compressor {
+ let is_compressed = match self.compressor {
Some(ref mut cmpr) => {
+ let buffer_len = buffer.len();
cmpr.compress(&values_data.buf, &mut buffer)?;
+ if uncompressed_size <= buffer.len() - buffer_len {
Review Comment:
I’ll follow up once there’s a plan for this future ticket. Just a note from me —the score is still a highly empirical value (and depends on the decompression speed), so I’ve embedded it into my rewriter and have been extensively rewriting Parquet files based on it.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Parquet: Do not compress v2 data page when compress is bad quality [arrow-rs]
Posted by "JigaoLuo (via GitHub)" <gi...@apache.org>.
JigaoLuo commented on PR #8257:
URL: https://github.com/apache/arrow-rs/pull/8257#issuecomment-3266966160
Hello everyone,
I just came across this PR and noticed that most of the discussion is happening here, so I’d like to continue the conversation in this thread rather than on the issue page.
I believe the direction of this PR aligns well with a previous issue we discussed in https://github.com/XiangpengHao/liquid-cache/issues/227. I’ve been working on my own `parquet-rewrite` tool that touches on similar ideas, particularly with the **score** metric—a kind of breakeven point to decide whether compression should be applied. The goal of this tool is to help the reader skip unnecessary compression that adds overhead without delivering meaningful size reduction, ultimately improving the reader's reading performance.
Setting this **score** is quite tricky and empirical. For now, I’ve set it at 10%, mainly to catch cases where compression offers no size benefit at all. Here is an example:
<img width="2415" height="660" alt="image" src="https://github.com/user-attachments/assets/0ab7438c-6516-46b0-bc17-e9c8b9b14273" />
---
As a side note, I’ve also made some patches to Xiangpeng’s viewer tool, which I use to inspect my generated Parquet files. This has been instrumental in iterating on my reader implementation.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Parquet: Do not compress v2 data page when compress is bad quality [arrow-rs]
Posted by "JigaoLuo (via GitHub)" <gi...@apache.org>.
JigaoLuo commented on PR #8257:
URL: https://github.com/apache/arrow-rs/pull/8257#issuecomment-3267224535
I agree. I’ve been thinking more about this, especially since my focus is primarily on cuDF rather than DataFusion.
At a high level, it’s a trade-off between computation (specifically decompression) and I/O (file size reduction). In CPU scenarios like datafusion, I believe reading compressed Parquet files tends to be computation-bound
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Parquet: Do not compress v2 data page when compress is bad quality [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #8257:
URL: https://github.com/apache/arrow-rs/pull/8257#discussion_r2326067523
##########
parquet/src/column/writer/mod.rs:
##########
@@ -4236,4 +4247,33 @@ mod tests {
.unwrap();
ColumnDescriptor::new(Arc::new(tpe), max_def_level, max_rep_level, path)
}
+
+ #[test]
+ fn test_page_v2_snappy_compression_fallback() {
+ // Test that PageV2 sets is_compressed to false when Snappy compression increases data size
+ let page_writer = TestPageWriter {};
+
+ // Create WriterProperties with PageV2 and Snappy compression
+ let props = WriterProperties::builder()
+ .set_writer_version(WriterVersion::PARQUET_2_0)
+ // Disable dictionary to ensure data is written directly
+ .set_dictionary_enabled(false)
+ .set_compression(Compression::SNAPPY)
+ .build();
+
+ let mut column_writer =
+ 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
Review Comment:
👍
##########
parquet/src/column/writer/mod.rs:
##########
@@ -1104,12 +1104,23 @@ impl<'a, E: ColumnValueEncoder> GenericColumnWriter<'a, E> {
rep_levels_byte_len + def_levels_byte_len + values_data.buf.len();
// Data Page v2 compresses values only.
- match self.compressor {
+ let is_compressed = match self.compressor {
Some(ref mut cmpr) => {
+ let buffer_len = buffer.len();
cmpr.compress(&values_data.buf, &mut buffer)?;
+ if uncompressed_size <= buffer.len() - buffer_len {
Review 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 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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org