fix review findings

peter-toth · peter-toth · commit 7187ebd2e053 · 2021-06-18T09:59:22.000+02:00
diff --git a/sql/catalyst/src/main/scala/org/apache/spark/sql/util/ReuseMap.scala b/sql/catalyst/src/main/scala/org/apache/spark/sql/util/ReuseMap.scala
@@ -42,7 +42,7 @@ class ReuseMap[T <: T2, T2 <: QueryPlan[T2]] {
    * @param plan the input plan
    * @return the matching plan or the input plan
    */
-  def lookupOrElseAdd(plan: T): T = {
+  private def lookupOrElseAdd(plan: T): T = {
     val sameSchema = map.getOrElseUpdate(plan.schema, ArrayBuffer())
     val samePlan = sameSchema.find(plan.sameResult)
     if (samePlan.isDefined) {
diff --git a/sql/core/src/main/scala/org/apache/spark/sql/execution/ExplainUtils.scala b/sql/core/src/main/scala/org/apache/spark/sql/execution/ExplainUtils.scala
@@ -38,10 +38,6 @@ object ExplainUtils extends AdaptiveSparkPlanHelper {
    *
    * @param plan Input query plan to process
    * @param append function used to append the explain output
-   *
-   * @return The last generated operation id for this input plan. This is to ensure we
-   *         always assign incrementing unique id to each operator.
-   *
    */
   private def processPlanSkippingSubqueries[T <: QueryPlan[T]](
       plan: T,
diff --git a/sql/core/src/main/scala/org/apache/spark/sql/execution/reuse/ReuseExchangeAndSubquery.scala b/sql/core/src/main/scala/org/apache/spark/sql/execution/reuse/ReuseExchangeAndSubquery.scala
@@ -26,6 +26,11 @@ import org.apache.spark.sql.util.ReuseMap
 /**
  * Find out duplicated exchanges and subqueries in the whole spark plan including subqueries, then
  * use the same exchange or subquery for all the references.
+ *
+ * Note that the Spark plan is a mutually recursive data structure:
+ * SparkPlan -> Expr -> Subquery -> SparkPlan -> Expr -> Subquery -> ...
+ * Therefore, in this rule, we recursively rewrite the exchanges and subqueries in a bottom-up way,
+ * in one go.
  */
 case object ReuseExchangeAndSubquery extends Rule[SparkPlan] {
 
