peter-toth commented on code in PR #57753:
URL: https://github.com/apache/spark/pull/57753#discussion_r3730359805


##########
sql/core/src/main/scala/org/apache/spark/sql/execution/planmerging/PlanMerger.scala:
##########
@@ -784,14 +828,56 @@ class PlanMerger(
       // turned every other relation into a DataSourceV2ScanRelation), so 
recover it by type here
       // rather than carrying it on DSv2DeferredScan.
       child.collectFirst { case r: DataSourceV2Relation => r }.flatMap { 
relation =>
-        tryBuildMergedDSv2Scan(relation, d.unionAttrs, d.strictFilters, 
bestEffortFilter)
-          .orElse(tryBuildMergedDSv2Scan(relation, d.unionAttrs, 
d.strictFilters, None))
+        tryBuildMergedDSv2Scan(relation, d.unionAttrs, d.strictFilters, 
bestEffortFilter,
+            d.requiredKeyGroupedPartitioning, d.requiredOrdering)
+          .orElse(tryBuildMergedDSv2Scan(relation, d.unionAttrs, 
d.strictFilters, None,
+            d.requiredKeyGroupedPartitioning, d.requiredOrdering))
           .map { built =>
             child.transformUp { case r: DataSourceV2Relation if r eq relation 
=> built }
           }
       }
   }
 
+  // The key-grouped partitioning the merged scan must reproduce to keep both 
inputs not-worse: they
+  // must be equal (bucketing is a table property), so a differing non-empty 
pair is INCOMPATIBLE
+  // (None); an empty side imposes no constraint. Compared canonically in cp's 
relation space (np's
+  // report was remapped into it by the caller).
+  private def combineRequiredKeyGroupedPartitioning(
+      a: Seq[Expression], b: Seq[Expression]): Option[Seq[Expression]] = {
+    if (a.isEmpty) Some(b)
+    else if (b.isEmpty) Some(a)
+    else if (a.map(_.canonicalized) == b.map(_.canonicalized)) Some(a)
+    else None
+  }
+
+  // The ordering the merged scan must satisfy to keep both inputs not-worse: 
the stronger of the
+  // two (the one that satisfies the other -- satisfying it implies satisfying 
the weaker). If
+  // neither satisfies the other they are INCOMPATIBLE (None). An empty 
ordering never constrains.
+  private def combineRequiredOrdering(
+      a: Seq[SortOrder], b: Seq[SortOrder]): Option[Seq[SortOrder]] = {
+    if (SortOrder.orderingSatisfies(a, b)) Some(a)
+    else if (SortOrder.orderingSatisfies(b, a)) Some(b)
+    else None
+  }
+
+  // True when the rebuilt merged scan does not reproduce the required 
key-grouped partitioning, or
+  // does not satisfy the required ordering (the combined report the merge 
must preserve, computed
+  // at the leaf). Gated per dimension by the dsv2ScanMerge degradation 
configs; an empty required
+  // report imposes no constraint. Compared in cp's relation space.
+  private def mergeDegradesReporting(
+      merged: DataSourceV2ScanRelation,
+      requiredKeyGroupedPartitioning: Seq[Expression],
+      requiredOrdering: Seq[SortOrder]): Boolean = {
+    val kgpDegraded = !dsv2AllowKeyGroupedPartitioningDegradation &&
+      requiredKeyGroupedPartitioning.nonEmpty &&

Review Comment:
   Both halves are answered in the summary comment — short version: the gain is 
real and is now documented as deliberate, and emptying the requirement when the 
inputs conflict is a no-op, because that dimension's config being on already 
short-circuits `mergeDegradesReporting`.
   



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