andygrove commented on PR #5696: URL: https://github.com/apache/datafusion-comet/pull/5696#issuecomment-5546337821
Thanks for tracing it through — agreed on the trait being purely a marker on the write, and on the commit-message contract being unaffected. One thing I want to push back on: the placeholder is not there by the time transitions get inserted. `CometExecRule._apply` strips `CometSinkPlaceHolder` and `CometScanWrapper` at the end of its own pass, and that pass is `preColumnarTransitions`, so `insertTransitions` never sees a placeholder. I did try the `CometPlan` guard first, and it took out ten AQE-on tests across `CometIcebergWriteActionSuite`, `CometIcebergWriteDetectionSuite` and `CometIcebergSystemFunctionSuite`, every one of them with `CometIcebergWriteExec requires a columnar (Comet native) child; got WholeStageCodegenExec` — the guard rejected the transition's child, so the `ColumnarToRow` survived and `CollapseCodegenStages` wrapped it. Instrumenting the rule on the CoW DELETE test prints `write child=ColumnarToRowExec stripped=Some(AQEShuffleReadExec) aqe=true`, so under AQE the child is an `AQEShuffleReadExec`, which is a plain Spark node. That also explains why the plain `ColumnarToRowExec` form has to stay in `stripColumnarToRow`: the `hasCometNativeChild` arm above never fires on it, because `QueryStageExec` is a `LeafExecNode` and the `op.exists(...)` walk cannot see the Comet exchange inside the stage. I pushed a comment recording that, since it was not obvious to me either. The TPC-H check that went red on the first run failed with `Network is unreachable` during setup; the push above re-runs it. -- 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]
