-
Notifications
You must be signed in to change notification settings - Fork 29k
[SPARK-28521][SQL] Fix error message for built-in functions #25261
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
|
cc @mgaido91 |
| } else { | ||
| // Otherwise, find a constructor method that matches the number of arguments, and use that. | ||
| val params = Seq.fill(expressions.size)(classOf[Expression]) | ||
| val f = constructors.find(_.getParameterTypes.toSeq == params).getOrElse { |
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.
but here we're still filtering by expressions..am I missing something?
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.
Two reasons:
- It will throw exception at
CheckAnalysisif parameter is aDataType. So we do not need handle it here:
scala> spark.sql("select cast(int)")
org.apache.spark.sql.AnalysisException: cannot resolve '`int`' given input columns: []; line 1 pos 12;
'Project [unresolvedalias('cast('int), None)]
+- OneRowRelation
at org.apache.spark.sql.catalyst.analysis.package$AnalysisErrorAt.failAnalysis(package.scala:42)
at org.apache.spark.sql.catalyst.analysis.CheckAnalysis$$anonfun$$nestedInanonfun$checkAnalysis$1$2.applyOrElse(CheckAnalysis.scala:113)
at org.apache.spark.sql.catalyst.analysis.CheckAnalysis$$anonfun$$nestedInanonfun$checkAnalysis$1$2.applyOrElse(CheckAnalysis.scala:110)
at org.apache.spark.sql.catalyst.trees.TreeNode.$anonfun$transformUp$2(TreeNode.scala:306)- It's hard to know a parameter is
DataTypeorExpressionhere.
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.
So, I think the problem here is not that we are not considering the DataType args, but the problem IMHO is that we are not handling the case validParametersCount == 0. I think in that case we should throw something like: Invalid arguments for function $name.. Indeed, in the cast case, I'd argue that it is not so clear to me whether the arguments are 1 or 2, since it is not cast(1, string) but cast(1 as string)... So I don't think that it is correct to state that the number of arguments for cast is 2.
|
Test build #108210 has finished for PR 25261 at commit
|
|
Test build #108438 has finished for PR 25261 at commit
|
mgaido91
left a comment
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, just a comment on wording. Thanks.
| validParametersCount.last | ||
| val expectedErrorMsg = validParametersCount.length match { | ||
| case 0 => | ||
| "" |
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.
nit: in case of 0, I'd prefer a more generic Invalid arguments for function $name, instead of Invalid number of arguments for function $name. Because as I mentioned earlier, I think it is questionable which is the number of arguments in this case...
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.
Done
# Conflicts: # sql/core/src/test/scala/org/apache/spark/sql/UDFSuite.scala
|
Test build #108500 has finished for PR 25261 at commit
|
|
Merged to master |
What changes were proposed in this pull request?
The reason is that we did not handle the case
validParametersCount.length == 0because the parameter types can beExpression,DataTypeandOption. This PR makes it handle the casevalidParametersCount.length == 0.How was this patch tested?
unit tests