[SPARK-30150][SQL] ADD FILE, ADD JAR, LIST FILE & LIST JAR Command do not accept quoted path #26779

iRakson · 2019-12-06T12:53:09Z

What changes were proposed in this pull request?

add file "abc.txt" and add file 'abc.txt' are not supported.
For these two spark sql gives FileNotFoundException.
Only add file abc.txt is supported currently.

After these changes path can be given as quoted text for ADD FILE, ADD JAR, LIST FILE, LIST JAR commands in spark-sql

Why are the changes needed?

In many of the spark-sql commands (like create table ,etc )we write path in quoted format only. To maintain this consistency we should support quoted format with this command as well.

Does this PR introduce any user-facing change?

Yes. Now users can write path with quotes.

How was this patch tested?

Manually tested.

iRakson · 2019-12-06T13:49:14Z

cc @cloud-fan

HyukjinKwon · 2019-12-07T01:55:24Z

sql/core/src/main/scala/org/apache/spark/sql/execution/SparkSqlParser.scala

@@ -357,7 +357,7 @@ class SparkSqlAstBuilder(conf: SQLConf) extends AstBuilder(conf) {
   * }}}
   */
  override def visitManageResource(ctx: ManageResourceContext): LogicalPlan = withOrigin(ctx) {
-    val mayebePaths = remainder(ctx.identifier).trim
+    val mayebePaths = pathWrapper(remainder(ctx.identifier).trim)


Can we use string literal or identifier rule in ANTLER rather than manually fixing here from identifer? BTW, does Hive support that case as well?

Yea, that approach @HyukjinKwon addressed above looks nicer to me.

I changed the grammar to take string literal as well for ADD/LIST.

Now all of add file abc.txt, add file 'abc.txt' and add file "abc.txt" are supported.

maropu · 2019-12-09T08:03:14Z

Can you add tests for that?

iRakson · 2019-12-09T08:09:33Z

Can you add tests for that?

Yeah Sure.

iRakson · 2019-12-09T09:30:03Z

Test Cases Added For all the 4 commands.
Please review this @maropu , @HyukjinKwon

sql/core/src/main/scala/org/apache/spark/sql/execution/SparkSqlParser.scala

maropu · 2019-12-10T01:08:00Z

Can you update the title to make it clearer? Is this pr not only for AddFile, right?

sql/core/src/test/scala/org/apache/spark/sql/execution/SparkSqlParserSuite.scala

maropu · 2019-12-10T01:12:19Z

ok to test

HyukjinKwon · 2019-12-10T02:01:05Z

@iRakson does Hive support this case?

iRakson · 2019-12-10T03:20:16Z

@HyukjinKwon No. Path passed in hive is unquoted i.e not as string literal.

sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4

SparkQA · 2019-12-10T05:24:22Z

Test build #115061 has finished for PR 26779 at commit fbf1add.

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

SparkQA · 2019-12-10T08:05:01Z

Test build #115084 has finished for PR 26779 at commit 7aafe85.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

iRakson · 2019-12-10T08:22:27Z

please re-test this one.

maropu · 2019-12-10T10:19:43Z

retest this please

SparkQA · 2019-12-10T14:29:25Z

Test build #115100 has finished for PR 26779 at commit 7aafe85.

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

cloud-fan · 2019-12-10T18:08:00Z

sql/core/src/test/scala/org/apache/spark/sql/execution/SparkSqlParserSuite.scala

+
+  test("manage resources") {
+    assertEqual("ADD FILE abc.txt", AddFileCommand("abc.txt"))
+    assertEqual("ADD FILE \'abc.txt\'", AddFileCommand("abc.txt"))


is \' the same as ' here?

shall we use '? it's easier to read

@cloud-fan Yes, Its better that way. I have made the changes.

dongjoon-hyun · 2019-12-11T04:52:16Z

sql/core/src/test/scala/org/apache/spark/sql/execution/SparkSqlParserSuite.scala

+    assertEqual("ADD FILE abc.txt", AddFileCommand("abc.txt"))
+    assertEqual("ADD FILE \'abc.txt\'", AddFileCommand("abc.txt"))
+    assertEqual("ADD FILE \"/path/to/abc.txt\"", AddFileCommand("/path/to/abc.txt"))
+    assertEqual("LIST FILE abc.txt", ListFilesCommand("abc.txt".split("\\s+")))


Shall we use simply Array("/path//abc.txt") instead of "abc.txt".split("\\s+")? (here and the below LIST test cases)

List commands are using .split("\\s+") on path given to them. Thus we have to do same here. @dongjoon-hyun

No. This is a test case. You can use the result of .split("\\s+").

Ok. I will use the result of .split("\s+").

dongjoon-hyun · 2019-12-11T04:55:54Z

sql/core/src/test/scala/org/apache/spark/sql/execution/SparkSqlParserSuite.scala

+    assertEqual("LIST JAR \'abc.jar\'", ListJarsCommand("abc.jar".split("\\s+")))
+    assertEqual("LIST JAR \"abc.jar\"", ListJarsCommand("abc.jar".split("\\s+")))
+    assertEqual("ADD FILE /path with space/abc.txt", AddFileCommand("/path with space/abc.txt"))
+    assertEqual("ADD JAR /path with space/abc.jar", AddJarCommand("/path with space/abc.jar"))


Although our parser will accept without any differences, shall we have a test case for Window-path, too?

@dongjoon-hyun Is it required to add test cases for window-path?

I think it's OK. We know that the parser accepts anything, we just need to prove that quoted path works.

SparkQA · 2019-12-11T13:09:12Z

Test build #115163 has finished for PR 26779 at commit b808d51.

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

cloud-fan · 2019-12-12T09:11:39Z

thanks, merging to master!

iRakson · 2019-12-12T09:22:33Z

Thanks all for reviewing this PR :) and merging it.

iRakson added 2 commits December 6, 2019 18:08

[SPARK-30150]AddFile Command do not accept quoted path

ab10933

[SPARK-30150]Fix

8949f70

dongjoon-hyun added the SQL label Dec 6, 2019

HyukjinKwon reviewed Dec 7, 2019

View reviewed changes

[SPARK-30150]Fix Review Comments

4141857

iRakson requested review from HyukjinKwon and maropu December 9, 2019 06:43

[SPARK-30150]Added Unit Tests

fbf1add

maropu reviewed Dec 10, 2019

View reviewed changes

sql/core/src/main/scala/org/apache/spark/sql/execution/SparkSqlParser.scala Show resolved Hide resolved

maropu reviewed Dec 10, 2019

View reviewed changes

sql/core/src/test/scala/org/apache/spark/sql/execution/SparkSqlParserSuite.scala Outdated Show resolved Hide resolved

iRakson changed the title ~~[SPARK-30150][SQL]AddFile Command do not accept quoted path~~ [SPARK-30150][SQL]ADD FILE, ADD JAR, LIST FILE & LIST JAR Command do not accept quoted path Dec 10, 2019

cloud-fan reviewed Dec 10, 2019

View reviewed changes

sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4 Show resolved Hide resolved

[SPARK-30150]Fix Review Comments

7aafe85

iRakson requested review from cloud-fan and maropu December 10, 2019 05:56

cloud-fan reviewed Dec 10, 2019

View reviewed changes

iRakson requested a review from cloud-fan December 11, 2019 02:13

dongjoon-hyun changed the title ~~[SPARK-30150][SQL]ADD FILE, ADD JAR, LIST FILE & LIST JAR Command do not accept quoted path~~ [SPARK-30150][SQL] ADD FILE, ADD JAR, LIST FILE & LIST JAR Command do not accept quoted path Dec 11, 2019

dongjoon-hyun reviewed Dec 11, 2019

View reviewed changes

[SPARK-30150]Fix Review Comments

b808d51

iRakson requested a review from dongjoon-hyun December 11, 2019 08:22

cloud-fan closed this in 2936507 Dec 12, 2019

maropu mentioned this pull request Dec 24, 2019

[SPARK-30342][SQL][DOC]Update LIST FILE/JAR command Documentation #26996

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-30150][SQL] ADD FILE, ADD JAR, LIST FILE & LIST JAR Command do not accept quoted path #26779

