zifeif2 commented on code in PR #59409:
URL: https://github.com/apache/spark/pull/59409#discussion_r4266343822
##########
sql/connect/common/src/main/scala/org/apache/spark/sql/connect/client/GrpcExceptionConverter.scala:
##########
@@ -406,7 +406,170 @@ private[client] object GrpcExceptionConverter {
errorClass = Option(getErrorClassOrFallback(params)),
messageParameters = errorParamsToMessageParameters(params),
context = params.queryContext,
- sqlState = getSqlStateOrFallback(params))))
+ sqlState = getSqlStateOrFallback(params)))).map { case (name,
constructor) =>
+ name -> { (params: ErrorParams) =>
+ if (params.errorClass.exists(ec =>
!SparkThrowableHelper.isValidErrorClass(ec))) {
+ // Connector conditions and conditions from newer servers may not be
defined locally.
+ // Use the server message verbatim: parameters in ErrorInfo may have
been truncated.
+ val exception = serverErrorFactory(name)(resolveParams(params))
+ if (params.cause.isDefined && exception.getCause == null) {
+ exception.initCause(params.cause.get)
+ }
+ exception
+ } else {
+ constructor(params)
+ }
+ }
+ }
+
+ // Constructors that accept an already rendered message without consulting
the local registry.
+ private lazy val serverErrorFactory: Map[String, ErrorParams => Throwable] =
Map(
+ errorConstructor(params =>
+ new StreamingQueryException(
+ queryDebugString = "", // Already included in the server message.
Review Comment:
[P2] Preserve streaming debug text in toString / printStackTrace
Setting queryDebugString to an empty string avoids appending the debug
section twice in getMessage, but StreamingQueryException.toString uses
cause.getMessage when a cause exists and appends queryDebugString separately.
The already-rendered outer message is therefore not used by toString in that
case. Standard printStackTrace and exception logging lose the server debug
section.
Reproduced with an unknown condition, a server message ending in "server
debug info", messageParameters("queryDebugString") = "server debug info", and
cause = new IllegalStateException("server cause"):
```text
Before: toString = ...StreamingQueryException: server cause
server debug info
printStackTrace contains server debug info: true
After: toString = ...StreamingQueryException: server cause
printStackTrace contains server debug info: false
```
Please retain the debug text for toString while suppressing duplicate
formatting in getMessage, or make the server-message toString path include the
already-formatted message. Add a regression assertion using a non-null cause;
the current tests only assert getMessage.
##########
sql/api/src/main/scala/org/apache/spark/sql/catalyst/parser/parsers.scala:
##########
@@ -173,21 +173,34 @@ case object ParseErrorListener extends BaseErrorListener {
* A [[ParseException]] is an [[SparkException]] that is thrown during the
parse process. It
* contains fields and an extended error message that make reporting and
diagnosing errors easier.
*/
-class ParseException private (
+class ParseException private[sql] (
val command: Option[String],
message: String,
val start: Origin,
- errorClass: Option[String] = None,
- messageParameters: Map[String, String] = Map.empty,
- queryContext: Array[QueryContext] = ParseException.getQueryContext())
+ errorClass: Option[String],
+ messageParameters: Map[String, String],
+ queryContext: Array[QueryContext],
+ override val sqlState: Option[String],
+ cause: Option[Throwable],
+ private val messagePreformatted: Boolean)
Review Comment:
[P2] Preserve the server-message construction path in withCommand
This adds a new state where ParseException carries an unknown condition and
a preformatted message, but withCommand still constructs a new ParseException
through the constructor that calls SparkThrowableHelper.getMessage(newCl,
params). The new flag, SQLSTATE, and cause are not propagated.
I reconstructed an exception through
errorFactory(classOf[ParseException].getName) with errorClass =
Some("SERVER_ONLY_ERROR"). getMessage returns the original server message, but
invoking withCommand("SELECT 1") throws:
```text
[INTERNAL_ERROR] Cannot find main error class 'SERVER_ONLY_ERROR' SQLSTATE:
XX000
```
For a preformatted server exception, copy the message, condition,
parameters, query context, SQLSTATE, cause, and preformatted flag without
consulting the local catalog. Keep the existing PARSE_SYNTAX_ERROR /
empty-command conversion for local parse errors. Please add a regression test
for this public copying method.
##########
sql/connect/common/src/main/scala/org/apache/spark/sql/connect/client/GrpcExceptionConverter.scala:
##########
@@ -406,7 +406,170 @@ private[client] object GrpcExceptionConverter {
errorClass = Option(getErrorClassOrFallback(params)),
messageParameters = errorParamsToMessageParameters(params),
context = params.queryContext,
- sqlState = getSqlStateOrFallback(params))))
+ sqlState = getSqlStateOrFallback(params)))).map { case (name,
constructor) =>
+ name -> { (params: ErrorParams) =>
+ if (params.errorClass.exists(ec =>
!SparkThrowableHelper.isValidErrorClass(ec))) {
+ // Connector conditions and conditions from newer servers may not be
defined locally.
+ // Use the server message verbatim: parameters in ErrorInfo may have
been truncated.
+ val exception = serverErrorFactory(name)(resolveParams(params))
+ if (params.cause.isDefined && exception.getCause == null) {
+ exception.initCause(params.cause.get)
+ }
+ exception
+ } else {
+ constructor(params)
+ }
+ }
+ }
+
+ // Constructors that accept an already rendered message without consulting
the local registry.
+ private lazy val serverErrorFactory: Map[String, ErrorParams => Throwable] =
Map(
Review Comment:
Simplification: keep both constructors in one registration
This duplicates the same 18 mapped types from errorFactory, and the
unknown-condition path later assumes serverErrorFactory(name) exists. A future
mapping must be added to both maps or only its unknown-condition path fails
with a missing map key.
Could errorConstructor accept both the existing constructor and the
server-message constructor, registered together per type, and select between
them in that helper? This removes the second map, its lazy initialization, and
the name-based lookup while retaining the existing known-condition behavior.
For example:
```scala
errorConstructor[IllegalArgumentException](
localConstructor,
serverMessageConstructor)
```
I compiled this structure in a temporary copy and all 10 focused tests from
#59449 passed. It mainly simplifies maintenance rather than dramatically
reducing line count. A generic SparkException fallback would be much shorter,
but would sacrifice the mapped exception type.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
