Skip to content

feat: Optionally split the Iceberg V2 write operator into distinct writer and committer operations - #4658

Open
jordepic wants to merge 5 commits into
apache:mainfrom
jordepic:iceberg-writes-split-c1
Open

feat: Optionally split the Iceberg V2 write operator into distinct writer and committer operations#4658
jordepic wants to merge 5 commits into
apache:mainfrom
jordepic:iceberg-writes-split-c1

Conversation

@jordepic

@jordepic jordepic commented Jun 15, 2026

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

(Partially, first of three) Closes #4322.

Rationale for this change

Iceberg spark writes are V2 operators and contain the functionality for writing data files, metadata files, and committing to the catalog. Ultimately, Comet is only well-positioned to just accelerate data file writing (assuming they're parquet files). It is also crucial to ensure that the actual data file writing piece of the spark plan for iceberg writing is included within the AQE block of a spark plan, thereby ensuring that we re-plan writes in response to runtime decisions regarding its upstream operators.

Our split is fairly simple - we write the data files like normal in the "writer" operator, serialize its output, and pass it back to the "committer" operator. In the future, we'll target just the "writer" operator for speedup with iceberg-rust.

What changes are included in this PR?

This PR contains 5 commits.

  1. Docs outlining the WHOLE iceberg-write acceleration feature, not just these changes (I'm happy to modify/remove as needed).
  2. Planning rules to move iceberg append and overwrite operations to our "split operator" design.
  3. Planning rules to move iceberg delete, update, and merge operations to our "split operator" design.
  4. Tests for part 2
  5. Tests for part 3

How are these changes tested?

We have unit tests for each operator that we're replacing that ensures that the plan shape is correct, we commit to our iceberg table the proper number of times, and our iceberg table end state is correct when we scan it after a write operation. I've been running with these changes locally now and they're all performing as expected as well.

@jordepic

Copy link
Copy Markdown
Contributor Author

Let me know what you think @mbutrovich , @comphead !

@andygrove

Copy link
Copy Markdown
Member

Thanks for splitting this out of #4487, @jordepic. This is a much better review target, and the split is clean. (Reviewed with LLM assistance.)

I traced the commit protocol against Spark's own V2 write and it is a faithful reproduction. The writer does per-task commit / abort / close via tryWithSafeFinallyAndFailureCallbacks, and the committer extends V2CommandExec so run() executes once for exactly-once commit, calling onDataWriterCommit per message then commit(messages), with abort(messages) plus addSuppressed on failure. The shared BatchWrite instance between writer and committer is the subtle detail that makes commit-time validation work, and I appreciate that it is documented. The fallback is safe too: the strategy returns Nil when the config is off or the plan is not an Iceberg write, so the worst case is no acceleration rather than a corrupted table, and that path is tested. Nice test coverage overall, especially the empty-source single-commit and the disabled-config fallthrough cases.

A few things I would like to resolve before approving, none of them large:

1. The useCommitCoordinator() guard fails hard at execution time. In IcebergWriteExec.doExecute the require(!batchWrite.useCommitCoordinator(), ...) throws mid-execution if an Iceberg catalog returns a BatchWrite that needs Spark's commit coordinator, which fails the whole query after the user has opted in. Since the strategy already has the Write at planning time, would it be cleaner to check this in matchedSparkWrite / buildTwoOp and return None there, so an unsupported write shape falls back to Spark's normal path instead of hard-failing?

2. AQE exactly-once confidence. The design wraps the writer child in IcebergWriteLogical so AQE re-plans only the writer, and the exchange/sort and empty-source tests exercise this indirectly. Could you confirm, or point at a test that asserts, that AQE re-planning the writer subtree cannot cause data files to be written or committed twice? The commit-count assertions look like they cover it, I just want to make sure that is the intent.

3. Docs scope. You flagged this yourself. Since this PR does not ship the native write path yet, could we trim iceberg-writes.md to what is actually enabled here, or mark the native sections as upcoming? Otherwise the merged docs describe functionality a user cannot turn on yet.

One minor note: commit messages are serialized with ObjectOutputStream into a BinaryType column. That is fine since Iceberg's messages are Serializable, just worth being aware it couples the wire format to Java serialization across the supported Iceberg versions.

Also a small process point: once these splits land, it is probably worth closing #4487 (it is currently conflicting and stale) so the splits become the source of truth.

@andygrove

Copy link
Copy Markdown
Member

Thanks for your patience with this @jordepic. I posted my AI review above. My human review is that I am in favor of getting this merged since it is gated behind a config that is disabled by default. I would prefer that we describe the functionality as experimental in the documentation for now, until this is more widely tested by multiple users.

@andygrove

Copy link
Copy Markdown
Member

@mbutrovich @comphead Do you have any additional thoughts on this?

@jordepic

Copy link
Copy Markdown
Contributor Author

Thanks! Let me get to this now - I think barring some much more sluggish changes on the iceberg/spark side to either change the v2 writer operator this is as clean as it'll get.

@mbutrovich
mbutrovich self-requested a review July 24, 2026 15:05
@jordepic
jordepic force-pushed the iceberg-writes-split-c1 branch from 77030ec to 8126020 Compare July 24, 2026 15:26
@jordepic

Copy link
Copy Markdown
Contributor Author

Went ahead and fixed that feedback @andygrove .

Docs simplified to reflect just this PR. test added to ensure exactly once committing under AQE. Fall back to normal spark iceberg writing if commit coordination is required.

@andygrove

Copy link
Copy Markdown
Member

Went ahead and fixed that feedback @andygrove .

Docs simplified to reflect just this PR. test added to ensure exactly once committing under AQE. Fall back to normal spark iceberg writing if commit coordination is required.

Thanks @jordepic. I've triggered CI.

case class IcebergCommitExec(
// Neither of these fields are serialized, this is all run on the driver.
@transient batchWrite: BatchWrite,
@transient refreshCache: () => Unit,

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.

ideally to introduce some type for that

type RefreshCache = () => Unit

@transient
refreshCache: RefreshCache,

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Do you think it reads that much better? Just wanted to emphasize that this is a simple lambda. Fair point!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done -- added IcebergCommitExec.RefreshCache (type RefreshCache = () => Unit) and used it for the constructor param.

@comphead

Copy link
Copy Markdown
Contributor

Thanks @jordepic I haven't yet go through PR in details, however if PR is supposed to close original ticket stating

Our goal is to support writes to iceberg so that we never have to convert back from columnar input data to row oriented at any point in an iceberg operation!

We need to check in tests there is no fallback for iceberg writes and the entire plan is fully native, I didn't see this in tests. Maybe I just missed this

@jordepic

jordepic commented Jul 24, 2026

Copy link
Copy Markdown
Contributor Author

We need to check in tests there is no fallback for iceberg writes and the entire plan is fully native, I didn't see this in tests. Maybe I just missed this

@comphead sorry - to be clear, we will fallback to using the normal java writer if we can't support the write. I was just being idealistic in that commit message - iceberg-rs can't support everything. This particular PR doesn't invoke any of that logic yet since it is only one of three separate pieces of this change. I'll have two more to put up after this.

I updated the issue text to emphasize that we're trying to run the full plan using native code.

and the entire plan is fully native

I haven't added the native acceleration just yet so those tests aren't in this particular PR, but they will be in the final code!

@comphead

comphead commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

I'll have two more to put up after this.

I see, thanks for the clarification, would be that correct to state the PR partially closes #4322 ?

@jordepic

Copy link
Copy Markdown
Contributor Author

Modified to reflect that, thank you @comphead !

@jordepic jordepic changed the title Optionally split the Iceberg write/commit operator into separate writer and committer operations feat: Optionally split the Iceberg V2 write operator into distinct writer and committer operations Jul 24, 2026
@mbutrovich

mbutrovich commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

I want to see what happens if I enable this and run the Iceberg Java suites. I know we're discussing merging this as experimental and disabled by default, but I just want the smoke test first. I'll run that overnight.

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

First pass, thanks @jordepic!

override protected def doExecute(): RDD[InternalRow] = {
val rdd = child.execute()
val factory = batchWrite.createBatchWriterFactory(PhysicalWriteInfoImpl(rdd.getNumPartitions))
require(

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.

Is this require unreachable? buildTwoOp already returns None when useCommitCoordinator() is true (IcebergWriteStrategy.scala L107), and the AQE re-plan case derives from that same guarded path.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Correct -- with the planning-time fallback it's unreachable through IcebergWriteStrategy. I kept it as a backstop for direct construction of the node and marked it as such with a short comment. Happy to drop it instead if you'd rather not carry it.

Comment on lines +133 to +146
def serializeMessage(message: WriterCommitMessage): Array[Byte] = {
val bos = new ByteArrayOutputStream()
val oos = new ObjectOutputStream(bos)
try oos.writeObject(message)
finally oos.close()
bos.toByteArray
}

def deserializeMessage(bytes: Array[Byte]): WriterCommitMessage = {
val bis = new ByteArrayInputStream(bytes)
val ois = new ObjectInputStream(bis)
try ois.readObject().asInstanceOf[WriterCommitMessage]
finally ois.close()
}

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.

Would Spark's serializer bound to Spark's classloader be safer here than the raw ObjectOutputStream / ObjectInputStream round-trip? My concern is that raw ObjectInputStream resolves classes with the latest user-defined loader and may fail to find Iceberg classes under --packages, REPL, or child-classloader isolation. Comet already uses SparkEnv.get.serializer for this elsewhere (CometShuffleDependency.scala L58, CometBlockStoreShuffleReader.scala L50). What do you think about Utils.serialize / Utils.deserialize with Utils.getContextOrSparkClassLoader? It might also let us drop one of two serialization passes, since the message is Java-serialized here and then serialized again by Spark's result serializer during executeCollect.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch. Switched to Utils.serialize / Utils.deserialize(bytes, Utils.getContextOrSparkClassLoader), which resolves classes through Spark's context classloader and fixes the --packages/REPL case. I kept the explicit byte-array column rather than trying to drop one of the two passes: the writer's output has to cross an ordinary RDD[InternalRow] boundary (that's what makes it visible to AQE), so the row always goes through Spark's result serializer; the inner pass is what turns the opaque WriterCommitMessage into a stable binary column.

Comment on lines +96 to +111
def runWriter(
writer: DataWriter[InternalRow],
iter: Iterator[InternalRow],
rowsMetric: SQLMetric,
projection: UnsafeProjection,
replaceDataDispatch: Option[ReplaceDataDispatchInfo]): Iterator[InternalRow] = {
val message = Utils.tryWithSafeFinallyAndFailureCallbacks(block = {
if (replaceDataDispatch.isDefined) {
runReplaceDataWriter(writer, iter, replaceDataDispatch.get, rowsMetric)
} else {
while (iter.hasNext) {
writer.write(iter.next())
rowsMetric.add(1L)
}
}
writer.commit()

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.

Should we be flushing the writer's dataWriter.currentMetricsValues during and after iteration? Spark does this in WritingSparkTask via IteratorWithMetrics, and it looks like that is how Iceberg surfaces per-task write metrics. Since runWriter only increments numOutputRows, my read is that the Iceberg task write metrics are lost when the split is enabled. Am I missing where they get picked up?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

You're right, they were dropped. IcebergWriteExec now declares write.supportedCustomMetrics() as V2 custom SQL metrics and the writer updates them from dataWriter.currentMetricsValues every CustomMetrics.NUM_ROWS_PER_UPDATE rows plus a final flush before the task commit, mirroring WritingSparkTask. Note Iceberg only implements supportedCustomMetrics() on its Spark 4.0+ modules, so on 3.x the set is empty either way.


override protected def doExecute(): RDD[InternalRow] = {
val rdd = child.execute()
val factory = batchWrite.createBatchWriterFactory(PhysicalWriteInfoImpl(rdd.getNumPartitions))

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.

How does this behave when the child RDD has zero partitions? Spark's SPARK-23271 handling runs a single-partition job so the empty commit is still produced deterministically, as writeWithV2 does. mapPartitionsInternal over a zero-partition RDD produces zero messages and relies on commit([]) behaving identically, which I'm not sure is guaranteed across write types. Could we add a test with a genuinely zero-partition child? The current empty-source test (L96) uses WHERE id < 0, which I believe yields one empty partition, not zero.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed -- doExecute now substitutes a dummy single-partition RDD when the child RDD has zero partitions, mirroring the SPARK-23271 handling in writeWithV2, so the commit always goes through the task-commit protocol. Added a test appending from a genuinely zero-partition RDD (sparkContext.emptyRDD, with getNumPartitions == 0 asserted) that checks exactly one commit message was collected. You're right that the existing empty-source test yields one empty partition; kept it since it covers a different shape.

Comment on lines +158 to +172
row.getInt(0) match {
case WRITE_OPERATION =>
rowProjection.project(row)
writer.write(rowProjection)
case WRITE_WITH_METADATA_OPERATION =>
rowProjection.project(row)
if (metadataProjection != null) metadataProjection.project(row)
val writeWithMetadata = dataWriterWriteWithMetadataMethod.getOrElse(
throw new UnsupportedOperationException(
"DataWriter.write(metadata, row) is not available in this Spark version but the " +
s"analyzer emitted operation code $WRITE_WITH_METADATA_OPERATION"))
writeWithMetadata.invoke(writer, metadataProjection, rowProjection)
case other =>
throw new IllegalArgumentException(
s"Unexpected ReplaceData operation code $other; supported: " +

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.

I believe codes 5 (WRITE) and 6 (WRITE_WITH_METADATA) are correct and complete for Spark 4.0+ copy-on-write ReplaceData (DELETE emits 6, UPDATE emits 6, MERGE emits 5 and 6), and that they do not exist before 4.0. This path looks safe only because IcebergReplaceDataShim (spark-3.x) returns None, routing 3.x through the plain write(row) loop. Could we state that invariant in a comment here? And would it be worth adding the suite to CI for all four Spark lines so the version split cannot regress unnoticed?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added the invariant comment (the codes are 4.0+ only; on 3.x IcebergReplaceDataShim returns None so rows take the plain write(row) loop and the dispatch is never reached). On CI: the suite is already in the scans shard of both pr_build_linux.yml and pr_build_macos.yml, and on Linux that shard runs across the full matrix (Spark 3.4/3.5/4.0/4.1/4.2), so the version split is exercised on every line.

Comment on lines +101 to +118
private def buildTwoOp(
write: Write,
rel: DataSourceV2Relation,
query: LogicalPlan,
replaceDataDispatch: Option[ReplaceDataDispatchInfo]): Option[SparkPlan] = {
val batchWrite = write.toBatch
if (batchWrite.useCommitCoordinator()) {
return None
}
// To mirror Spark ReplaceData semantics we invalidate our cache of the state of
// `originalTable`.
val refresh: () => Unit = () => IcebergRefreshCacheShim.recacheByPlan(rel)
Some(
IcebergCommitExec(
batchWrite,
refresh,
// `replaceDataDispatch` may project the data into the format the writer expects.
planLater(IcebergWriteLogical(query, batchWrite, replaceDataDispatch))))

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.

The writer sets requiredChildDistribution = UnspecifiedDistribution and depends on V2Writes having injected the repartition and local sort into .query. Iceberg uses the clustered writer for partitioned writes and throws on unclustered rows (ClusteredWriter.java L66-105). The AQE test (CometIcebergWriteActionSuite.scala L110) uses an unpartitioned table, so as far as I can tell coalesce and skew-split against a clustered partitioned write is never exercised. Could we add a partitioned write under AQE with a shuffle?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added a partitioned INSERT ... SELECT under AQE with an 8-partition shuffle (Iceberg's clustered distribution forces the exchange, AQE coalesces it), asserting the shuffle exists, exactly one commit, and all 500 rows land. Skew-split I couldn't force deterministically at unit-test scale, but the coalesce case covers the "AQE changed the shuffle output" class of risk for the clustered writer.

Comment on lines +412 to +425
private def assertExactlyOneCommit(snapshot: WriteSnapshot): Unit = {
assert(
snapshot.snapshotDelta == 1L,
s"expected exactly 1 new Iceberg snapshot, got ${snapshot.snapshotDelta}. Plans:\n" +
snapshot.plans.mkString("\n--\n"))
val (commits, writes) = collectIcebergWriteOps(snapshot.plans)
assert(
commits.nonEmpty,
s"expected >= 1 IcebergCommitExec in captured plans, got ${commits.size}. Plans:\n" +
snapshot.plans.mkString("\n--\n"))
assert(
writes.nonEmpty,
s"expected >= 1 IcebergWriteExec in captured plans, got ${writes.size}. Plans:\n" +
snapshot.plans.mkString("\n--\n"))

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.

assertExactlyOneCommit proves the commit ran once, but does it prove files were written once? And I don't see a failure test. Could we add a case that injects a task failure and a case that injects a commit failure, asserting the table is unchanged and abort ran? The abort path feels like the entire risk surface of a split write, and right now it looks untested. A speculative-execution case (spark.speculation=true) asserting a single commit and documenting the orphan-file behavior would also be reassuring.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This flushed out a real bug, thanks: on a failed write job, IcebergCommitExec previously threw out of executeCollect without ever calling batchWrite.abort. It now aborts (with empty messages -- per-task file cleanup already happened executor-side via DataWriter.abort) before rethrowing.

Tests added: (1) a task failure injected via a throwing UDF mid-write, asserting no new snapshot and the table unchanged; (2) a commit failure -- see the concurrent-append test on the other thread, where Iceberg's commit-time validation throws after the files are written and the job-level abort runs, asserting the failed DELETE left no snapshot.

Speculation I left untested: local-mode unit tests can't reliably trigger a speculative attempt. The exposure is the same as stock Spark+Iceberg though -- Iceberg opts out of the commit coordinator upstream too, so a straggler task attempt can at worst leave orphan data files that were never committed, on either path. Happy to note that in the guide if you'd like.

Comment on lines +381 to +382
try CometListenerBusUtils.waitUntilEmpty(spark.sparkContext)
catch { case _: java.util.concurrent.TimeoutException => () }

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.

Should we stop swallowing TimeoutException from waitUntilEmpty? My worry is that a dropped or late listener event turns the commit-count assertion into a silent pass. Would it be better to fail on timeout, or to read the snapshot delta directly from the Iceberg metadata rather than depending on listener delivery?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done -- the helper no longer swallows TimeoutException; a dropped listener event now fails the test instead of passing silently. Worth noting the commit-count half of the assertion (snapshotDelta) was already reading Iceberg's snapshot metadata directly, not the listener -- only plan capture depends on listener delivery.

Comment on lines +49 to +108
test("AppendData unpartitioned INSERT INTO routes through two-op") {
assume(icebergAvailable, "Iceberg not available in classpath")
withIcebergCatalog { warehouseDir =>
createTable(warehouseDir, "append_unpart", partitionSpec = "")
val snapshot = captureWrite("append_unpart") {
spark.sql(
"INSERT INTO cat.db.append_unpart VALUES " +
"(1, 'us-east', 10.5), (2, 'us-west', 20.3), (3, 'eu', 30.7)")
}
assertExactlyOneCommit(snapshot)
assertRows("append_unpart", expectedIds = Seq(1, 2, 3))
}
}

test("AppendData partitioned INSERT INTO routes through two-op") {
assume(icebergAvailable, "Iceberg not available in classpath")
withIcebergCatalog { warehouseDir =>
createTable(warehouseDir, "append_part", partitionSpec = "PARTITIONED BY (region)")
val snapshot = captureWrite("append_part") {
spark.sql(
"INSERT INTO cat.db.append_part VALUES " +
"(1, 'us-east', 10.5), (2, 'us-east', 20.3), (3, 'eu', 30.7)")
}
assertExactlyOneCommit(snapshot)
assertRows("append_part", expectedIds = Seq(1, 2, 3))
}
}

test("AppendData INSERT FROM SELECT survives the intervening exchange/sort") {
assume(icebergAvailable, "Iceberg not available in classpath")
withIcebergCatalog { warehouseDir =>
createTable(warehouseDir, "src", partitionSpec = "")
createTable(warehouseDir, "append_from_select", partitionSpec = "PARTITIONED BY (region)")
spark.sql(
"INSERT INTO cat.db.src VALUES " +
"(1, 'us-east', 10.5), (2, 'us-west', 20.3), (3, 'eu', 30.7)")

val snapshot = captureWrite("append_from_select") {
spark.sql(
"INSERT INTO cat.db.append_from_select " +
"SELECT id, region, amount FROM cat.db.src ORDER BY id")
}
assertExactlyOneCommit(snapshot)
assertRows("append_from_select", expectedIds = Seq(1, 2, 3))
}
}

test("AppendData on an empty source still emits a single commit") {
assume(icebergAvailable, "Iceberg not available in classpath")
withIcebergCatalog { warehouseDir =>
createTable(warehouseDir, "empty_target", partitionSpec = "")
val snapshot = captureWrite("empty_target") {
spark.sql(
"INSERT INTO cat.db.empty_target SELECT id, region, amount " +
"FROM (SELECT 1 AS id, 'r' AS region, 1.0 AS amount) WHERE id < 0")
}
assertExactlyOneCommit(snapshot)
assertRows("empty_target", expectedIds = Seq.empty)
}
}

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.

Could we add a multi-partition partitioned write so message collection and clustering see more than one task (several tests use coalesce(1))? A concurrent-writer test that drives Iceberg's commit-time conflict validation (validateNoConflictingData) would also be valuable. My understanding is that the shared BatchWrite instance exists to make that validation see the writer's scan state, and I don't think anything currently tests it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Both added: (1) a partitioned write with 4 shuffle partitions and AQE coalescing disabled, asserting numCommittedMessages >= 2 on the committer's metric; (2) a concurrent-writer test -- a gate UDF blocks the CoW DELETE's write job after its scan snapshot is pinned, a conflicting append commits in between, and the DELETE's commit fails Iceberg's serializable-isolation validation, leaving only the append's snapshot. (Runtime group filtering is disabled in that test; otherwise Iceberg's runtime file filter detects the snapshot change and aborts before the write even starts, which is a different guard than the one we want to exercise.) The second test pins exactly the shared-BatchWrite property: the committer's validation sees the writer's pinned scan state.

Comment on lines +62 to +66
## Supported operations

The split-operator plan is supported on every Spark version Comet supports, with identical
coverage on each:

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.

Could we revisit the "identical coverage on every Spark version" claim? The copy-on-write ReplaceData path looks like it differs by version: 4.0+ uses operation-coded rows with projections, while 3.4/3.5 use a plain row stream. Would it be more accurate to state that the row-level DML mechanism differs by Spark version so the docs match the shim behavior?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Reworded -- the docs now say the supported operations are the same on every version but the row-level DML mechanism differs (4.0+ operation-coded rows with projections, 3.4/3.5 a plain row stream).

@jordepic
jordepic force-pushed the iceberg-writes-split-c1 branch from 8126020 to 4bf5940 Compare July 25, 2026 15:36
@jordepic

Copy link
Copy Markdown
Contributor Author

@mbutrovich good morning! Did you ever get any feedback from the smoke tests?

@andygrove

Copy link
Copy Markdown
Member

Thanks for your patience @jordepic. We've been busy with getting the Comet 1.0 release ready (we'll create the release candidate next week), and I'm keen to merge this PR soon after that.

@andygrove

Copy link
Copy Markdown
Member

smoke

I kicked off the smoke test in CI
#5255

@andygrove

Copy link
Copy Markdown
Member

I ran a CI audit of this change with spark.comet.write.iceberg.splitOperator.enabled flipped to true, to see what the split-operator plan breaks when it is actually exercised. The audit PR is #5255 and the findings are written up in #5259.

Summary of the 4 root causes:

  1. CometIcebergRewriteActionSuite matches on AppendData, which the split operator replaces. Test-only fix.
  2. Iceberg's TestSystemFunctionPushDownInRowLevelOperations casts the plan root to V2TableWriteExec and hits a ClassCastException on IcebergCommitExec. This one is worth a design discussion: any third-party code that pattern-matches on the standard Iceberg write shape sees something it does not recognise. We can either patch the Iceberg diffs or keep a compatible root node.
  3. Merge metrics are lost. Iceberg derives the spark.merge-into.* snapshot summary properties from the write's driver-side metrics at commit time, and splitting write from commit drops them (36 TestCopyOnWriteMergeMetrics failures on Iceberg 1.11).
  4. TestCachedTableRefresh.testCachedTableWithSessionSchemaChangeAddColumn fails on Iceberg 1.11.

None of this blocks merging this PR, since the feature is off by default, but 2, 3, and 4 look like prerequisites before it can be enabled.

@jordepic
jordepic force-pushed the iceberg-writes-split-c1 branch from 4bf5940 to 6cf9902 Compare August 4, 2026 17:42
@andygrove

Copy link
Copy Markdown
Member

@jordepic could you fix conflicts?

Jordan Epstein added 2 commits August 4, 2026 15:10
Iceberg's V2 writes run as a single `V2ExistingTableWriteExec` command that both
writes files and commits. Spark's `InsertAdaptiveSparkPlan` treats it as a leaf,
so AQE never sees the data sub-query inside it and Comet's columnar rules can't
convert the scans / shuffles / sorts that feed the write.

This injects `IcebergWriteStrategy` ahead of Spark's `DataSourceV2Strategy` (via
`experimentalMethods.extraStrategies`, which is prepended to the planner's
strategy list). For Iceberg `AppendData`, `OverwriteByExpression`, and
`OverwritePartitionsDynamic` it rewrites the single command into two operators:

  IcebergCommitExec(batchWrite, refreshCache)   <- committer (driver-side)
  └── AdaptiveSparkPlanExec                       <- AQE bubble
      └── IcebergWriteExec(batchWrite)            <- writer (UnaryExecNode)
          └── <data sub-query>                    <- now visible to AQE / Comet

The writer emits one row per Spark task carrying the Java-serialised
`WriterCommitMessage`; the committer collects them and calls
`BatchWrite.commit(messages)` -- the same call Iceberg-Java makes internally --
then runs Spark's post-write cache refresh.

Two design points keep this stable under AQE, which re-runs the planner on each
materialised stage's `logicalLink`:

- The writer's child is wrapped in `IcebergWriteLogical`, a stable logical
  anchor, so each re-plan re-emits only the writer rather than vanishing it or
  re-firing the surrounding logical write and duplicating the commit.
- The `BatchWrite` is materialised once (`Write.toBatch()` mints a fresh
  instance per call) and shared between committer and writer, so commit-time
  validation sees the same scan / emitted-file state the writer used.

`WriteDelta` (merge-on-read) is intentionally not intercepted: its per-task
`DeltaWriter` is row-dispatched and the native writer can't emit position-delete
files, so the split plan would add planning complexity for no acceleration.
Copy-on-write DELETE / UPDATE / MERGE (`ReplaceData`) is handled in a follow-up
commit.

Guarded by `spark.comet.write.iceberg.splitOperator.enabled` (off by default);
when off, writes go straight through Iceberg-Java unchanged. File writing still
runs through Iceberg's JVM writer -- native Parquet write lands in a later
commit.
Jordan Epstein added 3 commits August 4, 2026 15:11
Adds copy-on-write row-level DML to the split-operator plan. Spark lowers a CoW
`DELETE` / `UPDATE` / `MERGE` on a V2 table into a `ReplaceData` logical node;
`IcebergWriteStrategy` now matches it (refreshing `originalTable`'s cache) and
routes it through the same committer + writer pair as appends/overwrites.

The writer gains a per-row dispatch path. On Spark 4.x a `ReplaceData` rewrite
prefixes each row with an operation code and carries a `ReplaceDataProjections`;
`IcebergWriteExec` applies the row / metadata projections per code (WRITE /
WRITE_WITH_METADATA) before handing rows to the `DataWriter`. The two-arg
`DataWriter.write(metadata, row)` is looked up reflectively since it is 4.x-only.
On Spark 3.4 / 3.5 the rewritten stream is already post-projection, so the shim
returns `None` and the writer keeps its plain `write(row)` loop.

`ReplaceDataProjections` does not exist on 3.4 / 3.5, so `ReplaceDataDispatchInfo`
mirrors it as a version-neutral carrier, populated by the per-version
`IcebergReplaceDataShim`.

Iceberg 1.5.2 (Spark 3.4) lacks native row-level operations and instead routes
UPDATE / MERGE through its own `ReplaceIcebergData` logical node. It has the same
field shape as Spark's `ReplaceData` and is matched by FQCN via reflection, so
the main module takes no compile dependency on iceberg-spark-extensions.
@jordepic
jordepic force-pushed the iceberg-writes-split-c1 branch from 6cf9902 to 99ca922 Compare August 4, 2026 20:22
@jordepic

jordepic commented Aug 4, 2026

Copy link
Copy Markdown
Contributor Author

@andygrove back up

@andygrove

Copy link
Copy Markdown
Member

@jordepic do you want to just enable spark.comet.write.iceberg.splitOperator.enabled on this PR temporariliy, so we can just run the tests here, and we can flip back to default before merging?

@jordepic

jordepic commented Aug 4, 2026

Copy link
Copy Markdown
Contributor Author

@andygrove - your call, if you want we could use the other PR to run in parallel.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Writes to Apache Iceberg Tables

4 participants