LuciferYang commented on code in PR #58419:
URL: https://github.com/apache/spark/pull/58419#discussion_r3940609010
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/basicPhysicalOperators.scala:
##########
@@ -1047,10 +1090,10 @@ case class UnionExec(children: Seq[SparkPlan]) extends
SparkPlan with CodegenSup
}
}
- // Memoized: consulted by `supportCodegen` (called multiple times by
- // `CollapseCodegenStages`) and by `metrics`. Conf and children are stable
- // for a given UnionExec instance; cross-plan staleness is impossible since
- // UnionExec is a case class and `withNewChildren` produces a fresh instance.
+ // Memoized so `supportCodegen` (called repeatedly by
`CollapseCodegenStages`)
+ // and `metrics` see one reason on one instance; `conf` is live, so
re-deriving
+ // could answer differently. Agreeing with the `withNewChildren` copy is the
+ // `isPlainUnion` tag's job, not this memo's.
@transient private lazy val supportCodegenFailureReason: Option[String] = {
Review Comment:
Good news: latching the whole reason worked, so I did not need the
unconditional-registration fallback. Fixed in `543684188d1`.
The reason now lives in a `CODEGEN_FAILURE_REASON` tag under the same
`decisionLock`, so `metrics` and `doProduce` see one answer by construction,
and the metric still stays off plans that fall back to `doExecute`. The
trade-off is the one I mentioned: `spark.sql.codegen.wholeStage.union.enabled`
becomes sticky per plan, the same semantics this PR already gives
`spark.sql.unionOutputPartitioning`. Glad to switch to the unconditional
registration if you would rather have that.
Your parenthetical about the exchange turned out to be the key part, and I
am glad you wrote it. With `spark.range(...).union(spark.range(...))` nothing
fails, because `withNewChildren` returns `this` when the children compare
equal, so there is no second instance to re-derive anything; the child has to
not be `CodegenSupport` for `insertInputAdapter` to produce a real copy. My
first repro attempt missed that and came back green. The new test puts a
`repartition(2)` on each side, and reverting the latch makes it fail with the
same `key not found: numOutputRows`.
The PR description now covers both routes.
--
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]
