-
Notifications
You must be signed in to change notification settings - Fork 29k
[SPARK-38692][SQL] Use error classes in the compilation errors of function args #36336
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
Conversation
# Conflicts: # sql/core/src/test/scala/org/apache/spark/sql/errors/QueryCompilationErrorsSuite.scala
|
cc @MaxGekk |
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionRegistry.scala
Outdated
Show resolved
Hide resolved
|
Can one of the admins verify this patch? |
# Conflicts: # sql/core/src/test/scala/org/apache/spark/sql/errors/QueryCompilationErrorsSuite.scala
| "sqlState" : "22023" | ||
| }, | ||
| "INVALID_FUNCTION_ARGUMENTS" : { | ||
| "message" : [ "Invalid <invalidType> of arguments for function <functionName>. <message>" ], |
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.
Could you use sub-classes here, see an example in #36307
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.
@MaxGekk Thanks. I will be use sub-classes to correct it later.
|
@lvshaokang Could you resolve conflicts, please. |
# Conflicts: # sql/core/src/test/scala/org/apache/spark/sql/errors/QueryCompilationErrorsSuite.scala
# Conflicts: # core/src/main/resources/error/error-classes.json # sql/core/src/test/scala/org/apache/spark/sql/errors/QueryCompilationErrorsSuite.scala
|
@MaxGekk PTAL.I have resolved conflicts,thanks. |
| "sqlState" : "22023" | ||
| }, | ||
| "INVALID_FUNCTION_ARGUMENTS" : { | ||
| "message" : [ "The function arguments invalid: " ], |
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.
As I can see, every sub-class repeats function name. I would propose to add the function name to the base class, like:
"message" : [ "Arguments of the <funcName> function are invalid: " ],| "message" : [ "The fraction of sec must be zero. Valid range is [0, 60]. If necessary set <config> to false to bypass this error. " ], | ||
| "sqlState" : "22023" | ||
| }, | ||
| "INVALID_FUNCTION_ARGUMENTS" : { |
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.
Would it be possible to use the existing error class INVALID_PARAMETER_VALUE?
# Conflicts: # core/src/main/resources/error/error-classes.json # sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryExecutionErrors.scala # sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/aggregate/FirstLastTestSuite.scala # sql/core/src/test/resources/sql-tests/results/udaf.sql.out # sql/core/src/test/resources/sql-tests/results/udf/udf-udaf.sql.out # sql/core/src/test/scala/org/apache/spark/sql/errors/QueryCompilationErrorsSuite.scala # sql/core/src/test/scala/org/apache/spark/sql/errors/QueryExecutionErrorsSuite.scala
|
@MaxGekk Sry.I recently had time to complete this issue.Please take a look this pr.I unifyed the error class message and moved the |
# Conflicts: # sql/core/src/test/resources/sql-tests/results/postgreSQL/text.sql.out # sql/core/src/test/resources/sql-tests/results/regexp-functions.sql.out
|
We're closing this PR because it hasn't been updated in a while. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable. |
What changes were proposed in this pull request?
Migrate the following errors in QueryCompilationErrors:
Why are the changes needed?
To follow the error class framework and improve test coverage.
Does this PR introduce any user-facing change?
No.
How was this patch tested?