[SPARK-5821] [SQL] JSON CTAS command should throw error message when delete path failure #4610

yanboliang · 2015-02-15T05:55:59Z

When using "CREATE TEMPORARY TABLE AS SELECT" to create JSON table, we first delete the path file or directory and then generate a new directory with the same name. But if only read permission was granted, the delete failed.
Here we just throwing an error message to let users know what happened.
ParquetRelation2 may also hit this problem. I think to restrict JSONRelation and ParquetRelation2 must base on directory is more reasonable for access control. Maybe I can do it in follow up works.

SparkQA · 2015-02-15T05:57:33Z

Test build #27506 has started for PR 4610 at commit 307278f.

This patch merges cleanly.

SparkQA · 2015-02-15T05:58:31Z

Test build #27506 has finished for PR 4610 at commit 307278f.

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

AmplabJenkins · 2015-02-15T05:58:32Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/27506/
Test FAILed.

SparkQA · 2015-02-15T06:02:33Z

Test build #27507 has started for PR 4610 at commit 3156c1b.

This patch merges cleanly.

SparkQA · 2015-02-15T06:13:55Z

Test build #27507 has finished for PR 4610 at commit 3156c1b.

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

AmplabJenkins · 2015-02-15T06:13:57Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/27507/
Test FAILed.

yhuai · 2015-02-15T06:26:52Z

sql/core/src/main/scala/org/apache/spark/sql/json/JSONRelation.scala

      // Only save data when the save mode is not ignore.
-      data.toJSON.saveAsTextFile(path)


Why not just save the data?

yhuai · 2015-02-15T06:33:46Z

@yanbohappy It will be great if we use this PR to only do the task of throwing a nice error message when the delete returns false and data already exists in the code path for overwrite.

SparkQA · 2015-02-15T06:37:24Z

Test build #27509 has started for PR 4610 at commit 3cdfb7a.

This patch merges cleanly.

SparkQA · 2015-02-15T07:16:13Z

Test build #27509 has finished for PR 4610 at commit 3cdfb7a.

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

AmplabJenkins · 2015-02-15T07:16:16Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/27509/
Test FAILed.

yanboliang · 2015-02-15T16:52:50Z

@yhuai
OK, if we don't support write to a table while reading it, the code will be more concise.
I will try to rewrite the code to focus on the issue SPARK-5812.
Thank you for your comments.

SparkQA · 2015-02-16T10:02:34Z

Test build #27551 has started for PR 4610 at commit 5a42d83.

This patch merges cleanly.

SparkQA · 2015-02-16T11:27:59Z

Test build #27551 has finished for PR 4610 at commit 5a42d83.

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

AmplabJenkins · 2015-02-16T11:28:03Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/27551/
Test PASSed.

yhuai · 2015-02-18T17:47:49Z

sql/core/src/main/scala/org/apache/spark/sql/json/JSONRelation.scala

+              throw new IOException(
+                s"Unable to clear output directory ${filesystemPath.toString} prior"
+                  + s" to CREATE a JSON table AS SELECT:\n${e.toString}")
+          }


@yanbohappy Seems we just throw another error message at here. Based on your JIRA description, I think you need to check if delete returns true or false when data already exists.

@yhuai Yes, it is expected. If there is no exception thrown, it can ensure delete success. I have investigated other codes in spark and they do the same thing for delete.

marmbrus · 2015-03-13T23:05:06Z

@yhuai what is the status here?

yhuai · 2015-03-14T01:41:22Z

Sorry for not following it. I will investigate it and reproduce the error reported in jira. I should have more ideas on how we want to have the change early next week.

SparkQA · 2015-03-16T10:18:03Z

Test build #28643 has started for PR 4610 at commit e4bc229.

This patch merges cleanly.

SparkQA · 2015-03-18T08:27:29Z

Test build #28784 has finished for PR 4610 at commit 46f0d9d.

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

AmplabJenkins · 2015-03-18T08:27:33Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/28784/
Test PASSed.

yhuai · 2015-03-19T02:37:39Z

sql/core/src/main/scala/org/apache/spark/sql/json/JSONRelation.scala

