You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "grundprinzip (via GitHub)" <gi...@apache.org> on 2023/11/05 23:54:09 UTC
[PR] [SPARK-XXX][CONNECT][PYTHON] Better error handling for SQL Exceptions [spark]
grundprinzip opened a new pull request, #43667:
URL: https://github.com/apache/spark/pull/43667
### What changes were proposed in this pull request?
This patch optimizes the handling of errors reported back to Python. First, it properly allows the extraction of the `ERROR_CLASS` and the `SQL_STATE` and gives simpler accces to the stack trace.
It therefore makes sure that the display of the stack trace is no longer only server-side decided but becomes a local usability property.
In addition the following methods on the `SparkConnectGrpcException` become actually useful:
* `getSqlState()`
* `getErrorClass()`
* `getStackTrace()`
### Why are the changes needed?
Compatibility
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
Updated the existing tests.
### 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-45808][CONNECT][PYTHON] Better error handling for SQL Exceptions [spark]
Posted by "grundprinzip (via GitHub)" <gi...@apache.org>.
grundprinzip commented on code in PR #43667:
URL: https://github.com/apache/spark/pull/43667#discussion_r1384766582
##########
connector/connect/server/src/main/scala/org/apache/spark/sql/connect/service/SparkConnectFetchErrorDetailsHandler.scala:
##########
@@ -46,9 +44,7 @@ class SparkConnectFetchErrorDetailsHandler(
ErrorUtils.throwableToFetchErrorDetailsResponse(
st = error,
- serverStackTraceEnabled = sessionHolder.session.conf.get(
- Connect.CONNECT_SERVER_STACKTRACE_ENABLED) || sessionHolder.session.conf.get(
Review Comment:
This is a bit more weird, in contrast to Python, the server backtrace is always there in Scala, but the user can decide how to print it.
--
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-45808][CONNECT][PYTHON] Better error handling for SQL Exceptions [spark]
Posted by "allisonwang-db (via GitHub)" <gi...@apache.org>.
allisonwang-db commented on code in PR #43667:
URL: https://github.com/apache/spark/pull/43667#discussion_r1384270902
##########
connector/connect/server/src/main/scala/org/apache/spark/sql/connect/service/SparkConnectFetchErrorDetailsHandler.scala:
##########
@@ -46,9 +44,7 @@ class SparkConnectFetchErrorDetailsHandler(
ErrorUtils.throwableToFetchErrorDetailsResponse(
st = error,
- serverStackTraceEnabled = sessionHolder.session.conf.get(
- Connect.CONNECT_SERVER_STACKTRACE_ENABLED) || sessionHolder.session.conf.get(
Review Comment:
Are we deprecating this config `Connect.CONNECT_SERVER_STACKTRACE_ENABLED`?
--
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-45808][CONNECT][PYTHON] Better error handling for SQL Exceptions [spark]
Posted by "grundprinzip (via GitHub)" <gi...@apache.org>.
grundprinzip commented on code in PR #43667:
URL: https://github.com/apache/spark/pull/43667#discussion_r1384460826
##########
connector/connect/server/src/main/scala/org/apache/spark/sql/connect/service/SparkConnectFetchErrorDetailsHandler.scala:
##########
@@ -46,9 +44,7 @@ class SparkConnectFetchErrorDetailsHandler(
ErrorUtils.throwableToFetchErrorDetailsResponse(
st = error,
- serverStackTraceEnabled = sessionHolder.session.conf.get(
- Connect.CONNECT_SERVER_STACKTRACE_ENABLED) || sessionHolder.session.conf.get(
Review Comment:
It's still used, but only verifies the display behavior rather than the stack trace generation.
--
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-45808][CONNECT][PYTHON] Better error handling for SQL Exceptions [spark]
Posted by "heyihong (via GitHub)" <gi...@apache.org>.
heyihong commented on code in PR #43667:
URL: https://github.com/apache/spark/pull/43667#discussion_r1384754927
##########
connector/connect/server/src/main/scala/org/apache/spark/sql/connect/service/SparkConnectFetchErrorDetailsHandler.scala:
##########
@@ -46,9 +44,7 @@ class SparkConnectFetchErrorDetailsHandler(
ErrorUtils.throwableToFetchErrorDetailsResponse(
st = error,
- serverStackTraceEnabled = sessionHolder.session.conf.get(
- Connect.CONNECT_SERVER_STACKTRACE_ENABLED) || sessionHolder.session.conf.get(
Review Comment:
Should we also make Connect.CONNECT_SERVER_STACKTRACE_ENABLED work for Scala client in this pr?
--
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-45808][CONNECT][PYTHON] Better error handling for SQL Exceptions [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on PR #43667:
URL: https://github.com/apache/spark/pull/43667#issuecomment-1799380556
Merged to 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-45808][CONNECT][PYTHON] Better error handling for SQL Exceptions [spark]
Posted by "itholic (via GitHub)" <gi...@apache.org>.
itholic commented on PR #43667:
URL: https://github.com/apache/spark/pull/43667#issuecomment-1800093308
Late LGTM. This is cool. Thanks!
--
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-45808][CONNECT][PYTHON] Better error handling for SQL Exceptions [spark]
Posted by "grundprinzip (via GitHub)" <gi...@apache.org>.
grundprinzip commented on code in PR #43667:
URL: https://github.com/apache/spark/pull/43667#discussion_r1383889882
##########
python/pyspark/errors/exceptions/connect.py:
##########
@@ -16,7 +16,7 @@
#
import pyspark.sql.connect.proto as pb2
import json
-from typing import Dict, List, Optional, TYPE_CHECKING
+from typing import Dict, List, Optional, TYPE_CHECKING, overload
Review Comment:
Done
--
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-45808][CONNECT][PYTHON] Better error handling for SQL Exceptions [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #43667:
URL: https://github.com/apache/spark/pull/43667#discussion_r1383863124
##########
python/pyspark/errors/exceptions/connect.py:
##########
@@ -16,7 +16,7 @@
#
import pyspark.sql.connect.proto as pb2
import json
-from typing import Dict, List, Optional, TYPE_CHECKING
+from typing import Dict, List, Optional, TYPE_CHECKING, overload
Review Comment:
```suggestion
from typing import Dict, List, Optional, TYPE_CHECKING
```
Linter complains :-).
--
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-XXX][CONNECT][PYTHON] Better error handling for SQL Exceptions [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on PR #43667:
URL: https://github.com/apache/spark/pull/43667#issuecomment-1796120836
Oh let's also file a JIRA btw
--
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-XXX][CONNECT][PYTHON] Better error handling for SQL Exceptions [spark]
Posted by "juliuszsompolski (via GitHub)" <gi...@apache.org>.
juliuszsompolski commented on code in PR #43667:
URL: https://github.com/apache/spark/pull/43667#discussion_r1383287339
##########
python/pyspark/errors/exceptions/connect.py:
##########
@@ -135,16 +233,48 @@ def __init__(
error_class: Optional[str] = None,
message_parameters: Optional[Dict[str, str]] = None,
reason: Optional[str] = None,
+ sql_state: Optional[str] = None,
+ stacktrace: Optional[str] = None,
Review Comment:
nit: server_stacktrace maybe?
##########
python/pyspark/errors/exceptions/connect.py:
##########
@@ -46,55 +46,153 @@ class SparkConnectException(PySparkException):
def convert_exception(
- info: "ErrorInfo", truncated_message: str, resp: Optional[pb2.FetchErrorDetailsResponse]
+ info: "ErrorInfo",
+ truncated_message: str,
+ resp: Optional[pb2.FetchErrorDetailsResponse],
+ display_stacktrace: bool = False,
Review Comment:
nit: maybe name it `display_server_stacktrace` to be better seen at a glance which stack trace it is.
--
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-45808][CONNECT][PYTHON] Better error handling for SQL Exceptions [spark]
Posted by "grundprinzip (via GitHub)" <gi...@apache.org>.
grundprinzip commented on PR #43667:
URL: https://github.com/apache/spark/pull/43667#issuecomment-1796129182
Filed SPARK-45808
--
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-45808][CONNECT][PYTHON] Better error handling for SQL Exceptions [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon closed pull request #43667: [SPARK-45808][CONNECT][PYTHON] Better error handling for SQL Exceptions
URL: https://github.com/apache/spark/pull/43667
--
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