[SPARK-22601][SQL] Data load is getting displayed successful on providing non existing nonlocal file path #19823

sujith71955 · 2017-11-26T18:18:18Z

What changes were proposed in this pull request?

When user tries to load data with a non existing hdfs file path system is not validating it and the load command operation is getting successful.
This is misleading to the user. already there is a validation in the scenario of none existing local file path. This PR has added validation in the scenario of nonexisting hdfs file path

How was this patch tested?

UT has been added for verifying the issue, also snapshots has been added after the verification in a spark yarn cluster

sujith71955 · 2017-11-26T18:19:39Z

srowen

I still kind of wonder whether this causes a problem in some scenario where the path will be created later after this is evaluated, but I don't know this path well enough to say for sure

srowen · 2017-11-26T20:08:56Z

sql/core/src/main/scala/org/apache/spark/sql/execution/command/tables.scala

+          val hadoopConf = sparkSession.sessionState.newHadoopConf()
+          val srcPath = new Path(path)
+          val fs = srcPath.getFileSystem(hadoopConf)
+          if(!fs.exists(srcPath)) {


Nit: space after if

srowen · 2017-11-26T20:09:09Z

sql/core/src/main/scala/org/apache/spark/sql/execution/command/tables.scala

@@ -341,6 +341,12 @@ case class LoadDataCommand(
      } else {
        val uri = new URI(path)
        if (uri.getScheme() != null && uri.getAuthority() != null) {
+          val hadoopConf = sparkSession.sessionState.newHadoopConf()


You can probably lift this out of the if-else and use it in the other branch too

SparkQA · 2017-11-26T20:13:13Z

Test build #3991 has finished for PR 19823 at commit bed0e65.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

gatorsmile · 2017-11-26T20:27:19Z

sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala

@@ -2624,7 +2624,13 @@ class SQLQuerySuite extends QueryTest with SharedSQLContext {
    val e = intercept[AnalysisException](sql("SELECT nvl(1, 2, 3)"))
    assert(e.message.contains("Invalid number of arguments"))
  }
-
+  test("load command invalid path validation ") {


Move this to DDLSuite?

nit: also blank lines

gatorsmile · 2017-11-26T20:27:57Z

sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala

+    withTable("tbl") {
+    sql("CREATE TABLE tbl(i INT, j STRING) USING parquet")
+    val e = intercept[AnalysisException](sql("load data inpath 'hdfs://localhost/doesnotexist.csv' into table tbl"))
+    assert(e.message.contains("LOAD DATA input path does not exist"))


indents between 2629 and 2631

gatorsmile · 2017-11-26T20:37:36Z

sql/core/src/main/scala/org/apache/spark/sql/execution/command/tables.scala

+          val srcPath = new Path(path)
+          val fs = srcPath.getFileSystem(hadoopConf)
+          if(!fs.exists(srcPath)) {
+             throw new AnalysisException(s"LOAD DATA input path does not exist: $path")


two space indents.

gatorsmile · 2017-11-26T20:39:48Z

sql/core/src/main/scala/org/apache/spark/sql/execution/command/tables.scala

@@ -341,6 +341,12 @@ case class LoadDataCommand(
      } else {
        val uri = new URI(path)
        if (uri.getScheme() != null && uri.getAuthority() != null) {
+          val hadoopConf = sparkSession.sessionState.newHadoopConf()


gatorsmile · 2017-11-26T20:40:22Z

Thanks for fixing this!

HyukjinKwon · 2017-11-27T01:29:45Z

sql/core/src/main/scala/org/apache/spark/sql/execution/command/tables.scala

@@ -341,6 +341,12 @@ case class LoadDataCommand(
      } else {
        val uri = new URI(path)
        if (uri.getScheme() != null && uri.getAuthority() != null) {
+          val hadoopConf = sparkSession.sessionState.newHadoopConf()
+          val srcPath = new Path(path)


nit: Let's use new Path(uri). I think we better validate uri in this scope.

But will this lose the normalization in new Path(path) ? Or normalization in URI covers it?

I think here path is an URI here otherwise val uri = new URI(path) should fail, and we return uri. So, I think we should check if the uri is valid.

HyukjinKwon · 2017-11-27T12:02:13Z

sql/core/src/main/scala/org/apache/spark/sql/execution/command/tables.scala

+        val srcPath = new Path(uri)
+        val fs = srcPath.getFileSystem(hadoopConf)
+        if (!fs.exists(srcPath)) {
+            throw new AnalysisException(s"LOAD DATA input path does not exist: $path")


tiny nit: double spaces

sujith71955 · 2017-11-27T12:26:04Z

Thanks for the comments guys, i am working on it.,will update the PR based on comments.

sujith71955 · 2017-11-27T12:27:31Z

Basically this validation stands good for both cases where scheme can come as null and not null, i will update the logic as Sean told. Thanks

sujith71955 · 2017-11-27T13:27:07Z

retest this please

sujith71955 · 2017-11-28T11:04:56Z

@gatorsmile @HyukjinKwon @srowen Please review as i modified the code as per provided comments. thanks

SparkQA · 2017-11-28T17:04:36Z

Test build #3996 has finished for PR 19823 at commit cc1b176.

This patch fails Scala style tests.
This patch does not merge cleanly.
This patch adds no public classes.

wzhfy · 2017-11-29T02:13:43Z

sql/core/src/test/scala/org/apache/spark/sql/execution/command/DDLSuite.scala

@@ -2392,5 +2392,13 @@ abstract class DDLSuite extends QueryTest with SQLTestUtils {
        }
      }
    }
+    test("load command invalid path validation ") {


@sujith71955 according to Jenkins, there's a whitespace at end of line

updated it. In my local build it was working fine. Thanks for the feedback

wzhfy · 2017-11-29T02:16:44Z

sql/core/src/test/scala/org/apache/spark/sql/execution/command/DDLSuite.scala

@@ -2392,5 +2392,13 @@ abstract class DDLSuite extends QueryTest with SQLTestUtils {
        }
      }
    }
+    test("load command invalid path validation ") {


nit: insert an empty line before the test

fixed. thanks

gatorsmile · 2017-11-29T04:56:09Z

retest this please

SparkQA · 2017-11-29T06:21:07Z

Test build #84283 has finished for PR 19823 at commit a60843c.

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

srowen · 2017-11-29T16:18:31Z

This is another problem downloading from an Apache mirror. I can add retry logic, but, hm, this seems to be a kind of fragile thing, to download several huge tarballs in unit tests. CC @cloud-fan

cloud-fan · 2017-11-29T17:28:00Z

How about we make it a runnable class and only run it occasionally or before the release?

cloud-fan · 2017-11-29T17:29:25Z

BTW this PR LGTM

SparkQA · 2017-11-29T17:55:16Z

Test build #3999 has finished for PR 19823 at commit 2a84256.

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

SparkQA · 2017-11-29T20:39:48Z

Test build #4000 has finished for PR 19823 at commit 2a84256.

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

gatorsmile · 2017-11-29T21:37:03Z

sql/core/src/test/scala/org/apache/spark/sql/execution/command/DDLSuite.scala

@@ -2392,5 +2392,14 @@ abstract class DDLSuite extends QueryTest with SQLTestUtils {
        }
      }
    }
+
+    test("load command for non local invalid path validation") {


Please move this test case out of

Seq(true, false).foreach { caseSensitive => ... }

Right gatorsmile, this is why test case was failing, one more change i have done,Since i am dealing with hive table i moved this test case to HiveDDLSuite.scala, hope its fine. Thanks for the review

…ding non existing hdfs file path ## What changes were proposed in this pull request? When user tries to load data with a non existing hdfs file path system is not validating it and the load command operation is getting successful. This is misleading to the user. already there is a validation in the scenario of local file path. This PR has added validation in the scenario of hdfs file path ## How was this patch tested? existing tests are present to verify the impact and manually the scenario is been verified in hdfs cluster

HyukjinKwon · 2017-11-30T22:37:07Z

ok to test

gatorsmile · 2017-12-01T00:57:49Z

LGTM pending Jenkins

SparkQA · 2017-12-01T01:29:15Z

Test build #84356 has finished for PR 19823 at commit f6eb4ad.

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

HyukjinKwon

another lgtm

sujith71955 · 2017-12-01T03:51:50Z

Thanks all for the review and guidance.

gatorsmile · 2017-12-01T04:47:09Z

Thanks! Merged to master/2.2

…ding non existing nonlocal file path ## What changes were proposed in this pull request? When user tries to load data with a non existing hdfs file path system is not validating it and the load command operation is getting successful. This is misleading to the user. already there is a validation in the scenario of none existing local file path. This PR has added validation in the scenario of nonexisting hdfs file path ## How was this patch tested? UT has been added for verifying the issue, also snapshots has been added after the verification in a spark yarn cluster Author: sujith71955 <[email protected]> Closes #19823 from sujith71955/master_LoadComand_Issue. (cherry picked from commit 16adaf6) Signed-off-by: gatorsmile <[email protected]>

…ding non existing nonlocal file path ## What changes were proposed in this pull request? When user tries to load data with a non existing hdfs file path system is not validating it and the load command operation is getting successful. This is misleading to the user. already there is a validation in the scenario of none existing local file path. This PR has added validation in the scenario of nonexisting hdfs file path ## How was this patch tested? UT has been added for verifying the issue, also snapshots has been added after the verification in a spark yarn cluster Author: sujith71955 <[email protected]> Closes apache#19823 from sujith71955/master_LoadComand_Issue. (cherry picked from commit 16adaf6) Signed-off-by: gatorsmile <[email protected]>

sujith71955 force-pushed the master_LoadComand_Issue branch from 5b247a8 to bed0e65 Compare November 26, 2017 18:34

srowen reviewed Nov 26, 2017

View reviewed changes

gatorsmile reviewed Nov 26, 2017

View reviewed changes

HyukjinKwon reviewed Nov 27, 2017

View reviewed changes

sujith71955 force-pushed the master_LoadComand_Issue branch from bed0e65 to 1540c16 Compare November 27, 2017 11:25

sujith71955 changed the title ~~[SPARK-22601][SQL] Data load is getting displayed successful on providing non existing hdfs file path~~ [WIP][SPARK-22601][SQL] Data load is getting displayed successful on providing non existing hdfs file path Nov 27, 2017

HyukjinKwon reviewed Nov 27, 2017

View reviewed changes

sujith71955 force-pushed the master_LoadComand_Issue branch 2 times, most recently from fa4b63d to cc1b176 Compare November 27, 2017 12:18

sujith71955 changed the title ~~[WIP][SPARK-22601][SQL] Data load is getting displayed successful on providing non existing hdfs file path~~ [SPARK-22601][SQL] Data load is getting displayed successful on providing non existing hdfs file path Nov 27, 2017

sujith71955 force-pushed the master_LoadComand_Issue branch 2 times, most recently from aca9c27 to a315bf3 Compare November 28, 2017 17:44

wzhfy reviewed Nov 29, 2017

View reviewed changes

sujith71955 force-pushed the master_LoadComand_Issue branch from a315bf3 to a60843c Compare November 29, 2017 04:46

sujith71955 force-pushed the master_LoadComand_Issue branch from a60843c to 2a84256 Compare November 29, 2017 11:26

gatorsmile reviewed Nov 29, 2017

View reviewed changes

sujith71955 force-pushed the master_LoadComand_Issue branch from 2a84256 to f6eb4ad Compare November 30, 2017 20:03

sujith71955 changed the title ~~[SPARK-22601][SQL] Data load is getting displayed successful on providing non existing hdfs file path~~ [SPARK-22601][SQL] Data load is getting displayed successful on providing non existing nonlocal file path Nov 30, 2017

HyukjinKwon approved these changes Dec 1, 2017

View reviewed changes

asfgit closed this in 16adaf6 Dec 1, 2017

[SPARK-22601][SQL] Data load is getting displayed successful on providing non existing nonlocal file path #19823

[SPARK-22601][SQL] Data load is getting displayed successful on providing non existing nonlocal file path #19823

Uh oh!

Conversation

sujith71955 commented Nov 26, 2017

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

sujith71955 commented Nov 26, 2017

Uh oh!

srowen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Nov 26, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gatorsmile commented Nov 26, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sujith71955 commented Nov 27, 2017

Uh oh!

sujith71955 commented Nov 27, 2017

Uh oh!

sujith71955 commented Nov 27, 2017

Uh oh!

sujith71955 commented Nov 28, 2017

Uh oh!

SparkQA commented Nov 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gatorsmile commented Nov 29, 2017

Uh oh!

SparkQA commented Nov 29, 2017

Uh oh!

srowen commented Nov 29, 2017

Uh oh!

cloud-fan commented Nov 29, 2017

Uh oh!

cloud-fan commented Nov 29, 2017

Uh oh!

SparkQA commented Nov 29, 2017

Uh oh!

SparkQA commented Nov 29, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon commented Nov 30, 2017

Uh oh!

gatorsmile commented Dec 1, 2017