From dea1f337debbd2d0bb2488117f80ec5ded4f76a5 Mon Sep 17 00:00:00 2001 From: Mark Grover Date: Tue, 25 Oct 2016 06:31:47 -0700 Subject: [PATCH 1/2] SPARK-18093][SQL] Fix default value test in SQLConfSuite to work regardless of warehouse dir's existence Appending a trailing slash, if there already isn't one for the sake comparison of the two paths. It doesn't take away from the essence of the check, but removes any potential mismatch due to lack of trailing slash. --- .../org/apache/spark/sql/internal/SQLConfSuite.scala | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/sql/core/src/test/scala/org/apache/spark/sql/internal/SQLConfSuite.scala b/sql/core/src/test/scala/org/apache/spark/sql/internal/SQLConfSuite.scala index a89a43fa1e777..47c6805715657 100644 --- a/sql/core/src/test/scala/org/apache/spark/sql/internal/SQLConfSuite.scala +++ b/sql/core/src/test/scala/org/apache/spark/sql/internal/SQLConfSuite.scala @@ -215,12 +215,18 @@ class SQLConfSuite extends QueryTest with SharedSQLContext { } test("default value of WAREHOUSE_PATH") { + def appendTrailingSlashIfNecessary(path: String): String = { + if (!path.endsWith("/")) path + "/" else path + } + val original = spark.conf.get(SQLConf.WAREHOUSE_PATH) try { // to get the default value, always unset it spark.conf.unset(SQLConf.WAREHOUSE_PATH.key) - assert(new Path(Utils.resolveURI("spark-warehouse")).toString === - spark.sessionState.conf.warehousePath + "/") + // JVM adds a trailing slash if the directory exists and leaves it as-is, if it doesn't + // In our comparison, add trailing slash to both sides if necessary, to account for both cases + assert(appendTrailingSlashIfNecessary(new Path(Utils.resolveURI("spark-warehouse")) + .toString) === appendTrailingSlashIfNecessary(spark.sessionState.conf.warehousePath)) } finally { sql(s"set ${SQLConf.WAREHOUSE_PATH}=$original") } From 707f1ee08730a528148ef936d18ffffc1e52c074 Mon Sep 17 00:00:00 2001 From: Mark Grover Date: Tue, 25 Oct 2016 15:06:17 -0700 Subject: [PATCH 2/2] Removing trailing slash instead of adding one --- .../org/apache/spark/sql/internal/SQLConfSuite.scala | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/sql/core/src/test/scala/org/apache/spark/sql/internal/SQLConfSuite.scala b/sql/core/src/test/scala/org/apache/spark/sql/internal/SQLConfSuite.scala index 47c6805715657..11d4693f1c2a3 100644 --- a/sql/core/src/test/scala/org/apache/spark/sql/internal/SQLConfSuite.scala +++ b/sql/core/src/test/scala/org/apache/spark/sql/internal/SQLConfSuite.scala @@ -215,18 +215,15 @@ class SQLConfSuite extends QueryTest with SharedSQLContext { } test("default value of WAREHOUSE_PATH") { - def appendTrailingSlashIfNecessary(path: String): String = { - if (!path.endsWith("/")) path + "/" else path - } val original = spark.conf.get(SQLConf.WAREHOUSE_PATH) try { // to get the default value, always unset it spark.conf.unset(SQLConf.WAREHOUSE_PATH.key) // JVM adds a trailing slash if the directory exists and leaves it as-is, if it doesn't - // In our comparison, add trailing slash to both sides if necessary, to account for both cases - assert(appendTrailingSlashIfNecessary(new Path(Utils.resolveURI("spark-warehouse")) - .toString) === appendTrailingSlashIfNecessary(spark.sessionState.conf.warehousePath)) + // In our comparison, strip trailing slash off of both sides, to account for such cases + assert(new Path(Utils.resolveURI("spark-warehouse")).toString.stripSuffix("/") === spark + .sessionState.conf.warehousePath.stripSuffix("/")) } finally { sql(s"set ${SQLConf.WAREHOUSE_PATH}=$original") }