[SPARK-18292][SQL] LogicalPlanToSQLSuite should not use resource dependent path for golden file generation #15789

dongjoon-hyun · 2016-11-06T09:04:39Z

What changes were proposed in this pull request?

LogicalPlanToSQLSuite uses the following command to update the existing answer files.

SPARK_GENERATE_GOLDEN_FILES=1 build/sbt "hive/test-only *LogicalPlanToSQLSuite"

However, after introducing getTestResourcePath, it fails to update the previous golden answer files in the predefined directory. This issue aims to fix that.

How was this patch tested?

It's a testsuite update. Manual.

…ndent path for golden file generation

dongjoon-hyun · 2016-11-06T09:06:52Z

Hi, @gatorsmile .
I create a PR for LogicalPlanToSQLSuite part from #15546 .

SparkQA · 2016-11-06T09:06:56Z

Test build #68233 has finished for PR 15789 at commit bd7258d.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

dongjoon-hyun · 2016-11-06T09:09:55Z

Oh, you mean the the following. I'll fix this PR again. Sorry, @gatorsmile .

  private val baseResourcePath = {
    // If regenerateGoldenFiles is true, we must be running this in SBT and we use hard-coded
    // relative path. Otherwise, we use classloader's getResource to find the location.
    if (regenerateGoldenFiles) {
      java.nio.file.Paths.get("src", "test", "resources", "sql-tests").toFile
    } else {
      val res = getClass.getClassLoader.getResource("sql-tests")
      new File(res.getFile)
    }
  }

dongjoon-hyun · 2016-11-06T09:22:10Z

Retest this please

dongjoon-hyun · 2016-11-06T09:25:34Z

It's fixed like SQLQueryTestSuite.

dongjoon-hyun · 2016-11-06T09:38:23Z

Retest this please.

SparkQA · 2016-11-06T09:48:43Z

Test build #68232 has finished for PR 15789 at commit 725cd45.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

dongjoon-hyun · 2016-11-06T09:54:19Z

There occurs four randomized aggregation tests failures due to OutOfMemoryError. But, it seems to be irrelevant. I'll wait the next test running currently.

[info] - randomized aggregation test - [typed, with partial + safe] - with grouping keys - with non-empty input *** FAILED *** (1 second, 512 milliseconds)
[info]   org.apache.spark.SparkException: Job aborted due to stage failure: Task 4 in stage 1897.0 failed 1 times, most recent failure: Lost task 4.0 in stage 1897.0 (TID 6408, localhost, executor driver): java.lang.OutOfMemoryError: Java heap space

dongjoon-hyun · 2016-11-06T10:38:43Z

Retest this please

SparkQA · 2016-11-06T12:14:42Z

Test build #68237 has finished for PR 15789 at commit bd7258d.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

srowen · 2016-11-06T14:08:22Z

sql/hive/src/test/scala/org/apache/spark/sql/catalyst/LogicalPlanToSQLSuite.scala

