You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2020/09/25 08:30:44 UTC

[GitHub] [spark] cloud-fan commented on a change in pull request #29860: [SPARK-32984][TESTS][SQL] Improve showing the differences between approved and actual plans of PlanStabilitySuite

cloud-fan commented on a change in pull request #29860:
URL: https://github.com/apache/spark/pull/29860#discussion_r494832113



##########
File path: sql/core/src/test/scala/org/apache/spark/sql/PlanStabilitySuite.scala
##########
@@ -153,23 +154,93 @@ trait PlanStabilitySuite extends TPCDSBase with DisableAdaptiveExecutionSuite {
       // write out for debugging
       FileUtils.writeStringToFile(actualSimplifiedFile, actualSimplified, StandardCharsets.UTF_8)
       FileUtils.writeStringToFile(actualExplainFile, explain, StandardCharsets.UTF_8)
+      val (approvedSimplifiedWithHint, actualSimplifiedWithHint) =
+        addDiffHint(approvedSimplified, actualSimplified)
 
       fail(
         s"""
           |Plans did not match:
           |last approved simplified plan: ${approvedSimplifiedFile.getAbsolutePath}
           |last approved explain plan: ${approvedExplainFile.getAbsolutePath}
           |
-          |$approvedSimplified
+          |$approvedSimplifiedWithHint
           |
           |actual simplified plan: ${actualSimplifiedFile.getAbsolutePath}
           |actual explain plan: ${actualExplainFile.getAbsolutePath}
           |
-          |$actualSimplified
+          |$actualSimplifiedWithHint
         """.stripMargin)
     }
   }
 
+  /**
+   * Add the hint to the simplified plans where they first become different.
+   */
+  private def addDiffHint(approvedSimplified: String, actualSimplified: String)
+    : (String, String) = {
+    // reverse the plan so we can compare the node from the bottom to top

Review comment:
       One hard problem is how to match the lines from both sides. It's possible that the left side has one more node in the middle, so simply matching lines bottom-up may not work. It's like git diff, we should do the match w.r.t. the content, which can be very complicated.
   
   Maybe we should just recommend some online text diff tools in the comment and ask people to use.




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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org