andygrove commented on code in PR #6664:
URL: https://github.com/apache/datafusion-comet/pull/6664#discussion_r4253014839
##########
spark/src/main/scala/org/apache/comet/iceberg/IcebergWriteStrategy.scala:
##########
@@ -31,16 +31,21 @@ import
org.apache.comet.CometSparkSessionExtensions.isCometLoaded
import org.apache.comet.shims.ShimCometMergeRows
/**
- * Spark strategy for Comet's split Iceberg V2 writer. WriteDelta is modeled
and dispatched here,
- * but position-delta rows are still executed by Iceberg's JVM DeltaWriter.
+ * Spark strategy for Comet's split Iceberg V2 writer. WriteDelta is modeled
and dispatched here
+ * when the testing split flag is on, but position-delta rows are still
executed by Iceberg's JVM
+ * DeltaWriter.
*/
case class IcebergWriteStrategy(session: SparkSession) extends SparkStrategy {
override def apply(plan: LogicalPlan): Seq[SparkPlan] = {
val conf = session.sessionState.conf
+ // The native write flag plans the split operator on its own. The split
flag plans it with
+ // the native writer off, which only tests do.
+ val splitEnabled = CometConf.COMET_ICEBERG_NATIVE_WRITE_ENABLED.get(conf)
||
Review Comment:
The old key can stay set after the upgrade, and the split flag is now a
testing setting that the suites depend on: they set it with the write flag off
to get the split plan with iceberg-java's writer as a baseline. So I kept the
write flag from overriding it and documented the combination in cb90a1b5f. The
1.2.0 upgrade entry now says a deployment that set the split flag in 1.1.0 has
to unset it, as well as set `spark.comet.write.iceberg.enabled=false`, to get
Spark's own operator back. It also says that on Spark 3.5+ the flag plans
merge-on-read writes as the split plan. The fallback list in
`iceberg-writes.md` says the same, and `the split flag keeps the split plan
with iceberg-java's writer when the write flag is off` pins the combination.
##########
docs/source/contributor-guide/iceberg-spark-tests.md:
##########
@@ -31,13 +31,11 @@ Here is an overview of the changes that the diffs make to
Iceberg:
uses a native Iceberg scan, these classes fail to compile and must be
removed.
- Configure test base classes (`TestBase`, `ExtensionsTestBase`,
`ScanTestBase`, etc.) to load the Comet Spark
plugin and shuffle manager
-- Enable the Iceberg write split-operator plan
(`spark.comet.write.iceberg.splitOperator.enabled`) alongside the
- native scan in every Comet-configured session. The flag is off by default
for users, so Iceberg's own suites
- are the only place the split plan (`IcebergCommit -> IcebergWrite`) is
exercised against Iceberg's write,
- commit, and row-level-operation tests. See [#5259]
-- Enable Comet's native (iceberg-rust) Parquet writer
(`spark.comet.write.iceberg.enabled`) in the same sessions.
- The native writer is experimental and off by default for users, so this is
where it runs against Iceberg's
- write, commit, and row-level-operation tests.
+- Enable Comet's Iceberg write path, the split-operator plan (`IcebergCommit
-> IcebergWrite`) and the native
+ (iceberg-rust) Parquet writer, alongside the native scan in every
Comet-configured session, so that Iceberg's
+ write, commit, and row-level-operation tests run through it.
`spark.comet.write.iceberg.enabled` switches both
+ and is on by default; the diffs set it explicitly, together with the
testing-only
+ `spark.comet.write.iceberg.splitOperator.enabled`, which it makes redundant.
See [#5259]
Review Comment:
It is still needed for merge-on-read writes on Spark 3.5+, which the write
flag alone leaves on Spark's plan. I changed the sentence in cb90a1b5f to say
the flag is redundant for the normal write path but still plans `WriteDelta` as
the split plan, so Iceberg's row-level-operation tests exercise it.
--
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]
