Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 7 commits into from
Closed

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

Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
5a42d83
JSONRelation CTAS should check if delete is successful
Feb 16, 2015
e4bc229
Update JSONRelation.scala
yanboliang Mar 16, 2015
79f7040
Update JSONRelation.scala
yanboliang Mar 16, 2015
e2df8d5
check path exisit when write
yanboliang Mar 16, 2015
46f0d9d
Update JSONRelation.scala
yanboliang Mar 18, 2015
42d7fb6
add unittest & fix output format
yanboliang Mar 19, 2015
c387fce
fix typos
yanboliang Mar 19, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 19 additions & 3 deletions sql/core/src/main/scala/org/apache/spark/sql/json/JSONRelation.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,9 +66,21 @@ private[sql] class DefaultSource
mode match {
case SaveMode.Append =>
sys.error(s"Append mode is not supported by ${this.getClass.getCanonicalName}")
case SaveMode.Overwrite =>
fs.delete(filesystemPath, true)
case SaveMode.Overwrite => {
try {
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")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
} catch {
case e: IOException =>
throw new IOException(
s"Unable to clear output directory ${filesystemPath.toString} prior"
+ s" to INSERT OVERWRITE a JSON table:\n${e.toString}")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Save here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will also catch the one thrown by try. Can we avoid this? If delete returns false, we just throw the exception after the catch. Also, you can put e as the cause of the new exception, like IOException("...", e).

}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

true
}
case SaveMode.ErrorIfExists =>
sys.error(s"path $path already exists.")
case SaveMode.Ignore => false
Expand Down Expand Up @@ -110,7 +122,11 @@ private[sql] case class JSONRelation(

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Get rid of :\n.

}
} catch {
case e: IOException =>
throw new IOException(
Expand Down