andygrove commented on PR #6137: URL: https://github.com/apache/datafusion-comet/pull/6137#issuecomment-5876194635
This is a light fully automated review since there are so many PRs open. The new `CometNativeScanExec` case in `revertToSpark` fixes the V1 Parquet path, but `CometIcebergNativeScanExec` still falls into the generic `case _ => cometExec.originalPlan` branch right below it (`spark/src/main/scala/org/apache/comet/rules/RevertNativeForTransitionHeavyStages.scala:202-214`), and it looks like it hits the same bug this PR closes. `CometIcebergNativeScanExec.originalPlan` is `@transient` and, per the comment in `spark/src/main/scala/org/apache/spark/sql/comet/CometIcebergNativeScanExec.scala:96-105`, is never touched when `CometPlanAdaptiveDynamicPruningFilters` rewrites the live scan's top-level `runtimeFilters`, so it keeps the pre-DPP `SubqueryAdaptiveBroadcastExec` placeholder. `serializedPartitionData` works around this by rebuilding `effectiveOriginalPlan = originalPlan.copy(runtimeFilters = runtimeFilters)` before reading it, but `revertToSpark` restores the stale `originalPlan` as-is. With AQE, dynamic partition pruning, `spark.comet.exec.transitionRe vert.enabled`, and a native Iceberg scan (`spark.comet.scan.icebergNative.enabled` defaults to `true`) in the reverted stage, wouldn't this reproduce the same `SubqueryAdaptiveBroadcastExec does not support the execute()` failure, just for the wrapped `BatchScanExec` instead of `FileSourceScanExec`? Would it make sense to extend this case, or generalize it, to cover `CometIcebergNativeScanExec` too? -- 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]
