cloud-fan commented on code in PR #58419:
URL: https://github.com/apache/spark/pull/58419#discussion_r4000536014


##########
sql/core/src/main/scala/org/apache/spark/sql/execution/basicPhysicalOperators.scala:
##########
@@ -1035,13 +1038,78 @@ case class UnionExec(children: Seq[SparkPlan]) extends 
SparkPlan with CodegenSup
     }
   }
 
-  // True when the codegen path applies: `outputPartitioning` is 
`UnknownPartitioning`,
-  // and `unionedInputRDD` matches the semantics of `sparkContext.union(...)` 
in `unionRDDs`.
-  // A `KeyedPartitioning` union also uses `sparkContext.union(...)` in 
`unionRDDs`, but
-  // codegen is disabled for it (`supportCodegenFailureReason` reports 
"partitioning-aware"):
-  // the per-partition key descriptor is consumed by a downstream 
`GroupPartitionsExec`, and
-  // keeping these unions out of whole-stage codegen matches the 
`HashPartitioning` union case.
-  private[sql] def isPlainUnion: Boolean = 
outputPartitioning.isInstanceOf[UnknownPartitioning]
+  /**
+   * True when this union behaves as a plain concatenation, so 
`unionedInputRDD` matches the
+   * semantics of `sparkContext.union(...)` in `unionRDDs`. It satisfies the 
partitioning gate on
+   * the codegen path, not the whole of it: `supportCodegenFailureReason` 
still applies its other
+   * checks. When this union merges its children's `KeyedPartitioning` 
instead, it concatenates all
+   * the same, but codegen stays off, with the reason "partitioning-aware", 
because a downstream
+   * `GroupPartitionsExec` consumes its key descriptor.
+   *
+   * Stamped, because the answer moves under its consumers.
+   * `InMemoryTableScanExec.outputPartitioning` reports `UnknownPartitioning` 
while its inner
+   * `AdaptiveSparkPlanExec` has no final plan, so a union can look plain when
+   * `CollapseCodegenStages` gates on it and partitioning-aware by the time 
the stage runs. The
+   * shell that gate builds wraps a `withNewChildren` copy, and a copy that 
re-derived here came
+   * back with empty `metrics` while `doProduce` asked `metricTerm` for 
`numOutputRows`. A fresh
+   * copy inherits the answer instead, since `withNewChildren` ends in 
`copyTagsFrom`.
+   *
+   * `UNION_OUTPUT_PARTITIONING` is read where the decision is stamped rather 
than in
+   * `rawPartitioning`, so it too is fixed once the plan is prepared: `conf` 
is live, and a plan
+   * must execute by the partitioning it was planned against.
+   *
+   * A read before `StampUnionDecisions` answers from the children as they are 
then, and does not
+   * write, so observing an unprepared plan cannot decide anything for the 
prepared one.
+   */
+  private[execution] def isPlainUnion: Boolean = 
stampedDecisions.map(_.plainUnion).getOrElse {
+    !conf.getConf(SQLConf.UNION_OUTPUT_PARTITIONING) ||
+      rawPartitioning.isInstanceOf[UnknownPartitioning]
+  }
+
+  private def stampedDecisions: Option[UnionExec.Decisions] =
+    getTagValue(UnionExec.DECISIONS)
+
+  /**
+   * Fixes this node's decisions for the rest of the plan's life. Called by 
`StampUnionDecisions`,
+   * first right after `EnsureRequirements`, so what the exchanges around this 
union were planned
+   * against is what execution uses. Nothing else writes this tag on an 
existing node, and the
+   * nodes the rule writes are freshly planned and not yet published, so no 
reader can be looking at
+   * one; `metrics` and the codegen gate read it later, and a node that 
already carries it keeps it,
+   * which is how the copy in the codegen shell stays in step with the gate.
+   */
+  private[execution] def stampDecisions(): Unit = if 
(stampedDecisions.isEmpty) {
+    setTagValue(UnionExec.DECISIONS, UnionExec.Decisions(
+      plainUnion = isPlainUnion,
+      unionCodegenEnabled = 
conf.getConf(SQLConf.WHOLESTAGE_UNION_CODEGEN_ENABLED),
+      maxChildren = conf.getConf(SQLConf.WHOLESTAGE_UNION_MAX_CHILDREN)))
+  }
+
+  /**
+   * A node stamped plain reports `UnknownPartitioning` even once its children 
agree on a concrete
+   * one: a fused union concatenates, and claiming their partitioning would 
let a parent skip an
+   * exchange it needs. The cost is SPARK-52921's exchange elimination for 
such a union.
+   *
+   * Only the decision is stamped, never the `Partitioning` itself. AQE 
coalescing changes the
+   * children's `numPartitions` after the stamp, and a stale count is what 
`unionRDDs` would hand
+   * `SQLPartitioningAwareUnionRDD`, which builds exactly that many partitions 
from each child.
+   *
+   * The reverse costs fusion. A rule that runs after the stamp and drops a 
child's partitioning
+   * leaves the node stamped non-plain, so the codegen gate answers 
"partitioning-aware" and
+   * `numOutputRows` goes unregistered, where re-deriving at the gate would 
have fused it.

Review Comment:
   Confirmed: the contrast now uses `whereas`. Resolved.
   
   <!-- SPARK_DEV_REVIEW_REPLY 
{"feedback_id":"inline:3999365353","thread_id":"inline:3999365353","verdict_sha256":"3f145606514841e98aa4f828110c8e9a2ef2a823fe82b8c1858abbafd71ce3c8"}
 -->



-- 
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]

Reply via email to