sunchao commented on PR #5483: URL: https://github.com/apache/datafusion-comet/pull/5483#issuecomment-5442492239
Updated in [92b0baac9](https://github.com/apache/datafusion-comet/commit/92b0baac92bafdb866547fda3161c4ce3eab981a). I checked the exchange question against the actual AQE/DPP lifecycle before widening the guard. On the ordinary exchange path, Spark [wraps exchanges in query-stage leaves](https://github.com/apache/spark/blob/v4.1.3/sql/core/src/main/scala/org/apache/spark/sql/execution/adaptive/AdaptiveSparkPlanExec.scala#L586-L637), so this transform does not revisit the exchange inside them. There is an exception worth distinguishing: the manually inserted DPP broadcast can carry a direct `LogicalQueryStage` link as the whole adaptive-plan root. I observed this in real SQL with a grouped fact input joining a grouped, filtered dimension, and in a grouped self-join. In both observed replans, repair restored the original aggregate link, but the prepared physical tree was equal to the reused tree, both costs were `SimpleCost(0)`, and `TEMP_LOGICAL_PLAN_TAG` retained the original aggregate identity. With Spark's standard cost evaluator, [AQE rejects that unchanged replan](https://github.com/apache/spark/blob/v4.1.3/sql/core/src/main/scala/org/apache/spark/sql/execution/adaptive/AdaptiveSparkPlanExec.scala#L343-L370), and [new stage creation prioritizes the retained temporary link](https://github.com/apache/spark/blob/v4.1.3/sql/core/src/main/scala/org/apache/spark/sql/execution/adaptive/AdaptiveSparkPlanExec.scala#L715-L727). All three probe queries matched Spark and retai ned Comet DPP subqueries. This does not support an additional exchange guard for the lifecycle tested here, so I kept the existing exchange restore/clear behavior and the #323 empty-link contract. I also left helper refactoring out of this focused fix. I retained `elapsed_compute > 0`: it checks a recorded native measurement, not a latency target. Comet exposes nanoseconds, and the locked DataFusion timer [adds at least one nanosecond per recorded duration](https://github.com/apache/datafusion/blob/54.1.0/datafusion/physical-expr-common/src/metrics/value.rs#L191-L204). A loaded CI runner does not create the proposed zero-duration failure. For runtime, the existing [Spark 4.1 CI job](https://github.com/apache/datafusion-comet/actions/runs/33020356343/job/98354848063) recorded 471 ms for this regression in an approximately 128-second suite, so I kept the input sizes. #5482 records the concrete stale-stage-link defect; removing all original-plan repair would be separate work and still needs to preserve #323. The follow-up adds the requested version-pinned Spark references beside the guard. Local validation rebuilt the full Spark 4.1.3 JVM reactor and passed all 31 planner tests plus five execution/AQE/DPP/shuffle-link regressions (36 total). The separate DPP probe also asserts physical-plan equality, equal cost, and preservation of the temporary logical identity. These local runs reused a previously built OSS native library; no native code changed. -- 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]