+            if (!fs.delete(filesystemPath, true)) {
+              throw new IOException(
+                s"Unable to clear output directory ${filesystemPath.toString} prior"
+                  + s" to INSERT OVERWRITE a JSON table:\n")


Can we say "prior to writing to JSON file" since a user can reach this code path through DataFrame.save, which is not related to table operation.

Also, get rid of :\n and add a . at the end of the message.

yhuai · 2015-03-19T02:45:20Z

Thank you for the update! It's very close. I think we want to do the same thing for ParquetRelation2. Is it possible to add a test for this?

cc @liancheng

SparkQA · 2015-03-19T07:48:17Z

Test build #28860 has started for PR 4610 at commit c387fce.

This patch merges cleanly.

SparkQA · 2015-03-19T09:15:04Z

Test build #28860 has finished for PR 4610 at commit c387fce.

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

AmplabJenkins · 2015-03-19T09:15:07Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/28860/
Test PASSed.

liancheng · 2015-03-19T12:15:33Z

@yhuai Thanks for cc-ing me this. Although ParquetRelation2 checks for exception, it doesn't check return value of fs.delete, and thus suffer the same issue. Opening a separate PR for this.

@yanboliang Thanks for pointing out the bug and working on this!

yanboliang · 2015-03-19T12:55:01Z

@yhuai Thank you for your comments on this PR
@liancheng If this LGTM, can I open another PR for ParquetRelation2 and work on it?

liancheng · 2015-03-19T13:00:49Z

@yanboliang Sure, please go ahead. I'm trying to verify this issue locally.

liancheng · 2015-03-20T08:17:20Z

@yanboliang Adding a check for return value at this line should be enough.

yanboliang · 2015-03-20T16:44:28Z

@liancheng Please review my fix for ParquetRelation2 in #5107

…ccessful Do the same check as #4610 for ParquetRelation2. Author: Yanbo Liang <[email protected]> Closes #5107 from yanboliang/spark-5821-parquet and squashes the following commits: 7092c8d [Yanbo Liang] ParquetRelation2 CTAS should check if delete is successful

…ccessful Do the same check as #4610 for ParquetRelation2. Author: Yanbo Liang <[email protected]> Closes #5107 from yanboliang/spark-5821-parquet and squashes the following commits: 7092c8d [Yanbo Liang] ParquetRelation2 CTAS should check if delete is successful (cherry picked from commit bc37c97) Signed-off-by: Cheng Lian <[email protected]>

…delete path failure When using "CREATE TEMPORARY TABLE AS SELECT" to create JSON table, we first delete the path file or directory and then generate a new directory with the same name. But if only read permission was granted, the delete failed. Here we just throwing an error message to let users know what happened. ParquetRelation2 may also hit this problem. I think to restrict JSONRelation and ParquetRelation2 must base on directory is more reasonable for access control. Maybe I can do it in follow up works. Author: Yanbo Liang <[email protected]> Author: Yanbo Liang <[email protected]> Closes #4610 from yanboliang/jsonInsertImprovements and squashes the following commits: c387fce [Yanbo Liang] fix typos 42d7fb6 [Yanbo Liang] add unittest & fix output format 46f0d9d [Yanbo Liang] Update JSONRelation.scala e2df8d5 [Yanbo Liang] check path exisit when write 79f7040 [Yanbo Liang] Update JSONRelation.scala e4bc229 [Yanbo Liang] Update JSONRelation.scala 5a42d83 [Yanbo Liang] JSONRelation CTAS should check if delete is successful (cherry picked from commit e5d2c37) Signed-off-by: Cheng Lian <[email protected]>

liancheng · 2015-03-21T03:24:30Z

Thanks, merged into master and branch-1.3.

liancheng · 2015-03-21T03:24:53Z

PS: #5107 has also been merged.

### What changes were proposed in this pull request? This PR amis to upgrade `fasterxml.jackson` from 2.17.1 to 2.17.2. ### Why are the changes needed? There are some bug fixes about [Databind](https://github.com/FasterXML/jackson-databind): [#4561](FasterXML/jackson-databind#4561): Issues using jackson-databind 2.17.1 with Reactor (wrt DeserializerCache and ReentrantLock) [#4575](FasterXML/jackson-databind#4575): StdDelegatingSerializer does not consider a Converter that may return null for a non-null input [#4577](FasterXML/jackson-databind#4577): Cannot deserialize value of type java.math.BigDecimal from String "3." (not a valid representation) [#4595](FasterXML/jackson-databind#4595): No way to explicitly disable wrapping in custom annotation processor [#4607](FasterXML/jackson-databind#4607): MismatchedInput: No Object Id found for an instance of X to assign to property 'id' [#4610](FasterXML/jackson-databind#4610): DeserializationFeature.FAIL_ON_UNRESOLVED_OBJECT_IDS does not work when used with Polymorphic type handling The full release note of 2.17.2: https://github.com/FasterXML/jackson/wiki/Jackson-Release-2.17.2 ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Pass GA. ### Was this patch authored or co-authored using generative AI tooling? No. Closes #47241 from wayneguow/upgrade_jackson. Authored-by: Wei Guo <[email protected]> Signed-off-by: yangjie01 <[email protected]>

### What changes were proposed in this pull request? This PR amis to upgrade `fasterxml.jackson` from 2.17.1 to 2.17.2. ### Why are the changes needed? There are some bug fixes about [Databind](https://github.com/FasterXML/jackson-databind): [apache#4561](FasterXML/jackson-databind#4561): Issues using jackson-databind 2.17.1 with Reactor (wrt DeserializerCache and ReentrantLock) [apache#4575](FasterXML/jackson-databind#4575): StdDelegatingSerializer does not consider a Converter that may return null for a non-null input [apache#4577](FasterXML/jackson-databind#4577): Cannot deserialize value of type java.math.BigDecimal from String "3." (not a valid representation) [apache#4595](FasterXML/jackson-databind#4595): No way to explicitly disable wrapping in custom annotation processor [apache#4607](FasterXML/jackson-databind#4607): MismatchedInput: No Object Id found for an instance of X to assign to property 'id' [apache#4610](FasterXML/jackson-databind#4610): DeserializationFeature.FAIL_ON_UNRESOLVED_OBJECT_IDS does not work when used with Polymorphic type handling The full release note of 2.17.2: https://github.com/FasterXML/jackson/wiki/Jackson-Release-2.17.2 ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Pass GA. ### Was this patch authored or co-authored using generative AI tooling? No. Closes apache#47241 from wayneguow/upgrade_jackson. Authored-by: Wei Guo <[email protected]> Signed-off-by: yangjie01 <[email protected]>

yanboliang changed the title ~~JSON external data source INSERT improvements initial draft~~ [SPARK-5821] [SPARK-5746] [SQL] JSON external data source INSERT improvements initial draft Feb 15, 2015

yanboliang changed the title ~~[SPARK-5821] [SPARK-5746] [SQL] JSON external data source INSERT improvements initial draft~~ [SPARK-5821] [SQL] JSON external data source INSERT improvements initial draft Feb 15, 2015

yanboliang mentioned this pull request Feb 15, 2015

[deprecated] [SPARK-5821] [SPARK-5746] [SQL] JSON external data source INSERT operation improvements #4607

Closed

yhuai reviewed Feb 15, 2015
View reviewed changes

yanboliang changed the title ~~[SPARK-5821] [SQL] JSON external data source INSERT improvements initial draft~~ [SPARK-5821] [SQL] JSON external data source insert error due to with permission Feb 15, 2015

yanboliang changed the title ~~[SPARK-5821] [SQL] JSON external data source insert error due to with permission~~ [SPARK-5821] [SQL] JSON external data source insert error due to without permission Feb 15, 2015

JSONRelation CTAS should check if delete is successful

5a42d83

yanboliang force-pushed the jsonInsertImprovements branch from 3cdfb7a to 5a42d83 Compare February 16, 2015 10:00

yanboliang changed the title ~~[SPARK-5821] [SQL] JSON external data source insert error due to without permission~~ [SPARK-5821] [SQL] JSON CTAS command should throw error message when delete path failure Feb 16, 2015

yhuai reviewed Feb 18, 2015
View reviewed changes

Update JSONRelation.scala

e4bc229

yhuai reviewed Mar 19, 2015
View reviewed changes

yanboliang added 2 commits March 19, 2015 15:37

add unittest & fix output format

42d7fb6

fix typos

c387fce

yanboliang mentioned this pull request Mar 20, 2015

[SPARK-5821] [SQL] ParquetRelation2 CTAS should check if delete is successful #5107

Closed

asfgit closed this in e5d2c37 Mar 21, 2015

yanboliang deleted the jsonInsertImprovements branch April 24, 2015 10:02

		// Only save data when the save mode is not ignore.
		data.toJSON.saveAsTextFile(path)

[SPARK-5821] [SQL] JSON CTAS command should throw error message when delete path failure #4610

[SPARK-5821] [SQL] JSON CTAS command should throw error message when delete path failure #4610

Conversation

yanboliang commented Feb 15, 2015

SparkQA commented Feb 15, 2015

SparkQA commented Feb 15, 2015

AmplabJenkins commented Feb 15, 2015

SparkQA commented Feb 15, 2015

SparkQA commented Feb 15, 2015

AmplabJenkins commented Feb 15, 2015

yhuai Feb 15, 2015

Choose a reason for hiding this comment

yhuai commented Feb 15, 2015

SparkQA commented Feb 15, 2015

SparkQA commented Feb 15, 2015

AmplabJenkins commented Feb 15, 2015

yanboliang commented Feb 15, 2015

SparkQA commented Feb 16, 2015

SparkQA commented Feb 16, 2015

AmplabJenkins commented Feb 16, 2015

yhuai Feb 18, 2015

Choose a reason for hiding this comment

yanboliang Feb 19, 2015

Choose a reason for hiding this comment

marmbrus commented Mar 13, 2015

yhuai commented Mar 14, 2015

SparkQA commented Mar 16, 2015

SparkQA commented Mar 18, 2015

AmplabJenkins commented Mar 18, 2015

yhuai Mar 19, 2015

Choose a reason for hiding this comment

yhuai Mar 19, 2015

Choose a reason for hiding this comment

yhuai commented Mar 19, 2015

SparkQA commented Mar 19, 2015

SparkQA commented Mar 19, 2015

AmplabJenkins commented Mar 19, 2015

liancheng commented Mar 19, 2015

yanboliang commented Mar 19, 2015

liancheng commented Mar 19, 2015

liancheng commented Mar 20, 2015

yanboliang commented Mar 20, 2015

liancheng commented Mar 21, 2015

liancheng commented Mar 21, 2015