Skip to content

test: exercise the Iceberg write split-operator plan in Iceberg's own suites - #5640

Merged
andygrove merged 1 commit into
apache:mainfrom
andygrove:fix-5259-enable-iceberg-split-operator
Sep 4, 2026
Merged

test: exercise the Iceberg write split-operator plan in Iceberg's own suites#5640
andygrove merged 1 commit into
apache:mainfrom
andygrove:fix-5259-enable-iceberg-split-operator

Conversation

@andygrove

Copy link
Copy Markdown
Member

Which issue does this PR close?

Closes #5259.

Rationale for this change

#5259 catalogued four buckets of CI failures from enabling Comet's Iceberg write
split-operator plan (IcebergCommit -> IcebergWrite) by default. It was filed on
2026-08-04, three days before #4658 merged, so it audited an in-review state of that
PR. All four buckets have since been fixed:

Bucket Where it was fixed
1. CometIcebergRewriteActionSuite asserts on AppendData #5361 — the plan filter now accepts AppendData or IcebergCommit (CometIcebergRewriteActionSuite.scala:282, :343-346)
2. Iceberg's tests cast the plan root to V2TableWriteExec All four Iceberg diffs patch TestSystemFunctionPushDownInRowLevelOperations.executeAndCollectFunctionCalls to unwrap
3. spark.merge-into.* missing from the snapshot summary IcebergWriteSummaryShim (Spark 4.1+) collects MergeRowsExec metrics and passes MergeSummaryImpl to BatchWrite.commit
4. TestCachedTableRefresh schema-change staleness IcebergRefreshCacheShim (Spark 4.1+) recaches by name via recacheTableOrView rather than by plan

What has not been fixed is the coverage gap the issue opens with: "Because the feature
ships off, nothing in CI exercises it."
Comet's own suites do set the flag
(CometIcebergWriteActionSuite, CometIcebergWriteDetectionSuite,
CometIcebergRewriteActionSuite), which covers bucket 1. But the Iceberg diffs inject
only two Comet configs — spark.comet.explainFallback.enabled and
spark.comet.scan.icebergNative.enabled — so Iceberg's own suites still run with the
split operator off. That is exactly where buckets 2, 3, and 4 failed, and those three
fixes are currently unverified by CI.

What changes are included in this PR?

Add .config("spark.comet.write.iceberg.splitOperator.enabled", "true") next to the
existing native-scan config in every Comet-configured SparkSession builder across the
four Iceberg test diffs — 63 sites each on 1.8.1 / 1.9.1 / 1.10.0 (which patch both
spark/v3.4 and spark/v3.5) and 31 on 1.11.0.

No user-facing default changes. spark.comet.write.iceberg.splitOperator.enabled
stays false in CometConf, and remains CATEGORY_TESTING. Flipping the default is a
separate decision — worth taking deliberately, since bucket 2 is a live example of
third-party code that pattern-matches on the stock write plan shape and breaks.

Two things a reviewer may want to push back on:

  • The config is added to the five src/jmh benchmark files too, not just src/test,
    matching how scan.icebergNative.enabled is already applied everywhere. Those aren't
    run in CI. The argument for including them is consistency and that someone
    benchmarking Comet Iceberg writes wants the plan Comet actually produces; happy to
    restrict to src/test if preferred.
  • The diffs are regenerated, not hand-edited, per the process in
    docs/source/contributor-guide/iceberg-spark-tests.md. That normalises a pre-existing
    anomaly and accounts for most of the line churn: the
    TestSystemFunctionPushDownInRowLevelOperations hunks (bucket 2's fix) had been
    appended at the end of each diff rather than sorted into git's canonical path order.
    Generating with --abbrev=10 keeps the index lines of unmodified files byte-identical,
    so index churn is limited to the files actually touched.

Also documents the new config in the diff-overview list in
docs/source/contributor-guide/iceberg-spark-tests.md.

How are these changes tested?

CI is the test — that is the point of the change. dev/diffs/iceberg/** is in the path
filter for all four Iceberg jobs in dev/ci/compute-changes.py, so iceberg_1_11 runs
automatically; this PR needs the run-iceberg-tests label to also cover 1.8 / 1.9 /
1.10, since the Spark 3.4 and 3.5 diffs are only exercised there.

No Comet source is touched, so Comet's own suites are unaffected.

The regenerated diffs were verified mechanically rather than by inspection. For each
version, the committed diff and the regenerated diff were each applied to a clean
checkout of the matching apache-iceberg-<version> tag and the resulting trees compared:

=== 1.8.1 ===   59 files changed, 63 insertions(+)
=== 1.9.1 ===   59 files changed, 63 insertions(+)
=== 1.10.0 ===  59 files changed, 63 insertions(+)
=== 1.11.0 ===  31 files changed, 31 insertions(+)

Zero removed lines, and every added line is the split-operator config — so the
regeneration is semantically identical to the previous diffs plus this change, despite
the reordering churn in the diff files themselves.

spotlessApply was not run against the Iceberg clones (it needs the full Gradle
toolchain). It would be a no-op here: each inserted line is a copy of the adjacent
.config(...) line with the same indentation, is at most 78 columns against a 100-column
limit, and all 220 insertions land mid-chain in a fluent builder (verified: none of the
anchor lines terminate with ;).

… suites

The split-operator plan (IcebergCommit -> IcebergWrite) ships off by default,
so Iceberg's Spark suites have never run against it. All four failure buckets
catalogued in apache#5259 have since been fixed, but nothing in CI covers three of
them.

Enable spark.comet.write.iceberg.splitOperator.enabled in every Comet-configured
SparkSession builder in the Iceberg test diffs, alongside the existing native
scan config. No user-facing default changes.

The diffs are regenerated rather than hand-edited, which also restores canonical
path ordering: the TestSystemFunctionPushDownInRowLevelOperations hunks had been
appended at the end of each diff instead of sorted into place.

@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.

Thanks for closing the coverage gap. I independently applied the old and new patches to all four matching Iceberg tags and confirmed exactly 220 config insertions, with every other resulting source byte and file mode preserved. The split-plan test helper remains intact, and the user-facing default stays false. I found no actionable P1/P2 issue. This was source and mechanical validation, not a local Iceberg suite run. Runtime integration checks are successful in the supplied CI snapshot, while the core and extension suites are still running.

@andygrove
andygrove merged commit 9cac716 into apache:main Sep 4, 2026
47 checks passed
@andygrove
andygrove deleted the fix-5259-enable-iceberg-split-operator branch September 4, 2026 10:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Iceberg write split-operator plan: CI failures when enabled by default

2 participants