andygrove commented on PR #4658: URL: https://github.com/apache/datafusion-comet/pull/4658#issuecomment-5071122344
Thanks for splitting this out of #4487, @jordepic. This is a much better review target, and the split is clean. (Reviewed with LLM assistance.) I traced the commit protocol against Spark's own V2 write and it is a faithful reproduction. The writer does per-task `commit` / `abort` / `close` via `tryWithSafeFinallyAndFailureCallbacks`, and the committer extends `V2CommandExec` so `run()` executes once for exactly-once commit, calling `onDataWriterCommit` per message then `commit(messages)`, with `abort(messages)` plus `addSuppressed` on failure. The shared `BatchWrite` instance between writer and committer is the subtle detail that makes commit-time validation work, and I appreciate that it is documented. The fallback is safe too: the strategy returns `Nil` when the config is off or the plan is not an Iceberg write, so the worst case is no acceleration rather than a corrupted table, and that path is tested. Nice test coverage overall, especially the empty-source single-commit and the disabled-config fallthrough cases. A few things I would like to resolve before approving, none of them large: **1. The `useCommitCoordinator()` guard fails hard at execution time.** In `IcebergWriteExec.doExecute` the `require(!batchWrite.useCommitCoordinator(), ...)` throws mid-execution if an Iceberg catalog returns a `BatchWrite` that needs Spark's commit coordinator, which fails the whole query after the user has opted in. Since the strategy already has the `Write` at planning time, would it be cleaner to check this in `matchedSparkWrite` / `buildTwoOp` and return `None` there, so an unsupported write shape falls back to Spark's normal path instead of hard-failing? **2. AQE exactly-once confidence.** The design wraps the writer child in `IcebergWriteLogical` so AQE re-plans only the writer, and the exchange/sort and empty-source tests exercise this indirectly. Could you confirm, or point at a test that asserts, that AQE re-planning the writer subtree cannot cause data files to be written or committed twice? The commit-count assertions look like they cover it, I just want to make sure that is the intent. **3. Docs scope.** You flagged this yourself. Since this PR does not ship the native write path yet, could we trim `iceberg-writes.md` to what is actually enabled here, or mark the native sections as upcoming? Otherwise the merged docs describe functionality a user cannot turn on yet. One minor note: commit messages are serialized with `ObjectOutputStream` into a `BinaryType` column. That is fine since Iceberg's messages are `Serializable`, just worth being aware it couples the wire format to Java serialization across the supported Iceberg versions. Also a small process point: once these splits land, it is probably worth closing #4487 (it is currently conflicting and stale) so the splits become the source of truth. -- 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]
