Support parquet bloom filter length #4885
Conversation
Signed-off-by: Letian Jiang <letian.jiang@outlook.com>
letian-jiang
force-pushed
the
bloom-filter-length
branch
from
351e1a9
to
1f04ca9
Compare
| impl Default for Statistics { | ||
| fn default() -> Self { | ||
| Statistics{ | ||
| max: Some(Vec::new()), |
There was a problem hiding this comment.
It occurs to me that this is technically a breaking change
There was a problem hiding this comment.
actually this is replaced by #[derive(Default)] at line 641
There was a problem hiding this comment.
if I didn't misunderstand it
There was a problem hiding this comment.
But that will default the fields to None
There was a problem hiding this comment.
You are right. It is a breaking change.
parquet/src/file/writer.rs
Outdated
| column_chunk | ||
| .meta_data | ||
| .as_mut() | ||
| .expect("can't have bloom filter without column metadata") | ||
| .bloom_filter_offset = Some(start_offset as i64); | ||
| column_chunk | ||
| .meta_data | ||
| .as_mut() | ||
| .expect("can't have bloom filter without column metadata") | ||
| .bloom_filter_length = | ||
| Some((end_offset - start_offset) as i32) |
There was a problem hiding this comment.
| column_chunk | |
| .meta_data | |
| .as_mut() | |
| .expect("can't have bloom filter without column metadata") | |
| .bloom_filter_offset = Some(start_offset as i64); | |
| column_chunk | |
| .meta_data | |
| .as_mut() | |
| .expect("can't have bloom filter without column metadata") | |
| .bloom_filter_length = | |
| Some((end_offset - start_offset) as i32) | |
| let meta = column_chunk.meta_data.as_mut().expect("can't have bloom filter without column metadata"); | |
| meta.bloom_filter_offset = Some(start_offset as i64); | |
| meta.bloom_filter_length = Some((end_offset - start_offset) as i32); |
Or something to that effect
| let length: usize = header.num_bytes.try_into().map_err(|_| { | ||
| ParquetError::General("Bloom filter length is invalid".to_string()) | ||
| })?; |
There was a problem hiding this comment.
We appear to have lost this check
|
@tustvold Hi, I have made some updates. Could you please take another look? |
|
Since 2.10 is not released, can bloom filter length be used now? 🤔 |
|
I hadn't realised it wasn't released, but it's a fairly innocuous change so I'm inclined to think this is fine |
|
Nice! This is helpful but I think currently parquet 2.10 is not released, so user might not able to write this... |
|
🤔 I think this doesn't harm as long as old readers can read new files (w. bloom filter length) correctly. |
Thrift supports this form of forwards compatibility natively |
Which issue does this PR close?
Closes #4398.
Rationale for this change
What changes are included in this PR?
If
bloom_filter_lengthis available inColumnMetaData, use a single IO to fetch both header and bitset.Are there any user-facing changes?