andygrove commented on code in PR #5696:
URL: https://github.com/apache/datafusion-comet/pull/5696#discussion_r3941343362
##########
spark/src/main/scala/org/apache/comet/rules/EliminateRedundantTransitions.scala:
##########
@@ -91,6 +91,18 @@ case class EliminateRedundantTransitions(session:
SparkSession)
// Write should be final operation in the plan
case ColumnarToRowExec(nativeWrite: CometNativeWriteExec) =>
nativeWrite
+ // `CometIcebergWriteExec` is row-based (it emits the serialised Iceberg
commit message) but
+ // consumes Arrow batches from its child over FFI, so Spark inserts a
columnar-to-row
+ // transition *underneath* it. Strip it so `doExecuteColumnar` sees the
columnar child
+ // directly; `CometIcebergNativeWrite.requiresNativeChildren` already
guarantees that child
+ // was Comet-native when the write was converted.
+ //
+ // The write deliberately does not tag itself as a
`ColumnarToRowTransition` to suppress the
+ // insertion: Spark leaves such a node untouched, so the whole subtree
below the write is
+ // never visited and the transitions the rest of that subtree needs are
never inserted
+ // (https://github.com/apache/datafusion-comet/issues/5689).
+ case w: CometIcebergWriteExec =>
Review Comment:
Filed as #5719, with you credited for spotting it.
I probed `revertToSpark` directly before writing it up, and both branches of
the arity check are broken, not just the warning one:
- leaf or multi-child child (size mismatch): returns the child as-is, write
gone
- unary child (both sizes 1): `child.withNewChildren(Seq(child))`, so the
write is gone **and** the child is duplicated
```
=== INPUT ===
CometIcebergWrite [iceberg_commit_message#6], , ICEBERG_WRITER_UNPARTITIONED
+- CometFilter [_1#4], (isnotnull(_1#4) AND (_1#4 > 2))
+- CometNativeScan parquet [_1#4] ...
=== REVERTED ===
Filter (isnotnull(_1#4) AND (_1#4 > 2))
+- Filter (isnotnull(_1#4) AND (_1#4 > 2))
+- ColumnarToRow
+- FileScan parquet [_1#4] ...
```
The duplication is harmless for an idempotent `Filter` but not for a
projection with `monotonically_increasing_id()`, a sample, or a limit.
--
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]
