You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "panbingkun (via GitHub)" <gi...@apache.org> on 2023/05/18 08:47:05 UTC
[GitHub] [spark] panbingkun opened a new pull request, #41214: [SPARK-43549][SQL] Assign a name to the error class _LEGACY_ERROR_TEMP_0036
panbingkun opened a new pull request, #41214:
URL: https://github.com/apache/spark/pull/41214
### What changes were proposed in this pull request?
The pr aims to assign a name to the error class _LEGACY_ERROR_TEMP_0036.
### Why are the changes needed?
The changes improve the error framework.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
Pass GA & Update UT.
--
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
[GitHub] [spark] panbingkun commented on a diff in pull request #41214: [SPARK-43549][SQL] Convert `_LEGACY_ERROR_TEMP_0036` to INVALID_SQL_SYNTAX
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on code in PR #41214:
URL: https://github.com/apache/spark/pull/41214#discussion_r1199746634
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryParsingErrors.scala:
##########
@@ -407,8 +407,8 @@ private[sql] object QueryParsingErrors extends QueryErrorsBase {
def computeStatisticsNotExpectedError(ctx: IdentifierContext): Throwable = {
new ParseException(
- errorClass = "_LEGACY_ERROR_TEMP_0036",
- messageParameters = Map("ctx" -> ctx.getText),
+ errorClass = "INVALID_SQL_SYNTAX",
+ messageParameters = Map("inputString" -> s"${ctx.getText} must be ${toSQLStmt("NOSCAN")}"),
Review Comment:
👌🏻
--
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
[GitHub] [spark] panbingkun commented on a diff in pull request #41214: [SPARK-43549][SQL] Convert `_LEGACY_ERROR_TEMP_0036` to INVALID_SQL_SYNTAX
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on code in PR #41214:
URL: https://github.com/apache/spark/pull/41214#discussion_r1198690104
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryParsingErrors.scala:
##########
@@ -407,8 +407,8 @@ private[sql] object QueryParsingErrors extends QueryErrorsBase {
def computeStatisticsNotExpectedError(ctx: IdentifierContext): Throwable = {
new ParseException(
- errorClass = "_LEGACY_ERROR_TEMP_0036",
- messageParameters = Map("ctx" -> ctx.getText),
+ errorClass = "INVALID_SQL_SYNTAX",
+ messageParameters = Map("inputString" -> s"${ctx.getText} must be ${toSQLStmt("NOSCAN")}"),
Review Comment:
I have found many similar uses in `QueryParsingErrors`. Should we refactor it to classify `INVALID_SQL_SYNTAX ` and avoid to embed error's text in source code?
eg: https://github.com/apache/spark/blob/df7e2d07f04c7e16af04c1292227a90516083de1/sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryParsingErrors.scala#L74-L80
Because I think it is not good to assign a new name for the above scenario, because it is a syntax error.
--
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
[GitHub] [spark] MaxGekk commented on a diff in pull request #41214: [SPARK-43549][SQL] Convert `_LEGACY_ERROR_TEMP_0036` to INVALID_SQL_SYNTAX
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #41214:
URL: https://github.com/apache/spark/pull/41214#discussion_r1198595578
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryParsingErrors.scala:
##########
@@ -407,8 +407,8 @@ private[sql] object QueryParsingErrors extends QueryErrorsBase {
def computeStatisticsNotExpectedError(ctx: IdentifierContext): Throwable = {
new ParseException(
- errorClass = "_LEGACY_ERROR_TEMP_0036",
- messageParameters = Map("ctx" -> ctx.getText),
+ errorClass = "INVALID_SQL_SYNTAX",
+ messageParameters = Map("inputString" -> s"${ctx.getText} must be ${toSQLStmt("NOSCAN")}"),
Review Comment:
It would be better to do not embed error's text in source code. Could you leave the error class as a separate one (+ assign name) or at least make it as a sub-class of `INVALID_SQL_SYNTAX`.
--
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
[GitHub] [spark] panbingkun commented on a diff in pull request #41214: [SPARK-43549][SQL] Convert `_LEGACY_ERROR_TEMP_0036` to INVALID_SQL_SYNTAX
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on code in PR #41214:
URL: https://github.com/apache/spark/pull/41214#discussion_r1204022197
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryParsingErrors.scala:
##########
@@ -407,8 +407,8 @@ private[sql] object QueryParsingErrors extends QueryErrorsBase {
def computeStatisticsNotExpectedError(ctx: IdentifierContext): Throwable = {
new ParseException(
- errorClass = "_LEGACY_ERROR_TEMP_0036",
- messageParameters = Map("ctx" -> ctx.getText),
+ errorClass = "INVALID_SQL_SYNTAX",
+ messageParameters = Map("inputString" -> s"${ctx.getText} must be ${toSQLStmt("NOSCAN")}"),
Review Comment:
@MaxGekk Let me re-submit a new PR, it was damaged by me, sorry..
--
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
[GitHub] [spark] panbingkun commented on a diff in pull request #41214: [SPARK-43549][SQL] Convert `_LEGACY_ERROR_TEMP_0036` to INVALID_SQL_SYNTAX
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on code in PR #41214:
URL: https://github.com/apache/spark/pull/41214#discussion_r1204022197
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryParsingErrors.scala:
##########
@@ -407,8 +407,8 @@ private[sql] object QueryParsingErrors extends QueryErrorsBase {
def computeStatisticsNotExpectedError(ctx: IdentifierContext): Throwable = {
new ParseException(
- errorClass = "_LEGACY_ERROR_TEMP_0036",
- messageParameters = Map("ctx" -> ctx.getText),
+ errorClass = "INVALID_SQL_SYNTAX",
+ messageParameters = Map("inputString" -> s"${ctx.getText} must be ${toSQLStmt("NOSCAN")}"),
Review Comment:
@MaxGekk Let me re-submit a new PR, it was damaged by me, sorry.
new PR: https://github.com/apache/spark/pull/41297
--
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
[GitHub] [spark] panbingkun closed pull request #41214: [SPARK-43549][SQL] Convert `_LEGACY_ERROR_TEMP_0036` to INVALID_SQL_SYNTAX
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun closed pull request #41214: [SPARK-43549][SQL] Convert `_LEGACY_ERROR_TEMP_0036` to INVALID_SQL_SYNTAX
URL: https://github.com/apache/spark/pull/41214
--
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
[GitHub] [spark] MaxGekk commented on a diff in pull request #41214: [SPARK-43549][SQL] Convert `_LEGACY_ERROR_TEMP_0036` to INVALID_SQL_SYNTAX
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #41214:
URL: https://github.com/apache/spark/pull/41214#discussion_r1199700956
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryParsingErrors.scala:
##########
@@ -407,8 +407,8 @@ private[sql] object QueryParsingErrors extends QueryErrorsBase {
def computeStatisticsNotExpectedError(ctx: IdentifierContext): Throwable = {
new ParseException(
- errorClass = "_LEGACY_ERROR_TEMP_0036",
- messageParameters = Map("ctx" -> ctx.getText),
+ errorClass = "INVALID_SQL_SYNTAX",
+ messageParameters = Map("inputString" -> s"${ctx.getText} must be ${toSQLStmt("NOSCAN")}"),
Review Comment:
> Should we refactor it to classify INVALID_SQL_SYNTAX and avoid to embed error's text in source code?
Yep. Let's refactor `INVALID_SQL_SYNTAX` first of all, and then come back to 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
[GitHub] [spark] panbingkun commented on a diff in pull request #41214: [SPARK-43549][SQL] Convert `_LEGACY_ERROR_TEMP_0036` to INVALID_SQL_SYNTAX
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on code in PR #41214:
URL: https://github.com/apache/spark/pull/41214#discussion_r1198690104
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryParsingErrors.scala:
##########
@@ -407,8 +407,8 @@ private[sql] object QueryParsingErrors extends QueryErrorsBase {
def computeStatisticsNotExpectedError(ctx: IdentifierContext): Throwable = {
new ParseException(
- errorClass = "_LEGACY_ERROR_TEMP_0036",
- messageParameters = Map("ctx" -> ctx.getText),
+ errorClass = "INVALID_SQL_SYNTAX",
+ messageParameters = Map("inputString" -> s"${ctx.getText} must be ${toSQLStmt("NOSCAN")}"),
Review Comment:
I have found many similar uses in `QueryParsingErrors`. Should we refactor it to classify `INVALID_SQL_SYNTAX ` and avoid to embed error's text in source code?
eg: https://github.com/apache/spark/blob/df7e2d07f04c7e16af04c1292227a90516083de1/sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryParsingErrors.scala#L74-L80
https://github.com/apache/spark/blob/df7e2d07f04c7e16af04c1292227a90516083de1/sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryParsingErrors.scala#L130-L153
Because I think it is not good to assign a new name for the above scenario, because it is a syntax error.
It seems more reasonable to be implemented as a sub-class of INVALID_SQL_SYNTAX.
--
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
[GitHub] [spark] panbingkun commented on a diff in pull request #41214: [SPARK-43549][SQL] Convert `_LEGACY_ERROR_TEMP_0036` to INVALID_SQL_SYNTAX
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on code in PR #41214:
URL: https://github.com/apache/spark/pull/41214#discussion_r1198690104
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryParsingErrors.scala:
##########
@@ -407,8 +407,8 @@ private[sql] object QueryParsingErrors extends QueryErrorsBase {
def computeStatisticsNotExpectedError(ctx: IdentifierContext): Throwable = {
new ParseException(
- errorClass = "_LEGACY_ERROR_TEMP_0036",
- messageParameters = Map("ctx" -> ctx.getText),
+ errorClass = "INVALID_SQL_SYNTAX",
+ messageParameters = Map("inputString" -> s"${ctx.getText} must be ${toSQLStmt("NOSCAN")}"),
Review Comment:
I have found many similar uses in `QueryParsingErrors`. Should we refactor it to classify `INVALID_SQL_SYNTAX ` and avoid to embed error's text in source code?
eg: https://github.com/apache/spark/blob/df7e2d07f04c7e16af04c1292227a90516083de1/sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryParsingErrors.scala#L74-L80
https://github.com/apache/spark/blob/df7e2d07f04c7e16af04c1292227a90516083de1/sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryParsingErrors.scala#L130-L153
Because I think it is not good to assign a new name for the above scenario, because it is a syntax error.
--
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
[GitHub] [spark] MaxGekk commented on a diff in pull request #41214: [SPARK-43549][SQL] Convert `_LEGACY_ERROR_TEMP_0036` to INVALID_SQL_SYNTAX
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #41214:
URL: https://github.com/apache/spark/pull/41214#discussion_r1203859827
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryParsingErrors.scala:
##########
@@ -407,8 +407,8 @@ private[sql] object QueryParsingErrors extends QueryErrorsBase {
def computeStatisticsNotExpectedError(ctx: IdentifierContext): Throwable = {
new ParseException(
- errorClass = "_LEGACY_ERROR_TEMP_0036",
- messageParameters = Map("ctx" -> ctx.getText),
+ errorClass = "INVALID_SQL_SYNTAX",
+ messageParameters = Map("inputString" -> s"${ctx.getText} must be ${toSQLStmt("NOSCAN")}"),
Review Comment:
@panbingkun Could you continue the work on the 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
[GitHub] [spark] panbingkun commented on a diff in pull request #41214: [SPARK-43549][SQL] Convert `_LEGACY_ERROR_TEMP_0036` to INVALID_SQL_SYNTAX
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on code in PR #41214:
URL: https://github.com/apache/spark/pull/41214#discussion_r1203965365
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryParsingErrors.scala:
##########
@@ -407,8 +407,8 @@ private[sql] object QueryParsingErrors extends QueryErrorsBase {
def computeStatisticsNotExpectedError(ctx: IdentifierContext): Throwable = {
new ParseException(
- errorClass = "_LEGACY_ERROR_TEMP_0036",
- messageParameters = Map("ctx" -> ctx.getText),
+ errorClass = "INVALID_SQL_SYNTAX",
+ messageParameters = Map("inputString" -> s"${ctx.getText} must be ${toSQLStmt("NOSCAN")}"),
Review Comment:
Yes, I will continue the work on the PR right now.
--
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