-
Notifications
You must be signed in to change notification settings - Fork 28.1k
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.
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-38949][SQL] Wrap SQL statements by double quotes in error messages #36259
Conversation
@ivoson @yutoacts @leesf @cloud-fan @HyukjinKwon @gengliangwang @panbingkun Could you review this PR, please. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM from my side.
new ParseException( | ||
errorClass = "INVALID_SQL_SYNTAX", | ||
messageParameters = Array( | ||
s"It is not allowed to define a ${toSQLStmt("TEMPORARY")} function" + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am wondering that should the temporary function
here be the sql statement?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes sense. Let's do that.
GAs passed. Merging to master. |
…ages In the PR, I propose to wrap any SQL statement in error messages by double quotes "", and apply new implementation of `QueryErrorsBase.toSQLStmt()` to all exceptions in `Query.*Errors` w/ error classes. Also this PR modifies all affected tests, see the list in the section "How was this patch tested?". To improve user experience with Spark SQL by highlighting SQL statements in error massage and make them more visible to users. Yes. The changes might influence on error messages that are visible to users. Before: ```sql The operation DESC PARTITION is not allowed ``` After: ```sql The operation "DESC PARTITION" is not allowed ``` By running affected test suites: ``` $ build/sbt "sql/testOnly *QueryExecutionErrorsSuite" $ build/sbt "sql/testOnly *QueryParsingErrorsSuite" $ build/sbt "sql/testOnly *QueryCompilationErrorsSuite" $ build/sbt "test:testOnly *QueryCompilationErrorsDSv2Suite" $ build/sbt "test:testOnly *ExtractPythonUDFFromJoinConditionSuite" $ build/sbt "testOnly *PlanParserSuite" $ build/sbt "sql/testOnly *SQLQueryTestSuite -- -z transform.sql" $ build/sbt "sql/testOnly *SQLQueryTestSuite -- -z join-lateral.sql" $ build/sbt "sql/testOnly *SQLQueryTestSuite -- -z describe.sql" ``` Closes apache#36259 from MaxGekk/error-class-apply-toSQLStmt. Authored-by: Max Gekk <max.gekk@gmail.com> Signed-off-by: Max Gekk <max.gekk@gmail.com> (cherry picked from commit 5aba2b3) Signed-off-by: Max Gekk <max.gekk@gmail.com>
### What changes were proposed in this pull request? In the PR, I propose to describe the rules of quoting elements in error messages introduced by the PRs: - #36210 - #36233 - #36259 - #36324 - #36335 - #36359 - #36579 ### Why are the changes needed? To improve code maintenance, and the process of code review. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? By existing GAs. Closes #36621 from MaxGekk/update-error-class-guide. Authored-by: Max Gekk <max.gekk@gmail.com> Signed-off-by: Max Gekk <max.gekk@gmail.com>
### What changes were proposed in this pull request? In the PR, I propose to describe the rules of quoting elements in error messages introduced by the PRs: - #36210 - #36233 - #36259 - #36324 - #36335 - #36359 - #36579 ### Why are the changes needed? To improve code maintenance, and the process of code review. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? By existing GAs. Closes #36621 from MaxGekk/update-error-class-guide. Authored-by: Max Gekk <max.gekk@gmail.com> Signed-off-by: Max Gekk <max.gekk@gmail.com> (cherry picked from commit 2a4d8a4) Signed-off-by: Max Gekk <max.gekk@gmail.com>
What changes were proposed in this pull request?
In the PR, I propose to wrap any SQL statement in error messages by double quotes "", and apply new implementation of
QueryErrorsBase.toSQLStmt()
to all exceptions inQuery.*Errors
w/ error classes. Also this PR modifies all affected tests, see the list in the section "How was this patch tested?".Why are the changes needed?
To improve user experience with Spark SQL by highlighting SQL statements in error massage and make them more visible to users.
Does this PR introduce any user-facing change?
Yes. The changes might influence on error messages that are visible to users.
Before:
The operation DESC PARTITION is not allowed
After:
The operation "DESC PARTITION" is not allowed
How was this patch tested?
By running affected test suites: