unikdahal commented on code in PR #6664:
URL: https://github.com/apache/datafusion-comet/pull/6664#discussion_r4252723097
##########
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:
One concern with the upgrade path here.
In 1.1 we documented enabling
"spark.comet.write.iceberg.splitOperator.enabled" together with native writes,
so users may still have this set after upgrading.
With the OR here, setting "spark.comet.write.iceberg.enabled=false" would
still use the split write path rather than restoring Spark's original write
operator. On Spark 3.5+ it would also enable the split "WriteDelta" path.
Should we either make the new flag authoritative, or mention in the
migration guide that the old split flag needs to be removed/unset? A test for
this combination would probably be useful too.
##########
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:
I don't think this flag is completely redundant. It is redundant for the
normal write path, but it is still needed to exercise the split "WriteDelta"
path for MOR writes on Spark 3.5+.
Might be worth clarifying that here.
--
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]
