Github user rednaxelafx commented on a diff in the pull request: https://github.com/apache/spark/pull/20224#discussion_r163461361 --- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/WholeStageCodegenExec.scala --- @@ -365,6 +388,26 @@ case class WholeStageCodegenExec(child: SparkPlan) extends UnaryExecNode with Co "pipelineTime" -> SQLMetrics.createTimingMetric(sparkContext, WholeStageCodegenExec.PIPELINE_DURATION_METRIC)) + /** + * ID for codegen stages within a query plan. + * It does not affect equality, nor does it participate in destructuring pattern matching. + * + * Within a query, a codegen stage in a plan starts counting from 1, in "insertion order". + * WholeStageCodegenExec operators are inserted into a plan in depth-first post-order. + * See CollapseCodegenStages.insertWholeStageCodegen for the definition of insertion order. + * + * 0 is reserved as a special ID value to indicate a temporary WholeStageCodegenExec object + * is created, e.g. for special fallback handling when an existing WholeStageCodegenExec + * failed to generate/compile code. + */ + val codegenStageId = WholeStageCodegenId.getNextStageId() --- End diff -- Thanks for your comment, @cloud-fan ! That's a nice catch that I hadn't really thought about. All the examples that I've run with are ones that wouldn't trigger changes to the plan after `CollapseCodegenStages`, which means for those examples `ReuseExchange` / `ReuseSubqueries` wouldn't have triggered. Yes, these two rules could potentially change the physical plan, which means when we `transformUp` in those rules (and any future rule after `CollapseCodegenStages`) it'd create new `WholeStageCodegenExec` objects outside of `CollapseCodegenStages`, and with my current implementation that'll result in the WSC copies having a codegen stage ID of 0. One way to workaround this is to move `CollapseCodegenStages` to always be the last rule in `org.apache.spark.sql.execution.QueryExecution#preparations`, so that we're sure there's no other transformation on the physical plan that could change the structure of the plan, except for fallback handling that could happen in a couple of `doExecute()`s -- these exception cases are expected and to me they are acceptable. If we go down that route, I'll probably have to tweak `CollapseCodegenStages` a little bit so that it can cope with the physical query plan potentially becoming a DAG instead of a tree, as the `ReuseExchange` / `ReuseSubqueries` rules may do that kind of transformation. This tweak is easy to implement and low risk: simply bailing out of the transforming a subtree when it sees a `WholeStageCodegenExec` already inserted into the plan would suffice. Let me actually update the PR with this tweak and see what happens in tests.
--- --------------------------------------------------------------------- To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org