andygrove commented on PR #5957: URL: https://github.com/apache/datafusion-comet/pull/5957#issuecomment-5876640490
This is a light fully automated review since there are so many PRs open. The new check at `RevertNativeForTransitionHeavyStages.scala:239` covers the AQE-off shape, but I think `SELECT _1, _2 FROM tbl DISTRIBUTE BY _2` with `maxTransitions=0` still breaks with AQE on, which is the default. AQE coalesces that small shuffle, so the result stage reaches this rule as `ColumnarToRowExec(AQEShuffleReadExec(ShuffleQueryStageExec))`. `stripped` is then the `AQEShuffleReadExec`, which `isStageBoundary` doesn't match, so the check doesn't fire. `insertTransitions` only adds a `ColumnarToRowExec` under a row-based parent, and `revertStageIfNeeded` at line 114 only fixes up the columnar-output direction, so the stage comes back as a bare columnar `AQEShuffleReadExec`. Its `doExecute` just casts the `CometShuffledBatchRDD`, so I'd expect `executeCollect` to fail casting a `ColumnarBatch` to `UnsafeRow`. I think any row-output stage whose reverted root stays columnar has the same gap, for example a scan-only query where `CometNativeScanExec` reverts to a vectorized `FileSourceScanExec`. Could `revertStageIfNeeded` wrap the result in `ColumnarToRowExec` when `!outputColumnar && reverted.supportsColumnar`, mirroring the `RowToColumnarExec` branch? Could the `DISTRIBUTE BY` test at `RevertNativeForTransitionHeavyStagesSuite.scala:665` also run with AQE on? -- 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]
