You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "stevomitric (via GitHub)" <gi...@apache.org> on 2024/03/08 13:45:03 UTC
[PR] [SPARK-47313] Added scala.MatchError handling inside QueryExecution.toInternalError [spark]
stevomitric opened a new pull request, #45438:
URL: https://github.com/apache/spark/pull/45438
### What changes were proposed in this pull request?
Added handling of scala.MatchError inside QueryExecution.toInternalError alongside existing java.lang.NullPointerException and java.lang.AssertionError
### Why are the changes needed?
Additional error coverage.
### Does this PR introduce _any_ user-facing change?
No
### 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-47313][SQL] Added scala.MatchError handling inside QueryExecution.toInternalError [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on PR #45438:
URL: https://github.com/apache/spark/pull/45438#issuecomment-1988571851
@stevomitric Congratulations with your first contribution to Apache Spark!
--
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-47313] Added scala.MatchError handling inside QueryExecution.toInternalError [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #45438:
URL: https://github.com/apache/spark/pull/45438#discussion_r1519136321
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/QueryExecution.scala:
##########
@@ -540,10 +540,11 @@ object QueryExecution {
}
/**
- * Converts asserts, null pointer exceptions to internal errors.
+ * Converts asserts, null pointer exceptions and scala match errors to internal errors.
*/
private[sql] def toInternalError(msg: String, e: Throwable): Throwable = e match {
- case e @ (_: java.lang.NullPointerException | _: java.lang.AssertionError) =>
+ case e @ (_: java.lang.NullPointerException | _: java.lang.AssertionError |
+ _: scala.MatchError) =>
Review Comment:
we can replace this case match by moving the logic to a new method to make the code more reable
```
private def isInternalError(e: Throwable): Boolean = e match {
case _: java.lang.NullPointerException => true
case _: _: java.lang.AssertionError => true
...
case _ => false
}
```
then the code here
```
if (isInternalError(e)) {
SparkException.internalError...
} else {
e
}
```
--
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-47313] Added scala.MatchError handling inside QueryExecution.toInternalError [spark]
Posted by "stevomitric (via GitHub)" <gi...@apache.org>.
stevomitric commented on code in PR #45438:
URL: https://github.com/apache/spark/pull/45438#discussion_r1517959688
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/QueryExecution.scala:
##########
@@ -540,20 +540,26 @@ object QueryExecution {
}
/**
- * Converts asserts, null pointer exceptions to internal errors.
+ * Converts asserts, null pointer exceptions and scala match errors to internal errors.
*/
private[sql] def toInternalError(msg: String, e: Throwable): Throwable = e match {
case e @ (_: java.lang.NullPointerException | _: java.lang.AssertionError) =>
Review Comment:
Merged `scala.MatchError` with other two 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-47313] Added scala.MatchError handling inside QueryExecution.toInternalError [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #45438:
URL: https://github.com/apache/spark/pull/45438#discussion_r1517781945
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/QueryExecution.scala:
##########
@@ -540,20 +540,26 @@ object QueryExecution {
}
/**
- * Converts asserts, null pointer exceptions to internal errors.
+ * Converts asserts, null pointer exceptions and scala match errors to internal errors.
*/
private[sql] def toInternalError(msg: String, e: Throwable): Throwable = e match {
case e @ (_: java.lang.NullPointerException | _: java.lang.AssertionError) =>
SparkException.internalError(
msg + " You hit a bug in Spark or the Spark plugins you use. Please, report this bug " +
"to the corresponding communities or vendors, and provide the full stack trace.",
e)
+ case e @ (_: scala.MatchError) =>
Review Comment:
let's avoid duplicated code, we can add a new function `def isInternalError`, then rewrite here
```
if (isInternalError(e)) {
internal error ...
} else {
e
}
```
--
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-47313] Added scala.MatchError handling inside QueryExecution.toInternalError [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #45438:
URL: https://github.com/apache/spark/pull/45438#discussion_r1517860532
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/QueryExecution.scala:
##########
@@ -540,20 +540,26 @@ object QueryExecution {
}
/**
- * Converts asserts, null pointer exceptions to internal errors.
+ * Converts asserts, null pointer exceptions and scala match errors to internal errors.
*/
private[sql] def toInternalError(msg: String, e: Throwable): Throwable = e match {
case e @ (_: java.lang.NullPointerException | _: java.lang.AssertionError) =>
Review Comment:
Why don't you add just one more case of `scala.MatchError` here?
The devs who will investigate the issue will see the cause exception `scala.MatchError`.
--
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-47313][SQL] Added scala.MatchError handling inside QueryExecution.toInternalError [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on PR #45438:
URL: https://github.com/apache/spark/pull/45438#issuecomment-1988551358
+1, LGTM. Merging to master.
Thank you, @stevomitric and @cloud-fan 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-47313][SQL] Added scala.MatchError handling inside QueryExecution.toInternalError [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk closed pull request #45438: [SPARK-47313][SQL] Added scala.MatchError handling inside QueryExecution.toInternalError
URL: https://github.com/apache/spark/pull/45438
--
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