andygrove opened a new pull request, #5640:
URL: https://github.com/apache/datafusion-comet/pull/5640
## 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 `;`).
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
