Skip to content

feat: support bloom filters in native Iceberg writes - #5724

Open
NikitaMatskevich wants to merge 33 commits into
apache:mainfrom
NikitaMatskevich:feat/iceberg-bloom-filter-writes
Open

NikitaMatskevich wants to merge 33 commits into
apache:mainfrom
NikitaMatskevich:feat/iceberg-bloom-filter-writes

Conversation

@NikitaMatskevich

@NikitaMatskevich NikitaMatskevich commented Sep 5, 2026 •

Copy link
Copy Markdown

Which issue does this PR close?

Partially addresses #5643 and #5649. Does not close the generic Parquet writer issue #5304.

Rationale for this change

Enable native Iceberg writes for tables with column Bloom filters on identifiable Iceberg 1.10+ runtimes. Older or unidentified runtimes use Java whenever any per-column Bloom property is configured, including false enablement and standalone FPP/NDV. Tables without those properties retain their existing native-write eligibility.

The default-off spark.comet.write.iceberg.bloomFilterFolding.enabled option accepts parquet-rs's folding behavior. Sparse filters can shrink toward the requested FPP, trading serialized space for potentially weaker pruning than Java's larger filters. The flag selects the writer; it cannot disable folding in the pinned parquet-rs implementation.

We are also pursuing an upstream parquet-rs option to disable folding, which would simplify compatibility with Java by retaining the initial filter size. The current Comet opt-in accepts folding; it does not disable it.

What changes are included in this PR?

  • Translate effective per-column enablement, FPP, and optional NDV through grouped protobuf settings, with Java-compatible initial allocation and conservative numeric/cap guards.
  • Keep FLOAT/DOUBLE Bloom writes on Java to preserve NaN equality. Unsupported paths and settings also use Java.
  • Restrict this PR to modern Iceberg naming semantics; remove legacy raw-path/alias resolution from production code. Legacy native Bloom support is split into feat: support native Iceberg Bloom writes on older runtimes #7226, whose branch is based on this PR.
  • Document the opt-in, pruning and boolean differences, eligibility conditions, and pinned-profile compatibility matrix under Accepted divergences.
  • Extend the existing Iceberg write benchmark with native Bloom off/on cases and document independent JVM timing/RSS collection using standard tooling.

How are these changes tested?

The PR adds real-file coverage for allocation, folding/FPP behavior, physical-type byte parity and membership, writer selection, and Java fallback. Local verification of the rebased parent includes Spark 4.1 / Iceberg 1.11 translation and Bloom eligibility regressions plus semantic Scalafix. JVM/native builds pass; #7226 carries the additional legacy-runtime coverage.

Full CI is pending: GitHub currently rejects fork workflow dispatches with an Actions-disabled response.

Benchmark results

Following Andy Grove’s suggestion, the existing CometIcebergWriteBenchmark compares native writes with Bloom filters off versus on at the default 1 MiB cap per column. Bloom filters are enabled on the bigint, string, and binary columns.

Measured on 2026-10-25 with a release native build; Spark 4.1 / Iceberg 1.11; JDK 21.0.6; Apple M4 Max, 64 GiB RAM; local[5], 8 GiB JVM heap. Each case writes 4,194,304 rows, with five measured iterations in a separate JVM.

Native writer Average write time Standard deviation Whole-process peak RSS
Bloom filters off 647 ms 20 ms 3,056 MiB
Bloom filters on 686 ms 21 ms 2,930 MiB

Bloom filters added approximately 6% to average write time in this local run. Peak RSS includes Maven/JVM startup, source generation, validation, and warmup; the lower observed RSS with filters enabled does not establish a memory saving. These measurements compare the native writer with filters off/on; they do not measure Java/native writer speed or reader pruning.

@andygrove andygrove added enhancement New feature or request area:Iceberg area:writer Native Parquet writer labels Sep 6, 2026
@NikitaMatskevich
NikitaMatskevich force-pushed the feat/iceberg-bloom-filter-writes branch 15 times, most recently from a9254ed to cd3042f Compare September 7, 2026 09:54
@NikitaMatskevich
NikitaMatskevich marked this pull request as ready for review September 7, 2026 09:55

@alessandro-nori alessandro-nori left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

found one divergence from iceberg Java, the rest of the implementation looks good to me

@NikitaMatskevich
NikitaMatskevich force-pushed the feat/iceberg-bloom-filter-writes branch from cd3042f to f86e8e4 Compare September 7, 2026 14:19
@NikitaMatskevich
NikitaMatskevich force-pushed the feat/iceberg-bloom-filter-writes branch from f86e8e4 to 1d8376b Compare September 7, 2026 15:30

@unikdahal unikdahal left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the detailed work here. The nested list/map physical-path fix looks good.

I found three remaining parquet-mr compatibility gaps in Bloom-filter property handling/sizing. Details inline.

@NikitaMatskevich
NikitaMatskevich force-pushed the feat/iceberg-bloom-filter-writes branch from 1d8376b to 7d218de Compare September 7, 2026 19:42

