You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "raghav-reglobe (via GitHub)" <gi...@apache.org> on 2026/05/16 05:50:52 UTC
[PR] Parquet: add adaptive bloom filter sizing (PARQUET-2326) [iceberg]
raghav-reglobe opened a new pull request, #16363:
URL: https://github.com/apache/iceberg/pull/16363
## What changes are proposed in this pull request?
Add a new table property to enable parquet-mr's adaptive bloom filter sizing
([PARQUET-2326](https://issues.apache.org/jira/browse/PARQUET-2326)) for Iceberg-managed Parquet writes:
- `write.parquet.bloom-filter-adaptive-enabled` (boolean, default `false`)
When enabled, parquet-mr's `ColumnValueCollector.initBloomFilter()` constructs an
`AdaptiveBlockSplitBloomFilter` instead of `BlockSplitBloomFilter`. The adaptive variant evaluates
N candidate filter sizes and picks the smallest that satisfies actual NDV at the configured FPP,
instead of always pre-allocating `bloom-filter-max-bytes`.
## Why are the changes needed?
Today, when bloom filter is enabled on a column without per-column NDV, parquet-mr's
`ColumnValueCollector.initBloomFilter()` allocates a fixed `bloom-filter-max-bytes` buffer:
```java
// from ColumnValueCollector
} else {
this.bloomFilter = new BlockSplitBloomFilter(maxBloomFilterSize, maxBloomFilterSize);
}
```
The buffer is then written to disk via `ParquetFileWriter.serializeBloomFilters()` regardless of
how many values were inserted. For low-row-count writes this produces a file dominated by an
empty bloom filter.
Empirical observations from a Spark Structured Streaming + Iceberg pipeline (~720 silver
tables on warm 600s trigger):
| TBLPROPERTIES | File size for 5-row write |
|---|---|
| no bloom | 902 bytes |
| `bloom-enabled.col.id=true, max-bytes=4194304` | 4,201,826 bytes (~4 MiB) |
| **+ adaptive (this PR)** | **268,465 bytes (~16x reduction)** |
For workloads that produce frequent low-row-count microbatches (CDC streaming, frequent commits),
this is a significant storage and S3 PUT cost reduction.
## How was this patch tested?
- New unit test `TestParquetAdaptiveBloomFilter` covers both:
- Adaptive enabled — verifies file is at least 2x smaller than non-adaptive
- Default behavior — verifies existing (non-adaptive) behavior is preserved when
the property is not set
- Empirically verified on a production Spark Structured Streaming + Iceberg pipeline.
File sizes dropped 4 MiB → ~268 KiB on streaming microbatch outputs.
## Backward compatibility
Default value is `false`, so existing tables and writers see no behavior change.
Operators opt in by setting `write.parquet.bloom-filter-adaptive-enabled=true`.
## Scope
This PR modifies the createWriterFunc code path (used by Spark, Flink, and other engines for
data writes). The legacy `ParquetWriteBuilder` fallback path (`createWriterFunc == null`) is
unchanged. If maintainers want adaptive support on the legacy path as well, happy to extend in
a follow-up.
## Files changed
- `core/src/main/java/org/apache/iceberg/TableProperties.java` — add 1 constant + default
- `parquet/src/main/java/org/apache/iceberg/parquet/Parquet.java` — wire the property through
Context + use in WriteBuilder
- `parquet/src/test/java/org/apache/iceberg/parquet/TestParquetAdaptiveBloomFilter.java` — new test
--
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
Re: [PR] Parquet: add adaptive bloom filter sizing (PARQUET-2254) [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #16363:
URL: https://github.com/apache/iceberg/pull/16363#discussion_r3529805024
##########
core/src/main/java/org/apache/iceberg/TableProperties.java:
##########
@@ -176,14 +176,21 @@ private TableProperties() {}
"write.delete.parquet.row-group-check-max-record-count";
public static final int PARQUET_ROW_GROUP_CHECK_MAX_RECORD_COUNT_DEFAULT = 10000;
- public static final String PARQUET_ROW_GROUP_SIZE_TRACK_UNCOMPRESSED =
- "write.parquet.row-group-size-track-uncompressed";
- public static final boolean PARQUET_ROW_GROUP_SIZE_TRACK_UNCOMPRESSED_DEFAULT = false;
-
public static final String PARQUET_BLOOM_FILTER_MAX_BYTES =
"write.parquet.bloom-filter-max-bytes";
public static final int PARQUET_BLOOM_FILTER_MAX_BYTES_DEFAULT = 1024 * 1024;
+ // Enables parquet-mr's adaptive bloom filter sizing (PARQUET-2254). When true,
+ // the writer evaluates ~5 candidate bloom filters and picks the smallest that
+ // satisfies actual NDV at FPP. Works in combination with
+ // PARQUET_BLOOM_FILTER_MAX_BYTES, which bounds the largest candidate — the
+ // adaptive size never exceeds it. Significantly reduces file size for
+ // low-row-count writes (e.g., streaming microbatches) that would otherwise pad
+ // the parquet file with an empty `bloom-filter-max-bytes` buffer.
Review Comment:
```suggestion
// the parquet file with an empty PARQUET_BLOOM_FILTER_MAX_BYTES buffer.
```
--
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
Re: [PR] Parquet: add adaptive bloom filter sizing (PARQUET-2254) [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #16363:
URL: https://github.com/apache/iceberg/pull/16363#discussion_r3529830769
##########
parquet/src/test/java/org/apache/iceberg/parquet/TestParquetAdaptiveBloomFilter.java:
##########
@@ -0,0 +1,119 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.parquet;
+
+import static org.apache.iceberg.TableProperties.PARQUET_BLOOM_FILTER_ADAPTIVE_ENABLED;
+import static org.apache.iceberg.TableProperties.PARQUET_BLOOM_FILTER_COLUMN_ENABLED_PREFIX;
+import static org.apache.iceberg.TableProperties.PARQUET_BLOOM_FILTER_MAX_BYTES;
+import static org.apache.iceberg.types.Types.NestedField.required;
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.file.Path;
+import org.apache.avro.generic.GenericData;
+import org.apache.avro.generic.GenericRecordBuilder;
+import org.apache.iceberg.Files;
+import org.apache.iceberg.Schema;
+import org.apache.iceberg.avro.AvroSchemaUtil;
+import org.apache.iceberg.io.FileAppender;
+import org.apache.iceberg.io.OutputFile;
+import org.apache.iceberg.types.Types;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
+
+/**
+ * Tests for the adaptive bloom filter sizing TBLPROPERTY ({@code
+ * write.parquet.bloom-filter-adaptive-enabled}).
+ *
+ * <p>Without adaptive sizing, parquet-mr's {@code ColumnValueCollector.initBloomFilter} allocates a
+ * fixed {@code bloom-filter-max-bytes} buffer per bloom-enabled column regardless of actual NDV
Review Comment:
all of the javadoc should actually use properties from the Iceberg codebase instead of property names from Parquet itself
--
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
Re: [PR] Parquet: add adaptive bloom filter sizing (PARQUET-2254) [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #16363:
URL: https://github.com/apache/iceberg/pull/16363#discussion_r3535150346
##########
core/src/main/java/org/apache/iceberg/TableProperties.java:
##########
@@ -176,14 +176,21 @@ private TableProperties() {}
"write.delete.parquet.row-group-check-max-record-count";
public static final int PARQUET_ROW_GROUP_CHECK_MAX_RECORD_COUNT_DEFAULT = 10000;
- public static final String PARQUET_ROW_GROUP_SIZE_TRACK_UNCOMPRESSED =
- "write.parquet.row-group-size-track-uncompressed";
- public static final boolean PARQUET_ROW_GROUP_SIZE_TRACK_UNCOMPRESSED_DEFAULT = false;
-
public static final String PARQUET_BLOOM_FILTER_MAX_BYTES =
"write.parquet.bloom-filter-max-bytes";
public static final int PARQUET_BLOOM_FILTER_MAX_BYTES_DEFAULT = 1024 * 1024;
+ // Enables Parquet's adaptive bloom filter sizing. When true,
+ // the writer evaluates ~5 candidate bloom filters and picks the smallest that
+ // satisfies actual NDV at FPP. Works in combination with
+ // PARQUET_BLOOM_FILTER_MAX_BYTES, which bounds the largest candidate — the
+ // adaptive size never exceeds it. Significantly reduces file size for
+ // low-row-count writes (e.g., streaming microbatches) that would otherwise pad
+ // the parquet file with an empty PARQUET_BLOOM_FILTER_MAX_BYTES buffer.
Review Comment:
nit: do we feel strong about this comment? Apart from the locations there is no comment in the file. It is nicely documented in the configuration.md. Is that enough?
--
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
Re: [PR] Parquet: add adaptive bloom filter sizing (PARQUET-2254) [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #16363:
URL: https://github.com/apache/iceberg/pull/16363#discussion_r3551086574
##########
parquet/src/test/java/org/apache/iceberg/parquet/TestParquet.java:
##########
@@ -516,6 +341,63 @@ public void testAvroWriterRejectsVariantType() {
.hasMessage("Avro writer does not support variant types");
}
+ @Test
+ public void adaptiveBloomFilterSizingShrinksFile() throws IOException {
+ // without PARQUET_BLOOM_FILTER_ADAPTIVE_ENABLED, the writer allocates the full
+ // PARQUET_BLOOM_FILTER_MAX_BYTES buffer (4 MiB here) regardless of the number
+ // of distinct values written
+ long sizeWithoutAdaptive = writeWithBloomFilter(false);
+ assertThat(sizeWithoutAdaptive)
+ .as("non-adaptive file should pad the bloom filter to PARQUET_BLOOM_FILTER_MAX_BYTES")
+ .isGreaterThan(3_500_000L);
+
+ // with PARQUET_BLOOM_FILTER_ADAPTIVE_ENABLED, the writer picks the smallest candidate
+ // bloom filter that satisfies the actual number of distinct values (5) at the
+ // configured FPP
+ long sizeWithAdaptive = writeWithBloomFilter(true);
+ assertThat(sizeWithAdaptive)
+ .as("adaptive file should be at least 2x smaller than the non-adaptive file")
+ .isLessThan(sizeWithoutAdaptive / 2);
+ }
+
+ @Test
+ public void adaptiveBloomFilterDisabledByDefault() throws IOException {
+ // when PARQUET_BLOOM_FILTER_ADAPTIVE_ENABLED is not set, the existing behavior is
+ // preserved: the full PARQUET_BLOOM_FILTER_MAX_BYTES buffer is allocated
+ long size = writeWithBloomFilter(false);
+
+ assertThat(size)
+ .as("default write should pad the bloom filter to PARQUET_BLOOM_FILTER_MAX_BYTES")
+ .isGreaterThan(3_500_000L);
Review Comment:
This is exactly the same code as R349-R351.
Is this intentional?
--
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
Re: [PR] Parquet: add adaptive bloom filter sizing (PARQUET-2326) [iceberg]
Posted by "raghav-reglobe (via GitHub)" <gi...@apache.org>.
raghav-reglobe commented on PR #16363:
URL: https://github.com/apache/iceberg/pull/16363#issuecomment-4766034351
@nastra despite passing all the checks, this PR going to close for inactivity, Can someone from your team, help me understand this process of getting it reviewed.
--
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
Re: [PR] Parquet: add adaptive bloom filter sizing (PARQUET-2254) [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #16363:
URL: https://github.com/apache/iceberg/pull/16363#discussion_r3529795602
##########
docs/docs/configuration.md:
##########
@@ -57,8 +56,8 @@ Iceberg tables support table properties to configure table behavior, like the de
| write.parquet.bloom-filter-max-bytes | 1048576 (1 MB) | The maximum number of bytes for a bloom filter bitset |
| write.parquet.bloom-filter-fpp.column.col1 | 0.01 | The false positive probability for a bloom filter applied to 'col1' (must > 0.0 and < 1.0) |
| write.parquet.bloom-filter-ndv.column.col1 | (not set) | The expected number of distinct values for a bloom filter applied to 'col1' (must > 0) |
+| write.parquet.bloom-filter-adaptive-enabled | false | Use parquet-mr adaptive bloom filter sizing (PARQUET-2254): evaluate candidate filters and pick the smallest that satisfies the actual NDV at the configured FPP, bounded by 'bloom-filter-max-bytes', instead of always allocating the full max-bytes buffer |
Review Comment:
```suggestion
| write.parquet.bloom-filter-adaptive-enabled | false | Use Parquet adaptive bloom filter sizing, which evaluate candidate filters and picks the smallest that satisfies the actual NDV at the configured FPP, bounded by 'write.parquet.bloom-filter-max-bytes', instead of always allocating the full max-bytes buffer |
```
--
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
Re: [PR] Parquet: add adaptive bloom filter sizing (PARQUET-2254) [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary merged PR #16363:
URL: https://github.com/apache/iceberg/pull/16363
--
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
Re: [PR] Parquet: add adaptive bloom filter sizing (PARQUET-2326) [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #16363:
URL: https://github.com/apache/iceberg/pull/16363#discussion_r3450628246
##########
parquet/src/main/java/org/apache/iceberg/parquet/Parquet.java:
##########
@@ -505,6 +516,7 @@ static class Context {
private final int rowGroupCheckMinRecordCount;
private final int rowGroupCheckMaxRecordCount;
private final int bloomFilterMaxBytes;
+ private final boolean bloomFilterAdaptiveEnabled;
Review Comment:
```suggestion
private final boolean adaptiveBloomFilterEnabled;
```
--
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
Re: [PR] Parquet: add adaptive bloom filter sizing (PARQUET-2326) [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #16363:
URL: https://github.com/apache/iceberg/pull/16363#discussion_r3450630355
##########
parquet/src/main/java/org/apache/iceberg/parquet/Parquet.java:
##########
@@ -767,6 +789,10 @@ int bloomFilterMaxBytes() {
return bloomFilterMaxBytes;
}
+ boolean bloomFilterAdaptiveEnabled() {
Review Comment:
```suggestion
boolean adaptiveBloomFilterEnabled() {
```
--
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
Re: [PR] Parquet: add adaptive bloom filter sizing (PARQUET-2254) [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #16363:
URL: https://github.com/apache/iceberg/pull/16363#discussion_r3559384092
##########
parquet/src/test/java/org/apache/iceberg/parquet/TestParquet.java:
##########
@@ -516,6 +341,52 @@ public void testAvroWriterRejectsVariantType() {
.hasMessage("Avro writer does not support variant types");
}
+ @Test
+ public void adaptiveBloomFilterSizingShrinksFile() throws IOException {
+ // when PARQUET_BLOOM_FILTER_ADAPTIVE_ENABLED is not set (the default), the writer
+ // allocates the full PARQUET_BLOOM_FILTER_MAX_BYTES buffer (4 MiB here) regardless
+ // of the number of distinct values written
+ long sizeWithoutAdaptive = writeWithBloomFilter(null);
+ assertThat(sizeWithoutAdaptive)
+ .as("non-adaptive file should pad the bloom filter to PARQUET_BLOOM_FILTER_MAX_BYTES")
+ .isGreaterThan(3_500_000L);
+
+ // with PARQUET_BLOOM_FILTER_ADAPTIVE_ENABLED, the writer picks the smallest candidate
+ // bloom filter that satisfies the actual number of distinct values (5) at the
+ // configured FPP
+ long sizeWithAdaptive = writeWithBloomFilter(true);
+ assertThat(sizeWithAdaptive)
+ .as("adaptive file should be at least 2x smaller than the non-adaptive file")
+ .isLessThan(sizeWithoutAdaptive / 2);
+ }
+
+ private long writeWithBloomFilter(Boolean adaptiveEnabled) throws IOException {
Review Comment:
nit: any reason for the `Boolean` instead of the primitive `boolean`?
--
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
Re: [PR] Parquet: add adaptive bloom filter sizing (PARQUET-2254) [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #16363:
URL: https://github.com/apache/iceberg/pull/16363#discussion_r3559500028
##########
parquet/src/test/java/org/apache/iceberg/parquet/TestParquet.java:
##########
@@ -516,6 +341,52 @@ public void testAvroWriterRejectsVariantType() {
.hasMessage("Avro writer does not support variant types");
}
+ @Test
+ public void adaptiveBloomFilterSizingShrinksFile() throws IOException {
+ // when PARQUET_BLOOM_FILTER_ADAPTIVE_ENABLED is not set (the default), the writer
+ // allocates the full PARQUET_BLOOM_FILTER_MAX_BYTES buffer (4 MiB here) regardless
+ // of the number of distinct values written
+ long sizeWithoutAdaptive = writeWithBloomFilter(null);
+ assertThat(sizeWithoutAdaptive)
+ .as("non-adaptive file should pad the bloom filter to PARQUET_BLOOM_FILTER_MAX_BYTES")
+ .isGreaterThan(3_500_000L);
+
+ // with PARQUET_BLOOM_FILTER_ADAPTIVE_ENABLED, the writer picks the smallest candidate
+ // bloom filter that satisfies the actual number of distinct values (5) at the
+ // configured FPP
+ long sizeWithAdaptive = writeWithBloomFilter(true);
+ assertThat(sizeWithAdaptive)
+ .as("adaptive file should be at least 2x smaller than the non-adaptive file")
+ .isLessThan(sizeWithoutAdaptive / 2);
+ }
+
+ private long writeWithBloomFilter(Boolean adaptiveEnabled) throws IOException {
Review Comment:
The Boolean is good if we want to keep the default value test.
We need to update the test cases, and also the `if` statement in the row 370-372
--
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
Re: [PR] Parquet: add adaptive bloom filter sizing (PARQUET-2254) [iceberg]
Posted by "raghav-reglobe (via GitHub)" <gi...@apache.org>.
raghav-reglobe commented on PR #16363:
URL: https://github.com/apache/iceberg/pull/16363#issuecomment-4775265082
Good catch on the issue number — you're right, PARQUET-2326 is the jcommander bump. The adaptive bloom filter feature is PARQUET-2254 (apache/parquet-java#1042, which added `AdaptiveBlockSplitBloomFilter`). Fixed that across the code, the PR title, and the description.
The rest of the review is in there too: renamed the field/accessor to `adaptiveBloomFilterEnabled`, noted in the javadoc that it pairs with `PARQUET_BLOOM_FILTER_MAX_BYTES`, documented the property in `configuration.md`, and dropped the `test` prefixes on the new tests.
--
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
Re: [PR] Parquet: add adaptive bloom filter sizing (PARQUET-2326) [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on PR #16363:
URL: https://github.com/apache/iceberg/pull/16363#issuecomment-4766078015
@raghav-reglobe is PARQUET-2326 actually the right issue? Because it refers to a jcommander version bump (https://github.com/apache/parquet-java/pull/1123)
--
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
Re: [PR] Parquet: add adaptive bloom filter sizing (PARQUET-2254) [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #16363:
URL: https://github.com/apache/iceberg/pull/16363#discussion_r3529857481
##########
parquet/src/test/java/org/apache/iceberg/parquet/TestParquetAdaptiveBloomFilter.java:
##########
@@ -0,0 +1,119 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.parquet;
+
+import static org.apache.iceberg.TableProperties.PARQUET_BLOOM_FILTER_ADAPTIVE_ENABLED;
+import static org.apache.iceberg.TableProperties.PARQUET_BLOOM_FILTER_COLUMN_ENABLED_PREFIX;
+import static org.apache.iceberg.TableProperties.PARQUET_BLOOM_FILTER_MAX_BYTES;
+import static org.apache.iceberg.types.Types.NestedField.required;
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.file.Path;
+import org.apache.avro.generic.GenericData;
+import org.apache.avro.generic.GenericRecordBuilder;
+import org.apache.iceberg.Files;
+import org.apache.iceberg.Schema;
+import org.apache.iceberg.avro.AvroSchemaUtil;
+import org.apache.iceberg.io.FileAppender;
+import org.apache.iceberg.io.OutputFile;
+import org.apache.iceberg.types.Types;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
+
+/**
+ * Tests for the adaptive bloom filter sizing TBLPROPERTY ({@code
+ * write.parquet.bloom-filter-adaptive-enabled}).
+ *
+ * <p>Without adaptive sizing, parquet-mr's {@code ColumnValueCollector.initBloomFilter} allocates a
+ * fixed {@code bloom-filter-max-bytes} buffer per bloom-enabled column regardless of actual NDV
+ * (the {@code !ndv.isPresent() && !adaptive} branch calls {@code new BlockSplitBloomFilter(
+ * maxBloomFilterSize, maxBloomFilterSize)}). For low-row-count writes this produces a file
+ * dominated by an empty bloom filter buffer. The adaptive flag (PARQUET-2254) makes parquet-mr pick
+ * the smallest of N candidate filter sizes that satisfies actual NDV at FPP.
+ *
+ * <p>These tests use the createWriterFunc code path because that is the path Iceberg uses for Spark
+ * and Flink data writes; the legacy ParquetWriteBuilder path goes through parquet-mr directly
+ * without consulting Iceberg-specific properties.
+ */
+public class TestParquetAdaptiveBloomFilter {
+
+ private static final Schema SCHEMA = new Schema(required(1, "id", Types.LongType.get()));
+
+ @TempDir private Path temp;
+
+ /**
+ * Writes a parquet file via the createWriterFunc code path with bloom filter enabled on the
+ * {@code id} column and a 4 MiB max-bytes cap. Returns the resulting file size.
+ */
+ private long writeAndMeasure(boolean adaptive) throws IOException {
Review Comment:
nit: this doesn't really measure anything but only returns the file length after writing
--
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
Re: [PR] Parquet: add adaptive bloom filter sizing (PARQUET-2254) [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on PR #16363:
URL: https://github.com/apache/iceberg/pull/16363#issuecomment-4894278475
@raghav-reglobe please disclose AI usage on the PR description. See also https://iceberg.apache.org/contribute/#guidelines-for-ai-assisted-contributions for some details
--
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
Re: [PR] Parquet: add adaptive bloom filter sizing (PARQUET-2254) [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on PR #16363:
URL: https://github.com/apache/iceberg/pull/16363#issuecomment-4957605400
Merged to main.
Thanks @raghav-reglobe for the PR and @nastra for the review!
--
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
Re: [PR] Parquet: add adaptive bloom filter sizing (PARQUET-2254) [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #16363:
URL: https://github.com/apache/iceberg/pull/16363#discussion_r3529823963
##########
docs/docs/configuration.md:
##########
@@ -57,8 +56,8 @@ Iceberg tables support table properties to configure table behavior, like the de
| write.parquet.bloom-filter-max-bytes | 1048576 (1 MB) | The maximum number of bytes for a bloom filter bitset |
| write.parquet.bloom-filter-fpp.column.col1 | 0.01 | The false positive probability for a bloom filter applied to 'col1' (must > 0.0 and < 1.0) |
| write.parquet.bloom-filter-ndv.column.col1 | (not set) | The expected number of distinct values for a bloom filter applied to 'col1' (must > 0) |
+| write.parquet.bloom-filter-adaptive-enabled | false | Use parquet-mr adaptive bloom filter sizing (PARQUET-2254): evaluate candidate filters and pick the smallest that satisfies the actual NDV at the configured FPP, bounded by 'bloom-filter-max-bytes', instead of always allocating the full max-bytes buffer |
Review Comment:
this should probably mention that adaptive bloom filter is ignored when per-column NDV is set
--
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
Re: [PR] Parquet: add adaptive bloom filter sizing (PARQUET-2254) [iceberg]
Posted by "raghav-reglobe (via GitHub)" <gi...@apache.org>.
raghav-reglobe commented on PR #16363:
URL: https://github.com/apache/iceberg/pull/16363#issuecomment-4795389075
@nastra I have done all changes you suggested along with correcting the reference to PARQUET-2254.
--
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
Re: [PR] Parquet: add adaptive bloom filter sizing (PARQUET-2326) [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #16363:
URL: https://github.com/apache/iceberg/pull/16363#discussion_r3450621790
##########
core/src/main/java/org/apache/iceberg/TableProperties.java:
##########
@@ -180,6 +180,16 @@ private TableProperties() {}
"write.parquet.bloom-filter-max-bytes";
public static final int PARQUET_BLOOM_FILTER_MAX_BYTES_DEFAULT = 1024 * 1024;
+ // Enables parquet-mr's adaptive bloom filter sizing (PARQUET-2326). When true,
+ // the writer evaluates ~5 candidate bloom filters and picks the smallest that
+ // satisfies actual NDV at FPP, instead of always allocating
+ // `bloom-filter-max-bytes`. Significantly reduces file size for low-row-count
+ // writes (e.g., streaming microbatches) that would otherwise pad the parquet
+ // file with an empty `bloom-filter-max-bytes` buffer.
Review Comment:
I would mention that this works in combination of `PARQUET_BLOOM_FILTER_MAX_BYTES`
--
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
Re: [PR] Parquet: add adaptive bloom filter sizing (PARQUET-2254) [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #16363:
URL: https://github.com/apache/iceberg/pull/16363#discussion_r3529841942
##########
parquet/src/test/java/org/apache/iceberg/parquet/TestParquetAdaptiveBloomFilter.java:
##########
@@ -0,0 +1,119 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.parquet;
+
+import static org.apache.iceberg.TableProperties.PARQUET_BLOOM_FILTER_ADAPTIVE_ENABLED;
+import static org.apache.iceberg.TableProperties.PARQUET_BLOOM_FILTER_COLUMN_ENABLED_PREFIX;
+import static org.apache.iceberg.TableProperties.PARQUET_BLOOM_FILTER_MAX_BYTES;
+import static org.apache.iceberg.types.Types.NestedField.required;
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.file.Path;
+import org.apache.avro.generic.GenericData;
+import org.apache.avro.generic.GenericRecordBuilder;
+import org.apache.iceberg.Files;
+import org.apache.iceberg.Schema;
+import org.apache.iceberg.avro.AvroSchemaUtil;
+import org.apache.iceberg.io.FileAppender;
+import org.apache.iceberg.io.OutputFile;
+import org.apache.iceberg.types.Types;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
+
+/**
+ * Tests for the adaptive bloom filter sizing TBLPROPERTY ({@code
+ * write.parquet.bloom-filter-adaptive-enabled}).
+ *
+ * <p>Without adaptive sizing, parquet-mr's {@code ColumnValueCollector.initBloomFilter} allocates a
+ * fixed {@code bloom-filter-max-bytes} buffer per bloom-enabled column regardless of actual NDV
+ * (the {@code !ndv.isPresent() && !adaptive} branch calls {@code new BlockSplitBloomFilter(
+ * maxBloomFilterSize, maxBloomFilterSize)}). For low-row-count writes this produces a file
+ * dominated by an empty bloom filter buffer. The adaptive flag (PARQUET-2254) makes parquet-mr pick
+ * the smallest of N candidate filter sizes that satisfies actual NDV at FPP.
+ *
+ * <p>These tests use the createWriterFunc code path because that is the path Iceberg uses for Spark
+ * and Flink data writes; the legacy ParquetWriteBuilder path goes through parquet-mr directly
+ * without consulting Iceberg-specific properties.
+ */
+public class TestParquetAdaptiveBloomFilter {
+
+ private static final Schema SCHEMA = new Schema(required(1, "id", Types.LongType.get()));
+
+ @TempDir private Path temp;
+
+ /**
+ * Writes a parquet file via the createWriterFunc code path with bloom filter enabled on the
+ * {@code id} column and a 4 MiB max-bytes cap. Returns the resulting file size.
+ */
+ private long writeAndMeasure(boolean adaptive) throws IOException {
+ File file = new File(temp.toFile(), "test_" + adaptive + "_" + System.nanoTime() + ".parquet");
+ OutputFile outFile = Files.localOutput(file);
+
+ Parquet.WriteBuilder writer =
+ Parquet.write(outFile)
+ .schema(SCHEMA)
+ .createWriterFunc(ParquetAvroWriter::buildWriter)
+ .set(PARQUET_BLOOM_FILTER_COLUMN_ENABLED_PREFIX + "id", "true")
+ .set(PARQUET_BLOOM_FILTER_MAX_BYTES, "4194304");
+
+ if (adaptive) {
+ writer = writer.set(PARQUET_BLOOM_FILTER_ADAPTIVE_ENABLED, "true");
+ }
+
+ org.apache.avro.Schema avroSchema = AvroSchemaUtil.convert(SCHEMA.asStruct());
+ try (FileAppender<GenericData.Record> appender = writer.build()) {
+ for (int i = 0; i < 5; i++) {
+ appender.add(new GenericRecordBuilder(avroSchema).set("id", (long) i).build());
+ }
+ }
+
+ return file.length();
+ }
+
+ @Test
+ public void adaptiveSizingShrinksFile() throws IOException {
+ // Without adaptive, parquet-mr writes the full bloom-filter-max-bytes buffer (~4 MiB).
Review Comment:
this here also refers to Parquet properties. Please use Iceberg properties everywhere
--
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
Re: [PR] Parquet: add adaptive bloom filter sizing (PARQUET-2326) [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #16363:
URL: https://github.com/apache/iceberg/pull/16363#issuecomment-4755743149
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions.
--
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
Re: [PR] Parquet: add adaptive bloom filter sizing (PARQUET-2326) [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #16363:
URL: https://github.com/apache/iceberg/pull/16363#discussion_r3450625553
##########
core/src/main/java/org/apache/iceberg/TableProperties.java:
##########
@@ -180,6 +180,16 @@ private TableProperties() {}
"write.parquet.bloom-filter-max-bytes";
public static final int PARQUET_BLOOM_FILTER_MAX_BYTES_DEFAULT = 1024 * 1024;
+ // Enables parquet-mr's adaptive bloom filter sizing (PARQUET-2326). When true,
+ // the writer evaluates ~5 candidate bloom filters and picks the smallest that
+ // satisfies actual NDV at FPP, instead of always allocating
+ // `bloom-filter-max-bytes`. Significantly reduces file size for low-row-count
+ // writes (e.g., streaming microbatches) that would otherwise pad the parquet
+ // file with an empty `bloom-filter-max-bytes` buffer.
Review Comment:
please also update `configuration.md`
--
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
Re: [PR] Parquet: add adaptive bloom filter sizing (PARQUET-2254) [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #16363:
URL: https://github.com/apache/iceberg/pull/16363#discussion_r3559114590
##########
parquet/src/test/java/org/apache/iceberg/parquet/TestParquet.java:
##########
@@ -516,6 +341,52 @@ public void testAvroWriterRejectsVariantType() {
.hasMessage("Avro writer does not support variant types");
}
+ @Test
+ public void adaptiveBloomFilterSizingShrinksFile() throws IOException {
+ // when PARQUET_BLOOM_FILTER_ADAPTIVE_ENABLED is not set (the default), the writer
+ // allocates the full PARQUET_BLOOM_FILTER_MAX_BYTES buffer (4 MiB here) regardless
+ // of the number of distinct values written
+ long sizeWithoutAdaptive = writeWithBloomFilter(false);
+ assertThat(sizeWithoutAdaptive)
+ .as("non-adaptive file should pad the bloom filter to PARQUET_BLOOM_FILTER_MAX_BYTES")
+ .isGreaterThan(3_500_000L);
+
+ // with PARQUET_BLOOM_FILTER_ADAPTIVE_ENABLED, the writer picks the smallest candidate
+ // bloom filter that satisfies the actual number of distinct values (5) at the
+ // configured FPP
+ long sizeWithAdaptive = writeWithBloomFilter(true);
+ assertThat(sizeWithAdaptive)
+ .as("adaptive file should be at least 2x smaller than the non-adaptive file")
+ .isLessThan(sizeWithoutAdaptive / 2);
+ }
+
+ private long writeWithBloomFilter(boolean adaptiveEnabled) throws IOException {
+ Schema schema = new Schema(required(1, "id", Types.LongType.get()));
+
+ ImmutableMap.Builder<String, String> propsBuilder =
+ ImmutableMap.<String, String>builder()
+ .put(PARQUET_BLOOM_FILTER_COLUMN_ENABLED_PREFIX + "id", "true")
+ .put(PARQUET_BLOOM_FILTER_MAX_BYTES, "4194304");
+ if (adaptiveEnabled) {
+ propsBuilder.put(PARQUET_BLOOM_FILTER_ADAPTIVE_ENABLED, "true");
+ }
Review Comment:
I'm okayish not testing the default value (that would be better, but it could be an overkill), but having a parameter setting to `false` and then relying on the default value is problematic.
Minimally this:
```suggestion
propsBuilder.put(PARQUET_BLOOM_FILTER_ADAPTIVE_ENABLED, String.valueOf(adaptiveEnabled));
```
Or with a Boolean parameter with true/false/null, and
```suggestion
if (adaptiveEnabled != null) {
propsBuilder.put(PARQUET_BLOOM_FILTER_ADAPTIVE_ENABLED, adaptiveEnabled.toString());
}
```
--
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
Re: [PR] Parquet: add adaptive bloom filter sizing (PARQUET-2254) [iceberg]
Posted by "raghav-reglobe (via GitHub)" <gi...@apache.org>.
raghav-reglobe commented on code in PR #16363:
URL: https://github.com/apache/iceberg/pull/16363#discussion_r3545800406
##########
core/src/main/java/org/apache/iceberg/TableProperties.java:
##########
@@ -176,14 +176,21 @@ private TableProperties() {}
"write.delete.parquet.row-group-check-max-record-count";
public static final int PARQUET_ROW_GROUP_CHECK_MAX_RECORD_COUNT_DEFAULT = 10000;
- public static final String PARQUET_ROW_GROUP_SIZE_TRACK_UNCOMPRESSED =
- "write.parquet.row-group-size-track-uncompressed";
- public static final boolean PARQUET_ROW_GROUP_SIZE_TRACK_UNCOMPRESSED_DEFAULT = false;
-
public static final String PARQUET_BLOOM_FILTER_MAX_BYTES =
"write.parquet.bloom-filter-max-bytes";
public static final int PARQUET_BLOOM_FILTER_MAX_BYTES_DEFAULT = 1024 * 1024;
+ // Enables Parquet's adaptive bloom filter sizing. When true,
+ // the writer evaluates ~5 candidate bloom filters and picks the smallest that
+ // satisfies actual NDV at FPP. Works in combination with
+ // PARQUET_BLOOM_FILTER_MAX_BYTES, which bounds the largest candidate — the
+ // adaptive size never exceeds it. Significantly reduces file size for
+ // low-row-count writes (e.g., streaming microbatches) that would otherwise pad
+ // the parquet file with an empty PARQUET_BLOOM_FILTER_MAX_BYTES buffer.
Review Comment:
Not strongly — dropped it. configuration.md covers it.
##########
parquet/src/test/java/org/apache/iceberg/parquet/TestParquetAdaptiveBloomFilter.java:
##########
@@ -0,0 +1,119 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.parquet;
+
+import static org.apache.iceberg.TableProperties.PARQUET_BLOOM_FILTER_ADAPTIVE_ENABLED;
+import static org.apache.iceberg.TableProperties.PARQUET_BLOOM_FILTER_COLUMN_ENABLED_PREFIX;
+import static org.apache.iceberg.TableProperties.PARQUET_BLOOM_FILTER_MAX_BYTES;
+import static org.apache.iceberg.types.Types.NestedField.required;
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.file.Path;
+import org.apache.avro.generic.GenericData;
+import org.apache.avro.generic.GenericRecordBuilder;
+import org.apache.iceberg.Files;
+import org.apache.iceberg.Schema;
+import org.apache.iceberg.avro.AvroSchemaUtil;
+import org.apache.iceberg.io.FileAppender;
+import org.apache.iceberg.io.OutputFile;
+import org.apache.iceberg.types.Types;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
+
+/**
+ * Tests for the adaptive bloom filter sizing TBLPROPERTY ({@code
+ * write.parquet.bloom-filter-adaptive-enabled}).
+ *
+ * <p>Without adaptive sizing, parquet-mr's {@code ColumnValueCollector.initBloomFilter} allocates a
+ * fixed {@code bloom-filter-max-bytes} buffer per bloom-enabled column regardless of actual NDV
+ * (the {@code !ndv.isPresent() && !adaptive} branch calls {@code new BlockSplitBloomFilter(
+ * maxBloomFilterSize, maxBloomFilterSize)}). For low-row-count writes this produces a file
+ * dominated by an empty bloom filter buffer. The adaptive flag (PARQUET-2254) makes parquet-mr pick
+ * the smallest of N candidate filter sizes that satisfies actual NDV at FPP.
+ *
+ * <p>These tests use the createWriterFunc code path because that is the path Iceberg uses for Spark
+ * and Flink data writes; the legacy ParquetWriteBuilder path goes through parquet-mr directly
+ * without consulting Iceberg-specific properties.
+ */
+public class TestParquetAdaptiveBloomFilter {
Review Comment:
No strong reason — moved the two tests into TestParquet next to the other writer-property tests and dropped the separate class.
##########
parquet/src/test/java/org/apache/iceberg/parquet/TestParquetAdaptiveBloomFilter.java:
##########
@@ -0,0 +1,119 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.parquet;
+
+import static org.apache.iceberg.TableProperties.PARQUET_BLOOM_FILTER_ADAPTIVE_ENABLED;
+import static org.apache.iceberg.TableProperties.PARQUET_BLOOM_FILTER_COLUMN_ENABLED_PREFIX;
+import static org.apache.iceberg.TableProperties.PARQUET_BLOOM_FILTER_MAX_BYTES;
+import static org.apache.iceberg.types.Types.NestedField.required;
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.file.Path;
+import org.apache.avro.generic.GenericData;
+import org.apache.avro.generic.GenericRecordBuilder;
+import org.apache.iceberg.Files;
+import org.apache.iceberg.Schema;
+import org.apache.iceberg.avro.AvroSchemaUtil;
+import org.apache.iceberg.io.FileAppender;
+import org.apache.iceberg.io.OutputFile;
+import org.apache.iceberg.types.Types;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
+
+/**
+ * Tests for the adaptive bloom filter sizing TBLPROPERTY ({@code
+ * write.parquet.bloom-filter-adaptive-enabled}).
+ *
+ * <p>Without adaptive sizing, parquet-mr's {@code ColumnValueCollector.initBloomFilter} allocates a
+ * fixed {@code bloom-filter-max-bytes} buffer per bloom-enabled column regardless of actual NDV
Review Comment:
The class javadoc is gone now that the tests live in TestParquet — the remaining comments reference the Iceberg property constants.
##########
parquet/src/test/java/org/apache/iceberg/parquet/TestParquetAdaptiveBloomFilter.java:
##########
@@ -0,0 +1,119 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.parquet;
+
+import static org.apache.iceberg.TableProperties.PARQUET_BLOOM_FILTER_ADAPTIVE_ENABLED;
+import static org.apache.iceberg.TableProperties.PARQUET_BLOOM_FILTER_COLUMN_ENABLED_PREFIX;
+import static org.apache.iceberg.TableProperties.PARQUET_BLOOM_FILTER_MAX_BYTES;
+import static org.apache.iceberg.types.Types.NestedField.required;
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.file.Path;
+import org.apache.avro.generic.GenericData;
+import org.apache.avro.generic.GenericRecordBuilder;
+import org.apache.iceberg.Files;
+import org.apache.iceberg.Schema;
+import org.apache.iceberg.avro.AvroSchemaUtil;
+import org.apache.iceberg.io.FileAppender;
+import org.apache.iceberg.io.OutputFile;
+import org.apache.iceberg.types.Types;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
+
+/**
+ * Tests for the adaptive bloom filter sizing TBLPROPERTY ({@code
+ * write.parquet.bloom-filter-adaptive-enabled}).
+ *
+ * <p>Without adaptive sizing, parquet-mr's {@code ColumnValueCollector.initBloomFilter} allocates a
+ * fixed {@code bloom-filter-max-bytes} buffer per bloom-enabled column regardless of actual NDV
+ * (the {@code !ndv.isPresent() && !adaptive} branch calls {@code new BlockSplitBloomFilter(
+ * maxBloomFilterSize, maxBloomFilterSize)}). For low-row-count writes this produces a file
+ * dominated by an empty bloom filter buffer. The adaptive flag (PARQUET-2254) makes parquet-mr pick
+ * the smallest of N candidate filter sizes that satisfies actual NDV at FPP.
+ *
+ * <p>These tests use the createWriterFunc code path because that is the path Iceberg uses for Spark
+ * and Flink data writes; the legacy ParquetWriteBuilder path goes through parquet-mr directly
+ * without consulting Iceberg-specific properties.
+ */
+public class TestParquetAdaptiveBloomFilter {
+
+ private static final Schema SCHEMA = new Schema(required(1, "id", Types.LongType.get()));
+
+ @TempDir private Path temp;
+
+ /**
+ * Writes a parquet file via the createWriterFunc code path with bloom filter enabled on the
+ * {@code id} column and a 4 MiB max-bytes cap. Returns the resulting file size.
+ */
+ private long writeAndMeasure(boolean adaptive) throws IOException {
+ File file = new File(temp.toFile(), "test_" + adaptive + "_" + System.nanoTime() + ".parquet");
+ OutputFile outFile = Files.localOutput(file);
+
+ Parquet.WriteBuilder writer =
+ Parquet.write(outFile)
+ .schema(SCHEMA)
+ .createWriterFunc(ParquetAvroWriter::buildWriter)
+ .set(PARQUET_BLOOM_FILTER_COLUMN_ENABLED_PREFIX + "id", "true")
+ .set(PARQUET_BLOOM_FILTER_MAX_BYTES, "4194304");
+
+ if (adaptive) {
+ writer = writer.set(PARQUET_BLOOM_FILTER_ADAPTIVE_ENABLED, "true");
+ }
+
+ org.apache.avro.Schema avroSchema = AvroSchemaUtil.convert(SCHEMA.asStruct());
+ try (FileAppender<GenericData.Record> appender = writer.build()) {
+ for (int i = 0; i < 5; i++) {
+ appender.add(new GenericRecordBuilder(avroSchema).set("id", (long) i).build());
+ }
+ }
+
+ return file.length();
+ }
+
+ @Test
+ public void adaptiveSizingShrinksFile() throws IOException {
+ // Without adaptive, parquet-mr writes the full bloom-filter-max-bytes buffer (~4 MiB).
Review Comment:
Done — switched to the Iceberg property constants.
##########
parquet/src/test/java/org/apache/iceberg/parquet/TestParquetAdaptiveBloomFilter.java:
##########
@@ -0,0 +1,119 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.parquet;
+
+import static org.apache.iceberg.TableProperties.PARQUET_BLOOM_FILTER_ADAPTIVE_ENABLED;
+import static org.apache.iceberg.TableProperties.PARQUET_BLOOM_FILTER_COLUMN_ENABLED_PREFIX;
+import static org.apache.iceberg.TableProperties.PARQUET_BLOOM_FILTER_MAX_BYTES;
+import static org.apache.iceberg.types.Types.NestedField.required;
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.file.Path;
+import org.apache.avro.generic.GenericData;
+import org.apache.avro.generic.GenericRecordBuilder;
+import org.apache.iceberg.Files;
+import org.apache.iceberg.Schema;
+import org.apache.iceberg.avro.AvroSchemaUtil;
+import org.apache.iceberg.io.FileAppender;
+import org.apache.iceberg.io.OutputFile;
+import org.apache.iceberg.types.Types;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
+
+/**
+ * Tests for the adaptive bloom filter sizing TBLPROPERTY ({@code
+ * write.parquet.bloom-filter-adaptive-enabled}).
+ *
+ * <p>Without adaptive sizing, parquet-mr's {@code ColumnValueCollector.initBloomFilter} allocates a
+ * fixed {@code bloom-filter-max-bytes} buffer per bloom-enabled column regardless of actual NDV
+ * (the {@code !ndv.isPresent() && !adaptive} branch calls {@code new BlockSplitBloomFilter(
+ * maxBloomFilterSize, maxBloomFilterSize)}). For low-row-count writes this produces a file
+ * dominated by an empty bloom filter buffer. The adaptive flag (PARQUET-2254) makes parquet-mr pick
+ * the smallest of N candidate filter sizes that satisfies actual NDV at FPP.
+ *
+ * <p>These tests use the createWriterFunc code path because that is the path Iceberg uses for Spark
+ * and Flink data writes; the legacy ParquetWriteBuilder path goes through parquet-mr directly
+ * without consulting Iceberg-specific properties.
+ */
+public class TestParquetAdaptiveBloomFilter {
+
+ private static final Schema SCHEMA = new Schema(required(1, "id", Types.LongType.get()));
+
+ @TempDir private Path temp;
+
+ /**
+ * Writes a parquet file via the createWriterFunc code path with bloom filter enabled on the
+ * {@code id} column and a 4 MiB max-bytes cap. Returns the resulting file size.
+ */
+ private long writeAndMeasure(boolean adaptive) throws IOException {
Review Comment:
Fair — the helper now just delegates to ParquetWritingTestUtils.write and returns the written length.
##########
docs/docs/configuration.md:
##########
@@ -57,8 +56,8 @@ Iceberg tables support table properties to configure table behavior, like the de
| write.parquet.bloom-filter-max-bytes | 1048576 (1 MB) | The maximum number of bytes for a bloom filter bitset |
| write.parquet.bloom-filter-fpp.column.col1 | 0.01 | The false positive probability for a bloom filter applied to 'col1' (must > 0.0 and < 1.0) |
| write.parquet.bloom-filter-ndv.column.col1 | (not set) | The expected number of distinct values for a bloom filter applied to 'col1' (must > 0) |
+| write.parquet.bloom-filter-adaptive-enabled | false | Use parquet-mr adaptive bloom filter sizing (PARQUET-2254): evaluate candidate filters and pick the smallest that satisfies the actual NDV at the configured FPP, bounded by 'bloom-filter-max-bytes', instead of always allocating the full max-bytes buffer |
Review Comment:
Good point — added.
--
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
Re: [PR] Parquet: add adaptive bloom filter sizing (PARQUET-2326) [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #16363:
URL: https://github.com/apache/iceberg/pull/16363#discussion_r3450634505
##########
parquet/src/test/java/org/apache/iceberg/parquet/TestParquetAdaptiveBloomFilter.java:
##########
@@ -0,0 +1,119 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.parquet;
+
+import static org.apache.iceberg.TableProperties.PARQUET_BLOOM_FILTER_ADAPTIVE_ENABLED;
+import static org.apache.iceberg.TableProperties.PARQUET_BLOOM_FILTER_COLUMN_ENABLED_PREFIX;
+import static org.apache.iceberg.TableProperties.PARQUET_BLOOM_FILTER_MAX_BYTES;
+import static org.apache.iceberg.types.Types.NestedField.required;
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.file.Path;
+import org.apache.avro.generic.GenericData;
+import org.apache.avro.generic.GenericRecordBuilder;
+import org.apache.iceberg.Files;
+import org.apache.iceberg.Schema;
+import org.apache.iceberg.avro.AvroSchemaUtil;
+import org.apache.iceberg.io.FileAppender;
+import org.apache.iceberg.io.OutputFile;
+import org.apache.iceberg.types.Types;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
+
+/**
+ * Tests for the adaptive bloom filter sizing TBLPROPERTY ({@code
+ * write.parquet.bloom-filter-adaptive-enabled}).
+ *
+ * <p>Without adaptive sizing, parquet-mr's {@code ColumnValueCollector.initBloomFilter} allocates a
+ * fixed {@code bloom-filter-max-bytes} buffer per bloom-enabled column regardless of actual NDV
+ * (the {@code !ndv.isPresent() && !adaptive} branch calls {@code new BlockSplitBloomFilter(
+ * maxBloomFilterSize, maxBloomFilterSize)}). For low-row-count writes this produces a file
+ * dominated by an empty bloom filter buffer. The adaptive flag (PARQUET-2326) makes parquet-mr pick
+ * the smallest of N candidate filter sizes that satisfies actual NDV at FPP.
+ *
+ * <p>These tests use the createWriterFunc code path because that is the path Iceberg uses for Spark
+ * and Flink data writes; the legacy ParquetWriteBuilder path goes through parquet-mr directly
+ * without consulting Iceberg-specific properties.
+ */
+public class TestParquetAdaptiveBloomFilter {
+
+ private static final Schema SCHEMA = new Schema(required(1, "id", Types.LongType.get()));
+
+ @TempDir private Path temp;
+
+ /**
+ * Writes a parquet file via the createWriterFunc code path with bloom filter enabled on the
+ * {@code id} column and a 4 MiB max-bytes cap. Returns the resulting file size.
+ */
+ private long writeAndMeasure(boolean adaptive) throws IOException {
+ File file = new File(temp.toFile(), "test_" + adaptive + "_" + System.nanoTime() + ".parquet");
+ OutputFile outFile = Files.localOutput(file);
+
+ Parquet.WriteBuilder writer =
+ Parquet.write(outFile)
+ .schema(SCHEMA)
+ .createWriterFunc(ParquetAvroWriter::buildWriter)
+ .set(PARQUET_BLOOM_FILTER_COLUMN_ENABLED_PREFIX + "id", "true")
+ .set(PARQUET_BLOOM_FILTER_MAX_BYTES, "4194304");
+
+ if (adaptive) {
+ writer = writer.set(PARQUET_BLOOM_FILTER_ADAPTIVE_ENABLED, "true");
+ }
+
+ org.apache.avro.Schema avroSchema = AvroSchemaUtil.convert(SCHEMA.asStruct());
+ try (FileAppender<GenericData.Record> appender = writer.build()) {
+ for (int i = 0; i < 5; i++) {
+ appender.add(new GenericRecordBuilder(avroSchema).set("id", (long) i).build());
+ }
+ }
+
+ return file.length();
+ }
+
+ @Test
+ public void testAdaptiveSizingShrinksFile() throws IOException {
+ // Without adaptive, parquet-mr writes the full bloom-filter-max-bytes buffer (~4 MiB).
+ long sizeWithoutAdaptive = writeAndMeasure(false);
+
+ // With adaptive, parquet-mr picks the smallest of N candidate filter sizes that fits actual
+ // NDV (5 values). The exact candidate depends on parquet-mr's sizing strategy; for very low
+ // NDV the smallest candidate is selected.
+ long sizeWithAdaptive = writeAndMeasure(true);
+
+ assertThat(sizeWithoutAdaptive)
+ .as("non-adaptive file should pad bloom buffer close to bloom-filter-max-bytes (~4 MiB)")
+ .isGreaterThan(3_500_000L);
+ assertThat(sizeWithAdaptive)
+ .as("adaptive file should be at least 2x smaller than the non-adaptive file")
+ .isLessThan(sizeWithoutAdaptive / 2);
+ }
+
+ @Test
+ public void testAdaptiveDisabledByDefault() throws IOException {
Review Comment:
please omit `test` prefixes from new tests
--
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
Re: [PR] Parquet: add adaptive bloom filter sizing (PARQUET-2254) [iceberg]
Posted by "raghav-reglobe (via GitHub)" <gi...@apache.org>.
raghav-reglobe commented on PR #16363:
URL: https://github.com/apache/iceberg/pull/16363#issuecomment-4917169252
Added an AI usage disclosure section to the PR description, and pushed the rest of the review feedback: the tests moved into TestParquet, the test comments now use the Iceberg property constants, the TableProperties comment is gone (configuration.md documents the property), and the docs row mentions that per-column NDV takes precedence over adaptive sizing.
--
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
Re: [PR] Parquet: add adaptive bloom filter sizing (PARQUET-2254) [iceberg]
Posted by "raghav-reglobe (via GitHub)" <gi...@apache.org>.
raghav-reglobe commented on code in PR #16363:
URL: https://github.com/apache/iceberg/pull/16363#discussion_r3559201647
##########
parquet/src/test/java/org/apache/iceberg/parquet/TestParquet.java:
##########
@@ -516,6 +341,52 @@ public void testAvroWriterRejectsVariantType() {
.hasMessage("Avro writer does not support variant types");
}
+ @Test
+ public void adaptiveBloomFilterSizingShrinksFile() throws IOException {
+ // when PARQUET_BLOOM_FILTER_ADAPTIVE_ENABLED is not set (the default), the writer
+ // allocates the full PARQUET_BLOOM_FILTER_MAX_BYTES buffer (4 MiB here) regardless
+ // of the number of distinct values written
+ long sizeWithoutAdaptive = writeWithBloomFilter(false);
+ assertThat(sizeWithoutAdaptive)
+ .as("non-adaptive file should pad the bloom filter to PARQUET_BLOOM_FILTER_MAX_BYTES")
+ .isGreaterThan(3_500_000L);
+
+ // with PARQUET_BLOOM_FILTER_ADAPTIVE_ENABLED, the writer picks the smallest candidate
+ // bloom filter that satisfies the actual number of distinct values (5) at the
+ // configured FPP
+ long sizeWithAdaptive = writeWithBloomFilter(true);
+ assertThat(sizeWithAdaptive)
+ .as("adaptive file should be at least 2x smaller than the non-adaptive file")
+ .isLessThan(sizeWithoutAdaptive / 2);
+ }
+
+ private long writeWithBloomFilter(boolean adaptiveEnabled) throws IOException {
+ Schema schema = new Schema(required(1, "id", Types.LongType.get()));
+
+ ImmutableMap.Builder<String, String> propsBuilder =
+ ImmutableMap.<String, String>builder()
+ .put(PARQUET_BLOOM_FILTER_COLUMN_ENABLED_PREFIX + "id", "true")
+ .put(PARQUET_BLOOM_FILTER_MAX_BYTES, "4194304");
+ if (adaptiveEnabled) {
+ propsBuilder.put(PARQUET_BLOOM_FILTER_ADAPTIVE_ENABLED, "true");
+ }
Review Comment:
Went with the Boolean variant — `null` now means unset, so the baseline write genuinely exercises the not-set default path (which its comment claims), and true/false set the property explicitly.
--
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
Re: [PR] Parquet: add adaptive bloom filter sizing (PARQUET-2254) [iceberg]
Posted by "raghav-reglobe (via GitHub)" <gi...@apache.org>.
raghav-reglobe commented on code in PR #16363:
URL: https://github.com/apache/iceberg/pull/16363#discussion_r3558756790
##########
parquet/src/test/java/org/apache/iceberg/parquet/TestParquet.java:
##########
@@ -516,6 +341,63 @@ public void testAvroWriterRejectsVariantType() {
.hasMessage("Avro writer does not support variant types");
}
+ @Test
+ public void adaptiveBloomFilterSizingShrinksFile() throws IOException {
+ // without PARQUET_BLOOM_FILTER_ADAPTIVE_ENABLED, the writer allocates the full
+ // PARQUET_BLOOM_FILTER_MAX_BYTES buffer (4 MiB here) regardless of the number
+ // of distinct values written
+ long sizeWithoutAdaptive = writeWithBloomFilter(false);
+ assertThat(sizeWithoutAdaptive)
+ .as("non-adaptive file should pad the bloom filter to PARQUET_BLOOM_FILTER_MAX_BYTES")
+ .isGreaterThan(3_500_000L);
+
+ // with PARQUET_BLOOM_FILTER_ADAPTIVE_ENABLED, the writer picks the smallest candidate
+ // bloom filter that satisfies the actual number of distinct values (5) at the
+ // configured FPP
+ long sizeWithAdaptive = writeWithBloomFilter(true);
+ assertThat(sizeWithAdaptive)
+ .as("adaptive file should be at least 2x smaller than the non-adaptive file")
+ .isLessThan(sizeWithoutAdaptive / 2);
+ }
+
+ @Test
+ public void adaptiveBloomFilterDisabledByDefault() throws IOException {
+ // when PARQUET_BLOOM_FILTER_ADAPTIVE_ENABLED is not set, the existing behavior is
+ // preserved: the full PARQUET_BLOOM_FILTER_MAX_BYTES buffer is allocated
+ long size = writeWithBloomFilter(false);
+
+ assertThat(size)
+ .as("default write should pad the bloom filter to PARQUET_BLOOM_FILTER_MAX_BYTES")
+ .isGreaterThan(3_500_000L);
Review Comment:
Semi-intentional — it was meant to pin the default (property-not-set) behavior as its own test, but you're right that it duplicated the baseline of the test above verbatim. Merged them: the non-adaptive write in `adaptiveBloomFilterSizingShrinksFile` is the default path, and its comment now says so.
--
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
Re: [PR] Parquet: add adaptive bloom filter sizing (PARQUET-2254) [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #16363:
URL: https://github.com/apache/iceberg/pull/16363#discussion_r3535180328
##########
parquet/src/test/java/org/apache/iceberg/parquet/TestParquetAdaptiveBloomFilter.java:
##########
@@ -0,0 +1,119 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.parquet;
+
+import static org.apache.iceberg.TableProperties.PARQUET_BLOOM_FILTER_ADAPTIVE_ENABLED;
+import static org.apache.iceberg.TableProperties.PARQUET_BLOOM_FILTER_COLUMN_ENABLED_PREFIX;
+import static org.apache.iceberg.TableProperties.PARQUET_BLOOM_FILTER_MAX_BYTES;
+import static org.apache.iceberg.types.Types.NestedField.required;
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.file.Path;
+import org.apache.avro.generic.GenericData;
+import org.apache.avro.generic.GenericRecordBuilder;
+import org.apache.iceberg.Files;
+import org.apache.iceberg.Schema;
+import org.apache.iceberg.avro.AvroSchemaUtil;
+import org.apache.iceberg.io.FileAppender;
+import org.apache.iceberg.io.OutputFile;
+import org.apache.iceberg.types.Types;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
+
+/**
+ * Tests for the adaptive bloom filter sizing TBLPROPERTY ({@code
+ * write.parquet.bloom-filter-adaptive-enabled}).
+ *
+ * <p>Without adaptive sizing, parquet-mr's {@code ColumnValueCollector.initBloomFilter} allocates a
+ * fixed {@code bloom-filter-max-bytes} buffer per bloom-enabled column regardless of actual NDV
+ * (the {@code !ndv.isPresent() && !adaptive} branch calls {@code new BlockSplitBloomFilter(
+ * maxBloomFilterSize, maxBloomFilterSize)}). For low-row-count writes this produces a file
+ * dominated by an empty bloom filter buffer. The adaptive flag (PARQUET-2254) makes parquet-mr pick
+ * the smallest of N candidate filter sizes that satisfies actual NDV at FPP.
+ *
+ * <p>These tests use the createWriterFunc code path because that is the path Iceberg uses for Spark
+ * and Flink data writes; the legacy ParquetWriteBuilder path goes through parquet-mr directly
+ * without consulting Iceberg-specific properties.
+ */
+public class TestParquetAdaptiveBloomFilter {
Review Comment:
Any specific reason why this is not part of the `TestParquet`?
If we create a new test class please make the test class and methods package private.
--
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
Re: [PR] Parquet: add adaptive bloom filter sizing (PARQUET-2254) [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #16363:
URL: https://github.com/apache/iceberg/pull/16363#discussion_r3529798405
##########
core/src/main/java/org/apache/iceberg/TableProperties.java:
##########
@@ -176,14 +176,21 @@ private TableProperties() {}
"write.delete.parquet.row-group-check-max-record-count";
public static final int PARQUET_ROW_GROUP_CHECK_MAX_RECORD_COUNT_DEFAULT = 10000;
- public static final String PARQUET_ROW_GROUP_SIZE_TRACK_UNCOMPRESSED =
- "write.parquet.row-group-size-track-uncompressed";
- public static final boolean PARQUET_ROW_GROUP_SIZE_TRACK_UNCOMPRESSED_DEFAULT = false;
-
public static final String PARQUET_BLOOM_FILTER_MAX_BYTES =
"write.parquet.bloom-filter-max-bytes";
public static final int PARQUET_BLOOM_FILTER_MAX_BYTES_DEFAULT = 1024 * 1024;
+ // Enables parquet-mr's adaptive bloom filter sizing (PARQUET-2254). When true,
Review Comment:
```suggestion
// EnablesParquet's adaptive bloom filter sizing. When true,
```
##########
core/src/main/java/org/apache/iceberg/TableProperties.java:
##########
@@ -176,14 +176,21 @@ private TableProperties() {}
"write.delete.parquet.row-group-check-max-record-count";
public static final int PARQUET_ROW_GROUP_CHECK_MAX_RECORD_COUNT_DEFAULT = 10000;
- public static final String PARQUET_ROW_GROUP_SIZE_TRACK_UNCOMPRESSED =
- "write.parquet.row-group-size-track-uncompressed";
- public static final boolean PARQUET_ROW_GROUP_SIZE_TRACK_UNCOMPRESSED_DEFAULT = false;
-
public static final String PARQUET_BLOOM_FILTER_MAX_BYTES =
"write.parquet.bloom-filter-max-bytes";
public static final int PARQUET_BLOOM_FILTER_MAX_BYTES_DEFAULT = 1024 * 1024;
+ // Enables parquet-mr's adaptive bloom filter sizing (PARQUET-2254). When true,
Review Comment:
```suggestion
// Enables Parquet's adaptive bloom filter sizing. When true,
```
--
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