[SPARK-41147][SQL] Assign a name to the legacy error class _LEGACY_ERROR_TEMP_1042
#38664
Conversation
MaxGekk
left a comment
MaxGekk
left a comment
There was a problem hiding this comment.
It seems like a duplicate of DATATYPE_MISMATCH.WRONG_NUM_ARGS. Can't you re-use it?
|
Sure, let me integrate into |
itholic
changed the title
_LEGACY_ERROR_TEMP_1042_LEGACY_ERROR_TEMP_1042
itholic
changed the title
_LEGACY_ERROR_TEMP_1042_LEGACY_ERROR_TEMP_1042
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionRegistry.scala
Outdated
Show resolved
Hide resolved
| .map(_.getParameterCount).distinct.sorted | ||
| throw QueryCompilationErrors.invalidFunctionArgumentNumberError( | ||
| validParametersCount, name, params.length) | ||
| expressions.map(toPrettySQL(_)).mkString(","), |
There was a problem hiding this comment.
Could you pass Seq[Expression] into invalidFunctionArgumentNumberError(), and form a string inside of the method as we do in other places like:
def moreThanOneGeneratorError(generators: Seq[Expression], clause: String): Throwable = {
new AnalysisException(
errorClass = "UNSUPPORTED_GENERATOR.MULTI_GENERATOR",
messageParameters = Map(
"clause" -> clause,
"num" -> generators.size.toString,
"generators" -> generators.map(toSQLExpr).mkString(", ")))
}For instance, if we change the implementation of toSQLExpr(), your exceptions won't pick up new impl.
There was a problem hiding this comment.
Sounds good! Just addressed it.
sql/core/src/test/resources/sql-tests/results/ansi/string-functions.sql.out
Outdated
Show resolved
Hide resolved
sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryCompilationErrors.scala
Outdated
Show resolved
Hide resolved
sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryCompilationErrors.scala
Outdated
Show resolved
Hide resolved
|
Thanks for the review, @MaxGekk |
| "expectedNum" : "2", | ||
| "functionName" : "`decode`" | ||
| "functionName" : "`decode`", | ||
| "sqlExpr" : "\"encode(abc, utf-8)\"" |
There was a problem hiding this comment.
The error message confuses slightly:
"Cannot resolve "encode(abc, utf-8)" due to data type mismatch:"
Actually, the issue is not in encode but in decode. sqlExpr should point out to the parent expression. @itholic Could you change this?
There was a problem hiding this comment.
Let me handle it. Thanks!
There was a problem hiding this comment.
ok. Ping me when you finish this.
| new AnalysisException( | ||
| errorClass = "WRONG_NUM_ARGS", | ||
| messageParameters = Map( | ||
| "sqlExpr" -> sqlExpr.map(toSQLExpr(_)).mkString(","), |
There was a problem hiding this comment.
This is not needed anymore. Could you remove it, please.
There was a problem hiding this comment.
Btw, seems like it's duplicated to #38707 ?
Can we just close this ticket ?
|
Closing since it's duplicated to #38707 |
2 participants
What changes were proposed in this pull request?
This PR proposes to assign a name to
_LEGACY_ERROR_TEMP_1042.Thus, in this PR we integrate
_LEGACY_ERROR_TEMP_1042intoDATATYPE_MISMATCH.WRONG_NUM_ARGSsince it's duplicated.Why are the changes needed?
The legacy error classes were temporary named to cover the all exceptions by error class, so we should assign the proper name on those all legacy error classes.
Does this PR introduce any user-facing change?
No.
How was this patch tested?