@unikdahal unikdahal left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing the earlier comments. I re-checked the latest changes, and those issues look resolved.

I found one remaining cross-version test issue and left an inline comment on it. Other than that, the implementation LGTM.

Also, could you please rebase against main to resolve the merge conflicts?

@sunchao sunchao left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correctness

Reviewed 7d218de7 against authoritative base 7f1e0018, with authored changes measured from merge base 719cba11. Previously, an enabled Bloom-filter property forced Iceberg writes onto the JVM path. The PR lifts that restriction, maps configured columns to Parquet paths, carries FPP/NDV/max-byte settings through protobuf, and configures the native writer. Its eligibility checks keep unsupported runtime properties, unrepresentable caps and problematic numeric values on the classic writer.

The previous discussions about enabled=false plus NDV, the 32-byte cap and overflowing NDV are addressed in the current control flow. NDV can re-enable a configured column, associated malformed values are checked even when enabled is false, and the two sizing boundaries fall back. The list/map path adjustment also addresses the previously reported missing intermediate path components.

One new [P2] remains: the new map assumes Iceberg Java's physical field names are also the native writer's names. Java sanitizes names such as order id to order_x20id, while the pinned native schema conversion preserves order id. The native Bloom-property lookup therefore misses the configured column. The inline comment requests matching the native writer's schema or falling back for renamed fields.

The maintained Spark 3.5 and 4.0 parsers accept backquoted names containing spaces and punctuation, and their schemas retain the names. This is a valid column shape, not an invalid-name input. Spark's V2 commit/abort protocol is unchanged by this PR. Bloom filters affect pruning rather than row values, so correct row results alone cannot establish that these properties were honored.

The existing cross-version test issue also remains. Both suites' shared assumption checks only the FPP constant. Iceberg 1.8.1 and 1.10.0 expose that constant but lack NDV support, so the assumption passes while production correctly rejects explicit NDV. NDV cases that expect native execution or Compatible need a separate capability assumption. I have not duplicated that inline comment.

Validation

No check runs, statuses or Actions runs were present for this head in the fresh check at 2026-09-08 04:40:59 UTC. No CI checkout, merge-tree equivalence or native artifact provenance could therefore be credited. The author's local-suite statement is not independently verified. The assigned base has advanced beyond this branch's merge base, including overlapping writer/reflection/test changes that must survive the requested rebase.

A bounded component test compiled the exact Java name-sanitizing methods, parquet-rs column-path identity/conversion and Comet path/sizing functions. Plain names matched. Three sanitized names missed the native column keys, and controls using native paths matched. It also passed 115 allocation round trips across supported powers of two and representative FPPs, plus default, underestimated-NDV, binding-cap and pathological-FPP controls. This used a simple property-map fixture and a Java precondition stub. It was not a complete Parquet-file or Spark/JNI reproduction. No local Comet build, Spark suite or performance benchmark was run. Maintained Spark 3.4/4.1 source branches were unavailable.

Performance

The default requests up to 1 MiB per configured column per row group, with larger supported caps up to 128 MiB. Allocation and hashing occur in the native writer, and fanout can multiply concurrent allocations. The sizing translation preserves the intended cap/NDV precedence in the inspected cases. The pinned parquet-rs encoder folds filters on flush, so final file size can differ from initial allocation.

The new path mismatch silently loses requested Bloom pruning for affected names. Apart from that finding, I found no material unnecessary work introduced by the translation. Schema-path reflection is performed during driver serialization, and the inverse sizing usually takes one candidate with a bounded search fallback. File-size assertions and sizing calculations do not establish write-throughput or downstream query gains.

Design

Separating eligibility, protobuf translation and native writer-property construction keeps fallback decisions before task execution. Explicit defaults prevent parquet-rs's different FPP default from leaking into Iceberg writes, and preserving absent NDV is necessary for the JVM sizing precedence. The physical-path mapping is the weak boundary: its source must agree with the schema actually emitted by the native writer.

Abstraction & complexity

The synthetic NDV is justified by the pinned API's lack of a separate byte-cap setter. The inverse calculation verifies its result, and the numeric search is bounded. The JVM representability check duplicates part of that sizing logic, but it serves a distinct purpose by rejecting unsupported settings before execution. Keeping those checks tied to dependency versions and boundary tests is necessary. Beyond correcting the path mapping, I found no actionable abstraction change.

@NikitaMatskevich
NikitaMatskevich force-pushed the feat/iceberg-bloom-filter-writes branch from 9875a7b to fcef2c4 Compare September 8, 2026 09:55
@NikitaMatskevich
NikitaMatskevich force-pushed the feat/iceberg-bloom-filter-writes branch from 08b480f to c978871 Compare November 10, 2026 13:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:Iceberg area:writer Native Parquet writer enhancement New feature or request run-all-spark-profiles Run the Comet test suites against every Spark profile on this pull request, ahead of the merge queue run-iceberg-tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants