[GitHub] [arrow-rs] tustvold commented on a diff in pull request #3175: Bloom filter config tweaks (#3023)

[GitBox <gi...@apache.org>]

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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org