[SPARK-30150][SQL] ADD FILE, ADD JAR, LIST FILE & LIST JAR Command do not accept quoted path #26779

iRakson commented Dec 6, 2019

iRakson commented Dec 6, 2019

HyukjinKwon Dec 7, 2019

maropu Dec 9, 2019

iRakson Dec 9, 2019

maropu commented Dec 9, 2019

iRakson commented Dec 9, 2019

iRakson commented Dec 9, 2019 •

edited

maropu commented Dec 10, 2019

maropu commented Dec 10, 2019

HyukjinKwon commented Dec 10, 2019

iRakson commented Dec 10, 2019

SparkQA commented Dec 10, 2019

SparkQA commented Dec 10, 2019

iRakson commented Dec 10, 2019

maropu commented Dec 10, 2019

SparkQA commented Dec 10, 2019

cloud-fan Dec 10, 2019

iRakson Dec 10, 2019

cloud-fan Dec 11, 2019

iRakson Dec 11, 2019

dongjoon-hyun Dec 11, 2019

iRakson Dec 11, 2019 •

edited

dongjoon-hyun Dec 11, 2019

iRakson Dec 11, 2019

dongjoon-hyun Dec 11, 2019

iRakson Dec 12, 2019 •

edited

cloud-fan Dec 12, 2019

SparkQA commented Dec 11, 2019

cloud-fan commented Dec 12, 2019

iRakson commented Dec 12, 2019

[SPARK-30150][SQL] ADD FILE, ADD JAR, LIST FILE & LIST JAR Command do not accept quoted path #26779

[SPARK-30150][SQL] ADD FILE, ADD JAR, LIST FILE & LIST JAR Command do not accept quoted path #26779

Conversation

iRakson commented Dec 6, 2019

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

iRakson commented Dec 6, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maropu commented Dec 9, 2019

iRakson commented Dec 9, 2019

iRakson commented Dec 9, 2019 • edited

maropu commented Dec 10, 2019

maropu commented Dec 10, 2019

HyukjinKwon commented Dec 10, 2019

iRakson commented Dec 10, 2019

SparkQA commented Dec 10, 2019

SparkQA commented Dec 10, 2019

iRakson commented Dec 10, 2019

maropu commented Dec 10, 2019

SparkQA commented Dec 10, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iRakson Dec 11, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iRakson Dec 12, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Dec 11, 2019

cloud-fan commented Dec 12, 2019

iRakson commented Dec 12, 2019

iRakson commented Dec 9, 2019 •

edited

iRakson Dec 11, 2019 •

edited

iRakson Dec 12, 2019 •

edited