You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "uros-db (via GitHub)" <gi...@apache.org> on 2024/08/26 15:24:54 UTC
[PR] [SPARK-49392] Catch errors when failing to write to external data source [spark]
uros-db opened a new pull request, #47873:
URL: https://github.com/apache/spark/pull/47873
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://spark.apache.org/contributing.html
2. Ensure you have added or run the appropriate tests for your PR: https://spark.apache.org/developer-tools.html
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][SPARK-XXXX] Your PR title ...'.
4. Be sure to keep the PR description updated to reflect all changes.
5. Please write your PR title to summarize what this PR proposes.
6. If possible, provide a concise example to reproduce the issue for a faster review.
7. If you want to add a new configuration, please read the guideline first for naming configurations in
'core/src/main/scala/org/apache/spark/internal/config/ConfigEntry.scala'.
8. If you want to add or modify an error type or message, please read the guideline first in
'common/utils/src/main/resources/error/README.md'.
-->
### What changes were proposed in this pull request?
Catch various exceptions thrown by the data source API, when failing to write to a data source, and rethrow `externalDataSourceException` to provide a more friendly error message for the user.
### Why are the changes needed?
To catch non-fatal exceptions when failing to save the results of a query into an external data source (for example: `com.crealytics.spark.excel`).
### Does this PR introduce _any_ user-facing change?
Yes, error messages when failing to write to an external data source should now be more user-friendly.
### How was this patch tested?
New test in `ExplainSuite`.
### Was this patch authored or co-authored using generative AI tooling?
No.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-49392][SQL] Catch errors when failing to write to external data source [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk closed pull request #47873: [SPARK-49392][SQL] Catch errors when failing to write to external data source
URL: https://github.com/apache/spark/pull/47873
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-49392] Catch errors when failing to write to external data source [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #47873:
URL: https://github.com/apache/spark/pull/47873#discussion_r1740004570
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/SaveIntoDataSourceCommand.scala:
##########
@@ -44,8 +45,23 @@ case class SaveIntoDataSourceCommand(
override def innerChildren: Seq[QueryPlan[_]] = Seq(query)
override def run(sparkSession: SparkSession): Seq[Row] = {
- val relation = dataSource.createRelation(
- sparkSession.sqlContext, mode, options, Dataset.ofRows(sparkSession, query))
+ var relation: BaseRelation = null
+
+ try {
+ relation = dataSource.createRelation(
+ sparkSession.sqlContext, mode, options, Dataset.ofRows(sparkSession, query))
+ } catch {
+ case e @ (_: NullPointerException | _: MatchError | _: ArrayIndexOutOfBoundsException) =>
Review Comment:
In the case when a datasource source throws `SparkArrayIndexOutOfBoundsException` which is a sub-class of `ArrayIndexOutOfBoundsException`, don't think we should wrap `SparkThrowable` again here. Could you fix this, please.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-49392] Catch errors when failing to write to external data source [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on code in PR #47873:
URL: https://github.com/apache/spark/pull/47873#discussion_r1754164234
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryCompilationErrors.scala:
##########
@@ -3982,6 +3982,14 @@ private[sql] object QueryCompilationErrors extends QueryErrorsBase with Compilat
messageParameters = Map("provider" -> name))
}
+ def externalDataSourceException(cause: Throwable): Throwable = {
+ new AnalysisException(
+ errorClass = "DATA_SOURCE_EXTERNAL_ERROR",
+ messageParameters = Map(),
Review Comment:
the idea is the data source fails, so we don't really have any of that (it's either null or non-existant)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-49392] Catch errors when failing to write to external data source [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #47873:
URL: https://github.com/apache/spark/pull/47873#discussion_r1758728469
##########
sql/core/src/test/scala/org/apache/spark/sql/ExplainSuite.scala:
##########
@@ -386,6 +386,24 @@ class ExplainSuite extends ExplainSuiteHelper with DisableAdaptiveExecutionSuite
Nil: _*)
}
+ test("Catch and log errors when failing to write to external data source") {
Review Comment:
Your test is not related to `explain` at all.
-1 for the changes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-49392] Catch errors when failing to write to external data source [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #47873:
URL: https://github.com/apache/spark/pull/47873#discussion_r1732463099
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/SaveIntoDataSourceCommand.scala:
##########
@@ -44,8 +45,23 @@ case class SaveIntoDataSourceCommand(
override def innerChildren: Seq[QueryPlan[_]] = Seq(query)
override def run(sparkSession: SparkSession): Seq[Row] = {
- val relation = dataSource.createRelation(
- sparkSession.sqlContext, mode, options, Dataset.ofRows(sparkSession, query))
+ var relation: BaseRelation = null
Review Comment:
I didn't get why do you perform this trick. Why cannot you just assign the result of `try {} ..` to `val BaseRelation = `
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-49392] Catch errors when failing to write to external data source [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #47873:
URL: https://github.com/apache/spark/pull/47873#discussion_r1732463099
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/SaveIntoDataSourceCommand.scala:
##########
@@ -44,8 +45,23 @@ case class SaveIntoDataSourceCommand(
override def innerChildren: Seq[QueryPlan[_]] = Seq(query)
override def run(sparkSession: SparkSession): Seq[Row] = {
- val relation = dataSource.createRelation(
- sparkSession.sqlContext, mode, options, Dataset.ofRows(sparkSession, query))
+ var relation: BaseRelation = null
Review Comment:
I didn't get why do you this trick. Why cannot you just assign the result of `try {} ..` to `val BaseRelation = `
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryCompilationErrors.scala:
##########
@@ -3982,6 +3982,14 @@ private[sql] object QueryCompilationErrors extends QueryErrorsBase with Compilat
messageParameters = Map("provider" -> name))
}
+ def externalDataSourceException(cause: Option[Throwable]): Throwable = {
+ new AnalysisException(
+ errorClass = "DATA_SOURCE_EXTERNAL_ERROR",
+ messageParameters = Map("error" -> cause.getOrElse("unknown").getClass.toString),
Review Comment:
You already added `cause` as a parameter to `AnalysisException`. Let's don't 1. duplicate error message; 2. avoid embedding a random uncontrolled text to error messages.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-49392] Catch errors when failing to write to external data source [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on code in PR #47873:
URL: https://github.com/apache/spark/pull/47873#discussion_r1758831456
##########
sql/core/src/test/scala/org/apache/spark/sql/ExplainSuite.scala:
##########
@@ -386,6 +386,24 @@ class ExplainSuite extends ExplainSuiteHelper with DisableAdaptiveExecutionSuite
Nil: _*)
}
+ test("Catch and log errors when failing to write to external data source") {
Review Comment:
alright, moving to `QueryCompilationErrorsSuite`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-49392] Catch errors when failing to write to external data source [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on code in PR #47873:
URL: https://github.com/apache/spark/pull/47873#discussion_r1753377484
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/SaveIntoDataSourceCommand.scala:
##########
@@ -44,8 +45,23 @@ case class SaveIntoDataSourceCommand(
override def innerChildren: Seq[QueryPlan[_]] = Seq(query)
override def run(sparkSession: SparkSession): Seq[Row] = {
- val relation = dataSource.createRelation(
- sparkSession.sqlContext, mode, options, Dataset.ofRows(sparkSession, query))
+ var relation: BaseRelation = null
+
+ try {
+ relation = dataSource.createRelation(
+ sparkSession.sqlContext, mode, options, Dataset.ofRows(sparkSession, query))
+ } catch {
+ case e @ (_: NullPointerException | _: MatchError | _: ArrayIndexOutOfBoundsException) =>
Review Comment:
fixed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-49392] Catch errors when failing to write to external data source [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #47873:
URL: https://github.com/apache/spark/pull/47873#discussion_r1755046575
##########
sql/core/src/test/scala/org/apache/spark/sql/ExplainSuite.scala:
##########
@@ -386,6 +387,24 @@ class ExplainSuite extends ExplainSuiteHelper with DisableAdaptiveExecutionSuite
Nil: _*)
}
+ test("Catch and log errors when failing to write to external data source") {
+ val password = "MyPassWord"
+ val token = "MyToken"
+ val value = "value"
+ val options = Map("password" -> password, "token" -> token, "key" -> value)
+ val query = spark.range(10).logicalPlan
+ val cmd = SaveIntoDataSourceCommand(query, null, options, SaveMode.Overwrite)
+
+ checkError(
+ exception = intercept[SparkThrowable] {
+ cmd.run(spark)
+ },
+ errorClass = "DATA_SOURCE_EXTERNAL_ERROR",
Review Comment:
`errorClass` doesn't exist anymore. Please, rebase on the recent master.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-49392] Catch errors when failing to write to external data source [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on code in PR #47873:
URL: https://github.com/apache/spark/pull/47873#discussion_r1758681111
##########
sql/core/src/test/scala/org/apache/spark/sql/ExplainSuite.scala:
##########
@@ -386,6 +386,24 @@ class ExplainSuite extends ExplainSuiteHelper with DisableAdaptiveExecutionSuite
Nil: _*)
}
+ test("Catch and log errors when failing to write to external data source") {
Review Comment:
there's already a similar test in this suite, so it seemed appropriate
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-49392] Catch errors when failing to write to external data source [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #47873:
URL: https://github.com/apache/spark/pull/47873#discussion_r1740001014
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryCompilationErrors.scala:
##########
@@ -3982,6 +3982,14 @@ private[sql] object QueryCompilationErrors extends QueryErrorsBase with Compilat
messageParameters = Map("provider" -> name))
}
+ def externalDataSourceException(cause: Option[Throwable]): Throwable = {
Review Comment:
Let's don't overcomplicate the signature. If the caller pass `Throwable` always, then let's adjust the signature:
```suggestion
def externalDataSourceException(cause: Throwable): Throwable = {
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-49392] Catch errors when failing to write to external data source [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #47873:
URL: https://github.com/apache/spark/pull/47873#discussion_r1754135793
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryCompilationErrors.scala:
##########
@@ -3982,6 +3982,14 @@ private[sql] object QueryCompilationErrors extends QueryErrorsBase with Compilat
messageParameters = Map("provider" -> name))
}
+ def externalDataSourceException(cause: Throwable): Throwable = {
+ new AnalysisException(
+ errorClass = "DATA_SOURCE_EXTERNAL_ERROR",
+ messageParameters = Map(),
Review Comment:
Can we provide any info about the datasource (like name, path and so on) which causes the issue. Just wonder how users should troubleshoot the issue.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-49392] Catch errors when failing to write to external data source [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on code in PR #47873:
URL: https://github.com/apache/spark/pull/47873#discussion_r1754164234
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryCompilationErrors.scala:
##########
@@ -3982,6 +3982,14 @@ private[sql] object QueryCompilationErrors extends QueryErrorsBase with Compilat
messageParameters = Map("provider" -> name))
}
+ def externalDataSourceException(cause: Throwable): Throwable = {
+ new AnalysisException(
+ errorClass = "DATA_SOURCE_EXTERNAL_ERROR",
+ messageParameters = Map(),
Review Comment:
the idea is the data source fails, so we don't really have any of that (it's either null or non-existant, so it causes another NPE when trying to do something like toString)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-49392] Catch errors when failing to write to external data source [spark]
Posted by "allisonwang-db (via GitHub)" <gi...@apache.org>.
allisonwang-db commented on code in PR #47873:
URL: https://github.com/apache/spark/pull/47873#discussion_r1731782588
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/SaveIntoDataSourceCommand.scala:
##########
@@ -44,8 +45,23 @@ case class SaveIntoDataSourceCommand(
override def innerChildren: Seq[QueryPlan[_]] = Seq(query)
override def run(sparkSession: SparkSession): Seq[Row] = {
- val relation = dataSource.createRelation(
- sparkSession.sqlContext, mode, options, Dataset.ofRows(sparkSession, query))
+ var relation: BaseRelation = null
+
+ try {
+ relation = dataSource.createRelation(
+ sparkSession.sqlContext, mode, options, Dataset.ofRows(sparkSession, query))
+ } catch {
+ case e @ (_: NullPointerException | _: MatchError | _: ArrayIndexOutOfBoundsException |
+ _: IllegalArgumentException | _: ClassCastException | _: IllegalStateException) =>
+ // These are some of the exceptions thrown by the data source API. We catch these
+ // exceptions here and rethrow QueryCompilationErrors.externalDataSourceException to
+ // provide a more friendly error message for the user. This list is not exhaustive.
+ throw QueryCompilationErrors.externalDataSourceException(Some(e))
+ case _: Throwable =>
+ // Skip other exceptions for now, as they may be handled by some other part of the code.
+ }
+
+ assert(relation != null)
Review Comment:
How can we guarantee the relation is not null here?
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/SaveIntoDataSourceCommand.scala:
##########
@@ -44,8 +45,23 @@ case class SaveIntoDataSourceCommand(
override def innerChildren: Seq[QueryPlan[_]] = Seq(query)
override def run(sparkSession: SparkSession): Seq[Row] = {
- val relation = dataSource.createRelation(
- sparkSession.sqlContext, mode, options, Dataset.ofRows(sparkSession, query))
+ var relation: BaseRelation = null
+
+ try {
+ relation = dataSource.createRelation(
+ sparkSession.sqlContext, mode, options, Dataset.ofRows(sparkSession, query))
+ } catch {
+ case e @ (_: NullPointerException | _: MatchError | _: ArrayIndexOutOfBoundsException |
+ _: IllegalArgumentException | _: ClassCastException | _: IllegalStateException) =>
+ // These are some of the exceptions thrown by the data source API. We catch these
+ // exceptions here and rethrow QueryCompilationErrors.externalDataSourceException to
+ // provide a more friendly error message for the user. This list is not exhaustive.
+ throw QueryCompilationErrors.externalDataSourceException(Some(e))
+ case _: Throwable =>
+ // Skip other exceptions for now, as they may be handled by some other part of the code.
Review Comment:
I don’t think we should ignore the other types of errors here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-49392] Catch errors when failing to write to external data source [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #47873:
URL: https://github.com/apache/spark/pull/47873#discussion_r1758594440
##########
sql/core/src/test/scala/org/apache/spark/sql/ExplainSuite.scala:
##########
@@ -386,6 +386,24 @@ class ExplainSuite extends ExplainSuiteHelper with DisableAdaptiveExecutionSuite
Nil: _*)
}
+ test("Catch and log errors when failing to write to external data source") {
Review Comment:
Why do you place this test to `ExplainSuite`? How does it rely to `explain`? If you cannot find better place, please, move it to `QueryCompilationErrorsSuite`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-49392] Catch errors when failing to write to external data source [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on PR #47873:
URL: https://github.com/apache/spark/pull/47873#issuecomment-2349458152
+1, LGTM. Merging to master.
Thank you, @uros-db and @allisonwang-db for review.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-49392] Catch errors when failing to write to external data source [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on code in PR #47873:
URL: https://github.com/apache/spark/pull/47873#discussion_r1753382349
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryCompilationErrors.scala:
##########
@@ -3982,6 +3982,14 @@ private[sql] object QueryCompilationErrors extends QueryErrorsBase with Compilat
messageParameters = Map("provider" -> name))
}
+ def externalDataSourceException(cause: Option[Throwable]): Throwable = {
Review Comment:
fixed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-49392] Catch errors when failing to write to external data source [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on code in PR #47873:
URL: https://github.com/apache/spark/pull/47873#discussion_r1732771909
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/SaveIntoDataSourceCommand.scala:
##########
@@ -44,8 +45,23 @@ case class SaveIntoDataSourceCommand(
override def innerChildren: Seq[QueryPlan[_]] = Seq(query)
override def run(sparkSession: SparkSession): Seq[Row] = {
- val relation = dataSource.createRelation(
- sparkSession.sqlContext, mode, options, Dataset.ofRows(sparkSession, query))
+ var relation: BaseRelation = null
Review Comment:
I think it's best done like this, because `LogicalRelation` will require `relation` to be a `BaseRelation`, while we don't know if try/catch will return something, throw an exception or just pass
I think we should catch only certain errors here, and not all possible exceptions
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-49392] Catch errors when failing to write to external data source [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on code in PR #47873:
URL: https://github.com/apache/spark/pull/47873#discussion_r1732764538
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryCompilationErrors.scala:
##########
@@ -3982,6 +3982,14 @@ private[sql] object QueryCompilationErrors extends QueryErrorsBase with Compilat
messageParameters = Map("provider" -> name))
}
+ def externalDataSourceException(cause: Option[Throwable]): Throwable = {
+ new AnalysisException(
+ errorClass = "DATA_SOURCE_EXTERNAL_ERROR",
+ messageParameters = Map("error" -> cause.getOrElse("unknown").getClass.toString),
Review Comment:
removed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org