andygrove commented on code in PR #6119:
URL: https://github.com/apache/datafusion-comet/pull/6119#discussion_r4109345949
##########
spark/src/main/scala/org/apache/spark/sql/comet/IcebergCommitExec.scala:
##########
@@ -106,6 +108,7 @@ case class IcebergCommitExec(
try {
messages.foreach(batchWrite.onDataWriterCommit)
+ IcebergCommitExec.runPreCommitHook(messages)
Review Comment:
Could the test use the deterministic conflict from `a commit-time failure
surfaces as the same exception as Spark's own write` instead of adding
`withPreCommitHook` here? A serializable overwrite with
`validate-from-snapshot-id` set to an older snapshot fails commit validation
after every task has finished, with no concurrency and no hook. I tried that
shape under `withNativeEnabled` and compared the data directory with the
manifests afterwards. It passes as is, and it fails with the native file
orphaned once `throw abortAfter(messages, cause)` becomes `throw cause`, the
same as your test. The handoff hook in #6117 exists because that boundary can't
be reached any other way, but this one can, so `IcebergCommitExec` wouldn't
need to carry test-only global state.
--
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]
