comphead commented on code in PR #6664: URL: https://github.com/apache/datafusion-comet/pull/6664#discussion_r4200267381
########## docs/source/user-guide/latest/migration-guide.md: ########## @@ -83,6 +83,28 @@ it. `spark.comet.convert.oneRowRelation.enabled` is on by default, so such a que without either setting, and logs no warning. The list remains the way to convert other leaf operators, such as the scan of a Data Source V2 connector. +### Iceberg Writes + +`spark.comet.write.iceberg.enabled` now defaults to `true`. Comet plans an Iceberg `INSERT INTO`, Review Comment: The 1.1.0 name for this setting was `spark.comet.iceberg.write.enabled`, and the [1.1.0 user guide](https://github.com/apache/datafusion-comet/blob/1.1.0/docs/source/user-guide/latest/iceberg-writes.md#L83) told people to set that name. #6494 renamed it to `spark.comet.write.iceberg.enabled` after the 1.1 branch was cut. Comet ignores the old name now, and nothing in this entry mentions it. That matters here because the entry says the setting "now defaults to `true`" and offers `spark.comet.write.iceberg.enabled=false` as the way back to 1.1.0 behavior. A deployment that set `spark.comet.iceberg.write.enabled=false` in 1.1.0 would get native writes after upgrading. This PR is also the one that moves the key out of `testing`, so it seems like the right place to say it. Could the entry name the old key and say Comet ignores it, the way the 1.1.0 section does for the testing keys it removed? ########## spark/src/main/scala/org/apache/comet/iceberg/IcebergWriteStrategy.scala: ########## @@ -38,9 +38,13 @@ 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) || + CometConf.COMET_ICEBERG_WRITE_SPLIT_OPERATOR_ENABLED.get(conf) // Planner strategies run whether or not Comet is enabled, so check it here too: with Comet // off, Spark must plan its own V2 write operator. - if (!isCometLoaded(conf) || !CometConf.COMET_ICEBERG_WRITE_SPLIT_OPERATOR_ENABLED.get(conf)) { + if (!isCometLoaded(conf) || !splitEnabled) { Review Comment: Should `spark.comet.exec.enabled=false` also keep Spark's own write operator? This strategy is injected unconditionally in `CometSparkSessionExtensions`, and it only checks `isCometLoaded`, the two write flags and plan-only mode. `isCometLoaded` covers `spark.comet.enabled`, off-heap memory, the shuffle manager and a few other startup conditions, but not `spark.comet.exec.enabled`. So with the new default, someone who runs Comet for scans or shuffle only would get `IcebergCommit` over a JVM `IcebergWrite` after upgrading. The operators page tells them `spark.comet.exec.enabled=false` turns off native execution, and the new fallback list in `iceberg-writes.md` names only `spark.comet.enabled=false`. `CometIcebergWriteActionSuite` covers that case (line 122) but not this one. I read this from the code and haven't run it. The description says the split plan alone gives users nothing over Spark's operator, so I'd lean toward adding `COMET_EXEC_ENABLED` to this check, with a test next to the `spark.comet.enabled=false` one. If you'd rather keep the split plan in that mode, could the fallback list say so? It could also mention plan-only mode, which this method checks too. ########## docs/source/user-guide/latest/iceberg-writes.md: ########## @@ -92,10 +96,8 @@ spark.sql.catalog.<name>=org.apache.iceberg.spark.SparkCatalog spark.sql.catalog.<name>.type=hadoop # or hive / glue / rest / ... spark.sql.catalog.<name>.warehouse=... -# Split-operator plan (experimental, off by default) -spark.comet.write.iceberg.splitOperator.enabled=true - -# Native Parquet writer (experimental, off by default; requires the split plan) +# Split-operator plan and native Parquet writer (on by default since Comet 1.2.0; false plans +# Spark's own write operator) spark.comet.write.iceberg.enabled=true Review Comment: This block still reads as an enable step. [Line 89](https://github.com/apache/datafusion-comet/blob/1dc0d67df2ee8b15dc5b8704598dfb71e2d22831/docs/source/user-guide/latest/iceberg-writes.md#L89) introduces it as "plus the write-side toggle", and this line sets `spark.comet.write.iceberg.enabled=true`, which is now the default. Copying the block changes nothing for this setting. Of the write-related lines, only `spark.comet.exec.localTableScan.enabled=true` changes behavior. Could the example show `spark.comet.write.iceberg.enabled=false` as the opt-out instead? Or the line could go, with the default stated in the text above. -- 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]
