-
Notifications
You must be signed in to change notification settings - Fork 807
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add ColumnChunkMetadataBuilder
clear APIs
#6523
Conversation
7b7a912
to
02d8b98
Compare
@@ -1315,12 +1334,24 @@ impl ColumnChunkMetaDataBuilder { | |||
self | |||
} | |||
|
|||
/// Clears the statistics for this column chunk. | |||
pub fn clear_statistics(mut self) -> Self { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, set_statistics
takes value: Statistics
not value: Option<Statistics>
and thus there is no way to make this change without breaking the API which we can't do until the next breaking release
/// Sets page encoding stats for this column chunk. | ||
pub fn set_page_encoding_stats(mut self, value: Vec<PageEncodingStats>) -> Self { | ||
self.0.encoding_stats = Some(value); | ||
self | ||
} | ||
|
||
/// Clears the page encoding stats for this column chunk. | ||
pub fn clear_page_encoding_stats(mut self) -> Self { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same reasoning applies to this API
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Seems straightforward enough to not need additional tests. I agree it would be nice to change the setters to take Option
s. That can be done in a follow-up. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like a pragmatic way forward to me
Which issue does this PR close?
Part of #6504
Rationale for this change
I want to use the
ColumnChunkMetaDataBuilder
to modify existing metadata in addition to creating new metadata, for the example in #6504.What changes are included in this PR?
ColumnChunkMetadataBuilder::clear_statistics
and other clear API to modifyOption
fieldsAre there any user-facing changes?
Some new APIs and docs