andygrove commented on PR #4658: URL: https://github.com/apache/datafusion-comet/pull/4658#issuecomment-5181963968
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. -- 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]
