[GitHub] [spark] Ngone51 commented on a change in pull request #29537: [SPARK-32466][SQL][FOLLOW-UP] Normalize Location info in explain plan

2020-08-25 Thread GitBox 


Ngone51 commented on a change in pull request #29537:
URL: https://github.com/apache/spark/pull/29537#discussion_r476214160



##
File path: sql/core/src/test/scala/org/apache/spark/sql/PlanStabilitySuite.scala
##
@@ -207,7 +209,7 @@ trait PlanStabilitySuite extends TPCDSBase with 
DisableAdaptiveExecutionSuite {
  *   WholeStageCodegen
  * Project [c_customer_id]
  */
-def getSimplifiedPlan(node: SparkPlan, depth: Int): String = {
+def simplifyNode(node: SparkPlan, depth: Int): String = {

Review comment:
   Address the comment from 
https://github.com/apache/spark/pull/29270#discussion_r476176111





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:
us...@infra.apache.org



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

[GitHub] [spark] Ngone51 commented on a change in pull request #29537: [SPARK-32466][SQL][FOLLOW-UP] Normalize Location info in explain plan

2020-08-25 Thread GitBox 


Ngone51 commented on a change in pull request #29537:
URL: https://github.com/apache/spark/pull/29537#discussion_r476213911



##
File path: sql/core/src/test/scala/org/apache/spark/sql/PlanStabilitySuite.scala
##
@@ -102,8 +104,8 @@ trait PlanStabilitySuite extends TPCDSBase with 
DisableAdaptiveExecutionSuite {
 
   private def isApproved(dir: File, actualSimplifiedPlan: String): Boolean = {
 val file = new File(dir, "simplified.txt")
-val approved = FileUtils.readFileToString(file, StandardCharsets.UTF_8)
-approved == actualSimplifiedPlan
+val expected = FileUtils.readFileToString(file, StandardCharsets.UTF_8)
+expected == actualSimplifiedPlan

Review comment:
   Address the comment from 
https://github.com/apache/spark/pull/29270#discussion_r476178381





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:
us...@infra.apache.org



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

[GitHub] [spark] Ngone51 commented on a change in pull request #29537: [SPARK-32466][SQL][FOLLOW-UP] Normalize Location info in explain plan

2020-08-25 Thread GitBox 


Ngone51 commented on a change in pull request #29537:
URL: https://github.com/apache/spark/pull/29537#discussion_r476214029



##
File path: sql/core/src/test/scala/org/apache/spark/sql/PlanStabilitySuite.scala
##
@@ -115,7 +117,7 @@ trait PlanStabilitySuite extends TPCDSBase with 
DisableAdaptiveExecutionSuite {
* @param explain the full explain output; this is saved to help debug later 
as the simplified
*plan is not too useful for debugging
*/
-  private def generateApprovedPlanFile(plan: SparkPlan, name: String, explain: 
String): Unit = {
+  private def generateGoldenFile(plan: SparkPlan, name: String, explain: 
String): Unit = {

Review comment:
   Address the comment from 
https://github.com/apache/spark/pull/29270#discussion_r476174979





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:
us...@infra.apache.org



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

[GitHub] [spark] Ngone51 commented on a change in pull request #29537: [SPARK-32466][SQL][FOLLOW-UP] Normalize Location info in explain plan

2020-08-25 Thread GitBox 


Ngone51 commented on a change in pull request #29537:
URL: https://github.com/apache/spark/pull/29537#discussion_r476211215



##
File path: 
sql/core/src/test/resources/tpcds-plan-stability/approved-plans-v1_4/q11.sf100/explain.txt
##
@@ -89,394 +89,394 @@ TakeOrderedAndProject (87)
 
 
 (1) Scan parquet default.store_sales
-Output [4]: [ss_sold_date_sk#1, ss_customer_sk#2, ss_ext_discount_amt#3, 
ss_ext_list_price#4]
+Output [4]: [ss_sold_date_sk#x, ss_customer_sk#x, ss_ext_discount_amt#x, 
ss_ext_list_price#x]

Review comment:
   It's the formatted explained plan. So it doesn't really have `Partition 
Statistics` and `codegenStageIds`.





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:
us...@infra.apache.org



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

[GitHub] [spark] Ngone51 commented on a change in pull request #29537: [SPARK-32466][SQL][FOLLOW-UP] Normalize Location info in explain plan

2020-08-25 Thread GitBox 


Ngone51 commented on a change in pull request #29537:
URL: https://github.com/apache/spark/pull/29537#discussion_r476196562



##
File path: 
sql/core/src/test/resources/tpcds-plan-stability/approved-plans-v1_4/q11.sf100/explain.txt
##
@@ -89,394 +89,394 @@ TakeOrderedAndProject (87)
 
 
 (1) Scan parquet default.store_sales
-Output [4]: [ss_sold_date_sk#1, ss_customer_sk#2, ss_ext_discount_amt#3, 
ss_ext_list_price#4]
+Output [4]: [ss_sold_date_sk#x, ss_customer_sk#x, ss_ext_discount_amt#x, 
ss_ext_list_price#x]

Review comment:
   Ok





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:
us...@infra.apache.org



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

[GitHub] [spark] Ngone51 commented on a change in pull request #29537: [SPARK-32466][SQL][FOLLOW-UP] Normalize Location info in explain plan

2020-08-24 Thread GitBox 


Ngone51 commented on a change in pull request #29537:
URL: https://github.com/apache/spark/pull/29537#discussion_r476117593



##
File path: 
sql/core/src/test/resources/tpcds-plan-stability/approved-plans-v1_4/q11.sf100/explain.txt
##
@@ -89,394 +89,394 @@ TakeOrderedAndProject (87)
 
 
 (1) Scan parquet default.store_sales
-Output [4]: [ss_sold_date_sk#1, ss_customer_sk#2, ss_ext_discount_amt#3, 
ss_ext_list_price#4]
+Output [4]: [ss_sold_date_sk#x, ss_customer_sk#x, ss_ext_discount_amt#x, 
ss_ext_list_price#x]

Review comment:
   Sorry for bringing massive changes again.
   
   Do you think we should avoid this change? If it should, I think we could 
make a custom copy of `replaceNotIncludedMsg` in `PlanStabilitySuite` instead 
of resue.





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:
us...@infra.apache.org



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