andygrove commented on PR #5626: URL: https://github.com/apache/datafusion-comet/pull/5626#issuecomment-5500603634
Not as written, and I think that is the right default for now. The rule only matches `WriteFilesExec` and `DataWritingCommandExec(InsertIntoHadoopFsRelationCommand)`, and then gates on the `FileFormat` class name being under `org.apache.spark.sql.execution.datasources.`. Iceberg writes are DSv2, so they land on `AppendDataExec` / `OverwriteByExpressionExec` / `WriteDeltaExec` (`V2TableWriteExec`) instead, and neither arm ever fires. The format prefix check was written precisely to exclude third-party writers, so Iceberg is the case it is keeping out rather than one it happens to miss. Whether it *could* work is a more interesting question, and structurally the plain append path looks like it would: * Iceberg's writer API is row-typed the same way Spark's is: `DataWriter<InternalRow>`, and the append task in Spark is `DataWritingSparkTask`, which is just `writer.writeAll(iter)`. Nothing between the iterator and the writer copies or buffers. * On the Iceberg side an unpartitioned append goes `RollingDataWriter` -> `SparkFileWriterFactory` -> `SparkParquetWriters`, which encodes field by field on the spot. But the safety argument would have to be redone against a much bigger writer set than Spark's, and it is a set we do not control the version of: * partitioned tables use the clustered/fanout writers, which key rows into maps * MERGE/UPDATE go through the position-delta and equality-delete writers via `DeltaWritingSparkTask`, and the equality-delete path keys inserted rows into a `StructLikeMap` * Iceberg's default distribution mode puts a shuffle and a local sort between the scan and the write for partitioned or sorted tables, and both of those need `UnsafeRow` anyway, so those plans would not qualify even if the writer did The part that bothers me is that for Spark's own writers the gate is at least argued against source that moves in lockstep with the Spark version we compile against. For Iceberg it would be argued against a dependency on its own release cadence, where a writer that starts retaining a row is a silent-corruption bug for us and a perfectly reasonable change for them. If this lands at all, that pushes me further toward caveat 1 in the description: an explicit allowlist of known-safe consumers rather than a package prefix, and Iceberg only added to it with its own round of round-trip tests. So: worth a follow-up issue rather than scope for this PR, and only after the core mechanism has convinced people it is worth having. -- 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]
