Expose page encoding ColumnChunkMetadata
#1322
Conversation
There was a problem hiding this comment.
Looks good to me -- thanks for breaking this work up in to small pieces @shanisolomon -- it makes it easier to review and keep the 🚋 rolling!
There appears to be some clippy issue with this PR that needs to be fixed to get a clean CI run, but otherwise 👍
| @@ -467,6 +469,17 @@ impl ColumnChunkMetaData { | |||
| self.statistics.as_ref() | |||
| } | |||
|
|
|||
| /// Returns `true` if this column chunk contains page encoding stats, `false` otherwise. | |||
| pub fn has_page_encoding_stats(&self) -> bool { | |||
There was a problem hiding this comment.
I think this method is redundant with the page_encoding_stats as I think most of the time any client code would process page_encoding_stats they would likely use code like:
if let Some(page_encoding_stats) = metadata.page_encoding_stats() {
// do something with `page_encoding_stats`However, I see it is consistent with the methods of this file (and I didn't call this out in earlier PRs 😅 )
On the other hand, we haven't released the other methods (like has_bloom_filter so it isn't too late to remove them)
There was a problem hiding this comment.
I introduce those methods in order to be consistent with the other has_ methods for optional values in the metadata. Would you like me to remove all?
There was a problem hiding this comment.
Here is my proposal:
- Merge this PR as is (it has some small clippy thing we need to fix to get the CI clean)
- Prepare a PR to remove all the has_XXX methods that have been added for extended metadata so that we don't add them to a released version
- Prepare a PR to remove the other (pre-existing has_XXX methods): Consider removing redundant has_XXX metadata functions in
ColumnChunkMetadata#1332
| @@ -815,6 +858,18 @@ mod tests { | |||
| .set_total_uncompressed_size(3000) | |||
| .set_data_page_offset(4000) | |||
| .set_dictionary_page_offset(Some(5000)) | |||
| .set_page_encoding_stats(vec
alamb
added
the
enhancement
|
Thanks again @shanisolomon |
Which issue does this PR close?
Closes #1321.
What changes are included in this PR?
Introduced page_encoding_stats struct, with to_thrift and from_thrift conversions, and exposed it from the column chunk metadata.