tustvold commented on code in PR #3175:
URL: https://github.com/apache/arrow-rs/pull/3175#discussion_r1031405061
##########
parquet/src/column/writer/encoder.rs:
##########
@@ -194,17 +194,10 @@ impl<T: DataType> ColumnValueEncoder for
ColumnValueEncoderImpl<T> {
let statistics_enabled = props.statistics_enabled(descr.path());
- let bloom_filter_enabled = props.bloom_filter_enabled(descr.path());
- let bloom_filter =
- if let Some(BloomFilterProperties { ndv, fpp }) =
bloom_filter_enabled {
- Sbbf::new_with_ndv_fpp(ndv, fpp)
- .map_err(|e| {
- eprintln!("invalid bloom filter properties: {}", e);
- })
- .ok()
- } else {
- None
- };
+ let bloom_filter = props
+ .bloom_filter_properties(descr.path())
+ .map(|props| Sbbf::new_with_ndv_fpp(*ndv, *fpp))
+ .transpose()?;
Review Comment:
It is better to propagate the error than printing it
##########
parquet/src/file/properties.rs:
##########
@@ -672,40 +673,37 @@ impl ColumnProperties {
/// otherwise it is a no-op.
/// If `value` is `false`, resets bloom filter properties to `None`.
fn set_bloom_filter_enabled(&mut self, value: bool) {
- if value {
- self.bloom_filter_enabled = self
- .bloom_filter_enabled()
- .or_else(|| Some(Default::default()));
- } else {
- self.bloom_filter_enabled = None;
+ if value && self.bloom_filter_properies.is_none() {
+ self.bloom_filter_properies = Some(Default::default())
+ } else if !value {
+ self.bloom_filter_properies = None
}
}
/// Sets the false positive probability for bloom filter for this column,
and implicitly enables
- /// bloom filter if not previously enabled. If the `value` is not between
0 and 1 exclusive, it is
- /// discarded as no-op.
+ /// bloom filter if not previously enabled.
+ ///
+ /// # Panics
Review Comment:
Better IMO to fail loud than silently
##########
parquet/src/file/properties.rs:
##########
@@ -260,16 +260,17 @@ impl WriterProperties {
.unwrap_or(DEFAULT_MAX_STATISTICS_SIZE)
}
- /// Returns whether bloom filter is enabled for a given column. Bloom
filter can be enabled over
- /// all or for a specific column, and is by default set to be disabled.
- pub fn bloom_filter_enabled(
+ /// Returns the [`BloomFilterProperties`] for the given column
+ ///
+ /// Returns `None` if bloom filter is disabled
+ pub fn bloom_filter_properties(
&self,
col: &ColumnPath,
- ) -> Option<BloomFilterProperties> {
+ ) -> Option<&BloomFilterProperties> {
Review Comment:
Generally it is better to return a reference so that callers can clone only
if necessary
##########
parquet/src/file/properties.rs:
##########
@@ -260,16 +260,17 @@ impl WriterProperties {
.unwrap_or(DEFAULT_MAX_STATISTICS_SIZE)
}
- /// Returns whether bloom filter is enabled for a given column. Bloom
filter can be enabled over
- /// all or for a specific column, and is by default set to be disabled.
- pub fn bloom_filter_enabled(
+ /// Returns the [`BloomFilterProperties`] for the given column
+ ///
+ /// Returns `None` if bloom filter is disabled
+ pub fn bloom_filter_properties(
Review Comment:
Given this returns `BloomFilterProperties` and not a boolean, I thought this
name was more appropriate
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
