LuciferYang commented on code in PR #58419:
URL: https://github.com/apache/spark/pull/58419#discussion_r3999036310
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/basicPhysicalOperators.scala:
##########
@@ -1044,11 +1091,21 @@ 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.
- @transient private lazy val supportCodegenFailureReason: Option[String] = {
+ // Latched for the same reason `isPlainUnion` is: `supportCodegen` and
`metrics` must see one
+ // answer, and `conf` is live. When a child is not `CodegenSupport`,
`insertInputAdapter` wraps
+ // it, so `withNewChildren` returns a real copy whose first evaluation of
this would land at
+ // execution; re-deriving there left `metrics` empty while `doProduce` asked
`metricTerm` for
+ // `numOutputRows`. The first force is not always the gate: under AQE it is
a plan-update event
+ // on the pre-stage-creation tree, so a term added here sees more of the
plan than the gate does.
+ private def supportCodegenFailureReason: Option[String] =
decisionLock.synchronized {
+ getTagValue(UnionExec.CODEGEN_FAILURE_REASON).getOrElse {
Review Comment:
Barriers added in `773c28999eb`'s successor. `StampUnionDecisions` is now
listed after each phase that can add a `UnionExec`: after the injected columnar
rules in `QueryExecution.preparations` and in AQE's `postStageCreationRules`,
and after the injected `queryStagePrepRules`. The write-once guard is what
keeps a later pass from moving a decision an earlier consumer planned against.
The cached-scan branch of stage creation needs none, since it rejects a result
that is no longer an `InMemoryTableScanLike`, which is a leaf.
Two halves are tested: a fresh node gets decided at the barrier, and a node
that already decided keeps its answer. The second half is the one that would
catch a lost guard, and I checked that by removing the guard: it fails on the
conf the node was stamped with. The wiring itself is pinned only for the
standard pipeline, by counting the rule in `QueryExecution.preparations`; the
AQE lists are private to `AdaptiveSparkPlanExec`, so those two positions are
not asserted anywhere. Saying so rather than writing a test that would not fail.
Your other five from that pass are in as well: the `SparkPlanInfo` trigger
is now the initial one `SQLExecution` builds before execution rather than the
plan update, the three conf docs describe physical preparation as the boundary,
`maxChildren` has the same-plan flip test (three children prepared at cap 3,
cap lowered to 2, the shell copy still fused with a counted `numOutputRows`),
and the two grammar fixes.
One thing your P2's Risks section anticipated has a concrete instance, and I
would rather put it in front of you than leave it in a comment.
`DisableUnnecessaryBucketedScan` runs after the first stamp, so a union over
two bucketed scans is stamped non-plain from the bucketed partitioning and then
loses it: the gate answers `partitioning-aware`, the union does not fuse and
registers no `numOutputRows`, where re-deriving at the gate would have fused
it. Results are unaffected, since the other branch re-derives and concatenates.
`outputPartitioning`'s scaladoc now records that direction next to the one it
already recorded.
Closing it means separating the two decisions rather than moving the stamp:
freeze the partitioning half where it is, and take the codegen half at the
pre-`CollapseCodegenStages` barrier, which is after every rule that can change
a child's partitioning. That is a design change on top of this one, and it
needs the fused-union-must-not-claim-a-partitioning invariant re-argued, so I
would rather do it as a follow-up unless you want it here.
--
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]