+    // If regenerateGoldenFiles is true, we must be running this in SBT and we use hard-coded
+    // relative path. Otherwise, we use classloader's getResource to find the location.
+    if (regenerateGoldenFiles) {
+      java.nio.file.Paths.get("src", "test", "resources", "sqlgen").toFile.getCanonicalPath


OK I see. This seems OK. I think it's OK to just do new File("src/test/resources/sqlgen") here instead, since that's what other tests do.

Thank you for review, @srowen .
For this one, there is a request in #15546 to follow the way SQLQueryTestSuite.

https://github.com/apache/spark/blob/master/sql/core/src/test/scala/org/apache/spark/sql/SQLQueryTestSuite.scala#L88-L93

gatorsmile · 2016-11-06T15:08:29Z

sql/hive/src/test/scala/org/apache/spark/sql/catalyst/LogicalPlanToSQLSuite.scala

+    // If regenerateGoldenFiles is true, we must be running this in SBT and we use hard-coded
+    // relative path. Otherwise, we use classloader's getResource to find the location.
+    if (regenerateGoldenFiles) {
+      java.nio.file.Paths.get("src", "test", "resources", "sqlgen").toFile.getCanonicalPath


Just want to confirm that you already tested it in your private branch, right?

I checked that. But, to make it sure, I'll check it again right now.

Yep. I tested this in my private branch in a separate folder. When I changed the query for range, the output file is changed like the following.

SPARK-18292-SQLGEN:SPARK-18292$ pwd /Users/dhyun/SPARK-18292-SQLGEN SPARK-18292-SQLGEN:SPARK-18292$ git branch * SPARK-18292 master SPARK-18292-SQLGEN:SPARK-18292$ pwd /Users/dhyun/SPARK-18292-SQLGEN SPARK-18292-SQLGEN:SPARK-18292$ git status On branch SPARK-18292 Changes not staged for commit: (use "git add <file>..." to update what will be committed) (use "git checkout -- <file>..." to discard changes in working directory) modified: sql/hive/src/test/resources/sqlgen/range.sql modified: sql/hive/src/test/scala/org/apache/spark/sql/catalyst/LogicalPlanToSQLSuite.scala

gatorsmile · 2016-11-06T15:09:16Z

LGTM except a comment.

dongjoon-hyun · 2016-11-06T17:36:21Z

Thank you, @srowen and @gatorsmile .
I replied the comment.
For java.nio.file.Paths.get("src", "test", "resources", "sqlgen").toFile.getCanonicalPath, could you give me advice again if I need to change that again?

srowen

Oh OK, let's leave it then.

dongjoon-hyun · 2016-11-06T20:33:15Z

Thank you, @srowen !

rxin · 2016-11-06T21:41:33Z

Can you explain what the actual issue is? I read the description and still have no idea.

dongjoon-hyun · 2016-11-06T22:25:41Z

Hi, @rxin .

Yep. The actual issue is the current code generates the new golden files in the following folder. So, we cannot identify which file is changed and cannot commit the newly generated files easily.

.../spark/sql/hive/target/scala-2.11/test-classes/sqlgen

When we added this module initially, we faced the exactly same issue and decided to use the absolute path and to assume the following command in the source tree root.

SPARK_GENERATE_GOLDEN_FILES=1 build/sbt "hive/test-only *LogicalPlanToSQLSuite"

srowen · 2016-11-08T14:07:08Z

I'll merge if there are no further comments. To restate -- basically the test helper function returns a path that is perfectly fine for reading test data, and is flexible because it doesn't depend on what the current working dir is. However it won't work as a location to write to, which is only rarely ever needed like in cases like this.

dongjoon-hyun · 2016-11-08T17:49:16Z

Thank you, @srowen !

srowen · 2016-11-09T17:48:32Z

Merged to master/2.1

…ndent path for golden file generation ## What changes were proposed in this pull request? `LogicalPlanToSQLSuite` uses the following command to update the existing answer files. ```bash SPARK_GENERATE_GOLDEN_FILES=1 build/sbt "hive/test-only *LogicalPlanToSQLSuite" ``` However, after introducing `getTestResourcePath`, it fails to update the previous golden answer files in the predefined directory. This issue aims to fix that. ## How was this patch tested? It's a testsuite update. Manual. Author: Dongjoon Hyun <dongjoon@apache.org> Closes #15789 from dongjoon-hyun/SPARK-18292. (cherry picked from commit 02c5325) Signed-off-by: Sean Owen <sowen@cloudera.com>

dongjoon-hyun · 2016-11-09T17:50:04Z

Thank you for review and merging, @srowen , @gatorsmile , and @rxin .

…ndent path for golden file generation ## What changes were proposed in this pull request? `LogicalPlanToSQLSuite` uses the following command to update the existing answer files. ```bash SPARK_GENERATE_GOLDEN_FILES=1 build/sbt "hive/test-only *LogicalPlanToSQLSuite" ``` However, after introducing `getTestResourcePath`, it fails to update the previous golden answer files in the predefined directory. This issue aims to fix that. ## How was this patch tested? It's a testsuite update. Manual. Author: Dongjoon Hyun <dongjoon@apache.org> Closes apache#15789 from dongjoon-hyun/SPARK-18292.

dongjoon-hyun added 2 commits November 6, 2016 00:59

[SPARK-18292][SQL] LogicalPlanToSQLSuite should not use resource depe…

725cd45

…ndent path for golden file generation

Fix

bd7258d

srowen reviewed Nov 6, 2016

View reviewed changes

gatorsmile reviewed Nov 6, 2016

View reviewed changes

srowen approved these changes Nov 6, 2016

View reviewed changes

asfgit closed this in 02c5325 Nov 9, 2016

dongjoon-hyun deleted the SPARK-18292 branch November 16, 2016 19:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-18292][SQL] LogicalPlanToSQLSuite should not use resource dependent path for golden file generation #15789

[SPARK-18292][SQL] LogicalPlanToSQLSuite should not use resource dependent path for golden file generation #15789

dongjoon-hyun commented Nov 6, 2016 •

edited

dongjoon-hyun commented Nov 6, 2016

SparkQA commented Nov 6, 2016

dongjoon-hyun commented Nov 6, 2016

dongjoon-hyun commented Nov 6, 2016

dongjoon-hyun commented Nov 6, 2016

dongjoon-hyun commented Nov 6, 2016

SparkQA commented Nov 6, 2016

dongjoon-hyun commented Nov 6, 2016

dongjoon-hyun commented Nov 6, 2016

SparkQA commented Nov 6, 2016

srowen Nov 6, 2016

dongjoon-hyun Nov 6, 2016

gatorsmile Nov 6, 2016

dongjoon-hyun Nov 6, 2016

dongjoon-hyun Nov 6, 2016

gatorsmile commented Nov 6, 2016

dongjoon-hyun commented Nov 6, 2016

srowen left a comment

dongjoon-hyun commented Nov 6, 2016

rxin commented Nov 6, 2016

dongjoon-hyun commented Nov 6, 2016

srowen commented Nov 8, 2016

dongjoon-hyun commented Nov 8, 2016

srowen commented Nov 9, 2016

dongjoon-hyun commented Nov 9, 2016

[SPARK-18292][SQL] LogicalPlanToSQLSuite should not use resource dependent path for golden file generation #15789

[SPARK-18292][SQL] LogicalPlanToSQLSuite should not use resource dependent path for golden file generation #15789

Conversation

dongjoon-hyun commented Nov 6, 2016 • edited

What changes were proposed in this pull request?

How was this patch tested?

dongjoon-hyun commented Nov 6, 2016

SparkQA commented Nov 6, 2016

dongjoon-hyun commented Nov 6, 2016

dongjoon-hyun commented Nov 6, 2016

dongjoon-hyun commented Nov 6, 2016

dongjoon-hyun commented Nov 6, 2016

SparkQA commented Nov 6, 2016

dongjoon-hyun commented Nov 6, 2016

dongjoon-hyun commented Nov 6, 2016

SparkQA commented Nov 6, 2016

srowen Nov 6, 2016

Choose a reason for hiding this comment

dongjoon-hyun Nov 6, 2016

Choose a reason for hiding this comment

gatorsmile Nov 6, 2016

Choose a reason for hiding this comment

dongjoon-hyun Nov 6, 2016

Choose a reason for hiding this comment

dongjoon-hyun Nov 6, 2016

Choose a reason for hiding this comment

gatorsmile commented Nov 6, 2016

dongjoon-hyun commented Nov 6, 2016

srowen left a comment

Choose a reason for hiding this comment

dongjoon-hyun commented Nov 6, 2016

rxin commented Nov 6, 2016

dongjoon-hyun commented Nov 6, 2016

srowen commented Nov 8, 2016

dongjoon-hyun commented Nov 8, 2016

srowen commented Nov 9, 2016

dongjoon-hyun commented Nov 9, 2016

dongjoon-hyun commented Nov 6, 2016 •

edited