[GitHub] [iceberg] singhpk234 commented on a diff in pull request #5035: Add Parquet Row Group Bloom Filter Support for write path

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

singhpk234 commented on code in PR #5035:
URL: https://github.com/apache/iceberg/pull/5035#discussion_r897151281


##########
docs/tables/configuration.md:
##########
@@ -50,6 +50,8 @@ Iceberg tables support table properties to configure table behavior, like the de
 | write.parquet.dict-size-bytes      | 2097152 (2 MB)     | Parquet dictionary page size                       |
 | write.parquet.compression-codec    | gzip               | Parquet compression codec: zstd, brotli, lz4, gzip, snappy, uncompressed |
 | write.parquet.compression-level    | null               | Parquet compression level                          |
+| write.parquet.bloom-filter-enabled.column.col1          | (not set) | Enables writing a bloom filter for the column |

Review Comment:
   [minor] should we say it enables bf for col1 : `Enables writing a bloom filter for the column : col1`
   
   like we did [here](https://github.com/apache/iceberg/pull/5035/files#diff-59125802f6fa3d6d51c716484c3cdff3d85af7c834b8102296b2321b17aec696R64)



##########
parquet/src/main/java/org/apache/iceberg/parquet/Parquet.java:
##########
@@ -293,19 +305,29 @@ public <D> FileAppender<D> build() throws IOException {
             .withRowGroupSize(rowGroupSize)
             .withPageSize(pageSize)
             .withDictionaryPageSize(dictionaryPageSize);
-        // Todo: The following code needs to be improved in the bloom filter write path PR.
-        for (Map.Entry<String, String> entry : config.entrySet()) {
-          String key = entry.getKey();
-          if (key.startsWith(PARQUET_BLOOM_FILTER_COLUMN_ENABLED_PREFIX)) {
-            String columnPath = key.replaceFirst(PARQUET_BLOOM_FILTER_COLUMN_ENABLED_PREFIX, "");
-            String value = entry.getValue();
-            parquetWriteBuilder.withBloomFilterEnabled(columnPath, Boolean.valueOf(value));
-          }
+
+        for (Map.Entry<String, String> entry : columnBloomFilterEnabled.entrySet()) {
+          String colPath = entry.getKey();
+          String bloomEnabled = entry.getValue();
+          parquetWriteBuilder.withBloomFilterEnabled(colPath, Boolean.valueOf(bloomEnabled));
         }
+
         return new ParquetWriteAdapter<>(parquetWriteBuilder.build(), metricsConfig);
       }
     }
 
+    private static Map<String, String> bloomColumnConfigMap(String prefix, Map<String, String> config) {
+      Map<String, String> columnBloomFilterConfig = Maps.newHashMap();
+      config.keySet().stream()
+          .filter(key -> key.startsWith(prefix))
+          .forEach(key -> {
+            String columnPath = key.replaceFirst(prefix, "");
+            String bloomFilterMode = config.get(key);
+            columnBloomFilterConfig.put(columnPath, bloomFilterMode);
+          });
+      return columnBloomFilterConfig;
+    }

Review Comment:
   [minor] can use this util [PropertyUtil#propertiesWithPrefix](https://github.com/apache/iceberg/blob/master/core/src/main/java/org/apache/iceberg/util/PropertyUtil.java#L85-L97)



-- 
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: issues-unsubscribe@iceberg.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org