[GitHub] [spark] cloud-fan commented on a change in pull request #26712: [SPARK-29883][SQL] Improve error messages when function name is an alias
cloud-fan commented on a change in pull request #26712: [SPARK-29883][SQL]
Improve error messages when function name is an alias
URL: https://github.com/apache/spark/pull/26712#discussion_r354698401
##
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionRegistry.scala
##
@@ -618,19 +618,41 @@ object FunctionRegistry {
}
throw new AnalysisException(invalidArgumentsMsg)
}
-Try(f.newInstance(expressions : _*).asInstanceOf[Expression]) match {
- case Success(e) => e
- case Failure(e) =>
-// the exception is an invocation exception. To get a meaningful
message, we need the
-// cause.
-throw new AnalysisException(e.getCause.getMessage)
+try {
+ f.newInstance(expressions : _*).asInstanceOf[Expression]
+} catch {
+ // the exception is an invocation exception. To get a meaningful
message, we need the
+ // cause.
+ case e: Exception => throw new
AnalysisException(e.getCause.getMessage)
}
}
}
(name, (expressionInfo[T](name), builder))
}
+ private def expressionWithAlias[T <: Expression](name: String)
+ (implicit tag: ClassTag[T]): (String, (ExpressionInfo, FunctionBuilder))
= {
+val constructors = tag.runtimeClass.getConstructors
+ .filter(_.getParameterTypes.head == classOf[String])
+assert(constructors.length == 1)
+val builder = (expressions: Seq[Expression]) => {
+ val params = classOf[String] +:
Seq.fill(expressions.size)(classOf[Expression])
+ val f = constructors.find(_.getParameterTypes.toSeq == params).getOrElse
{
Review comment:
> also every function's constructor takes Expression(s) as argument
We don't guarantee it. And we should fail with meaningful error message if
it happens, instead of "Invalid number of arguments"
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #26712: [SPARK-29883][SQL] Improve error messages when function name is an alias
cloud-fan commented on a change in pull request #26712: [SPARK-29883][SQL]
Improve error messages when function name is an alias
URL: https://github.com/apache/spark/pull/26712#discussion_r354671403
##
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionRegistry.scala
##
@@ -618,19 +618,41 @@ object FunctionRegistry {
}
throw new AnalysisException(invalidArgumentsMsg)
}
-Try(f.newInstance(expressions : _*).asInstanceOf[Expression]) match {
- case Success(e) => e
- case Failure(e) =>
-// the exception is an invocation exception. To get a meaningful
message, we need the
-// cause.
-throw new AnalysisException(e.getCause.getMessage)
+try {
+ f.newInstance(expressions : _*).asInstanceOf[Expression]
+} catch {
+ // the exception is an invocation exception. To get a meaningful
message, we need the
+ // cause.
+ case e: Exception => throw new
AnalysisException(e.getCause.getMessage)
}
}
}
(name, (expressionInfo[T](name), builder))
}
+ private def expressionWithAlias[T <: Expression](name: String)
+ (implicit tag: ClassTag[T]): (String, (ExpressionInfo, FunctionBuilder))
= {
+val constructors = tag.runtimeClass.getConstructors
+ .filter(_.getParameterTypes.head == classOf[String])
+assert(constructors.length == 1)
+val builder = (expressions: Seq[Expression]) => {
+ val params = classOf[String] +:
Seq.fill(expressions.size)(classOf[Expression])
+ val f = constructors.find(_.getParameterTypes.toSeq == params).getOrElse
{
Review comment:
here we not only check the # of arguments, but also argument types. Shall we
only check the # of arguments and leave the type check to the `newInstance`
call?
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #26712: [SPARK-29883][SQL] Improve error messages when function name is an alias
cloud-fan commented on a change in pull request #26712: [SPARK-29883][SQL]
Improve error messages when function name is an alias
URL: https://github.com/apache/spark/pull/26712#discussion_r354248459
##
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionRegistry.scala
##
@@ -618,19 +618,36 @@ object FunctionRegistry {
}
throw new AnalysisException(invalidArgumentsMsg)
}
-Try(f.newInstance(expressions : _*).asInstanceOf[Expression]) match {
- case Success(e) => e
- case Failure(e) =>
-// the exception is an invocation exception. To get a meaningful
message, we need the
-// cause.
-throw new AnalysisException(e.getCause.getMessage)
+try {
+ f.newInstance(expressions : _*).asInstanceOf[Expression]
+} catch {
+ // the exception is an invocation exception. To get a meaningful
message, we need the
+ // cause.
+ case e: Exception => throw new
AnalysisException(e.getCause.getMessage)
}
}
}
(name, (expressionInfo[T](name), builder))
}
+ private def expressionWithAlias[T <: Expression](name: String)
+ (implicit tag: ClassTag[T]): (String, (ExpressionInfo, FunctionBuilder))
= {
+val constructors = tag.runtimeClass.getConstructors
+ .filter(_.getParameterTypes.head == classOf[String])
+assert(constructors.length == 1)
+val builder = (expressions: Seq[Expression]) => {
+ try {
+constructors.head.newInstance(name.toString,
expressions.head).asInstanceOf[Expression]
Review comment:
how about `newInstance((name.toString +: expressions): _*)`?
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #26712: [SPARK-29883][SQL] Improve error messages when function name is an alias
cloud-fan commented on a change in pull request #26712: [SPARK-29883][SQL]
Improve error messages when function name is an alias
URL: https://github.com/apache/spark/pull/26712#discussion_r354160319
##
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionRegistry.scala
##
@@ -618,19 +618,36 @@ object FunctionRegistry {
}
throw new AnalysisException(invalidArgumentsMsg)
}
-Try(f.newInstance(expressions : _*).asInstanceOf[Expression]) match {
- case Success(e) => e
- case Failure(e) =>
-// the exception is an invocation exception. To get a meaningful
message, we need the
-// cause.
-throw new AnalysisException(e.getCause.getMessage)
+try {
+ f.newInstance(expressions : _*).asInstanceOf[Expression]
+} catch {
+ // the exception is an invocation exception. To get a meaningful
message, we need the
+ // cause.
+ case e: Exception => throw new
AnalysisException(e.getCause.getMessage)
}
}
}
(name, (expressionInfo[T](name), builder))
}
+ private def expressionWithAlias[T <: Expression](name: String)
+ (implicit tag: ClassTag[T]): (String, (ExpressionInfo, FunctionBuilder))
= {
+val constructors = tag.runtimeClass.getConstructors
+ .filter(_.getParameterTypes.head == classOf[String])
+assert(constructors.length == 1)
+val builder = (expressions: Seq[Expression]) => {
+ try {
+constructors.head.newInstance(name.toString,
expressions.head).asInstanceOf[Expression]
Review comment:
BTW is it possible to do `newInstance(name.toString, expressions: _*)`? Then
it can work for other expressions that take more than 1 parameter.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #26712: [SPARK-29883][SQL] Improve error messages when function name is an alias
cloud-fan commented on a change in pull request #26712: [SPARK-29883][SQL]
Improve error messages when function name is an alias
URL: https://github.com/apache/spark/pull/26712#discussion_r354159692
##
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionRegistry.scala
##
@@ -618,19 +618,36 @@ object FunctionRegistry {
}
throw new AnalysisException(invalidArgumentsMsg)
}
-Try(f.newInstance(expressions : _*).asInstanceOf[Expression]) match {
- case Success(e) => e
- case Failure(e) =>
-// the exception is an invocation exception. To get a meaningful
message, we need the
-// cause.
-throw new AnalysisException(e.getCause.getMessage)
+try {
+ f.newInstance(expressions : _*).asInstanceOf[Expression]
+} catch {
+ // the exception is an invocation exception. To get a meaningful
message, we need the
+ // cause.
+ case e: Exception => throw new
AnalysisException(e.getCause.getMessage)
}
}
}
(name, (expressionInfo[T](name), builder))
}
+ private def expressionWithAlias[T <: Expression](name: String)
+ (implicit tag: ClassTag[T]): (String, (ExpressionInfo, FunctionBuilder))
= {
+val constructors = tag.runtimeClass.getConstructors
+ .filter(_.getParameterTypes.head == classOf[String])
+assert(constructors.length == 1)
+val builder = (expressions: Seq[Expression]) => {
+ try {
+constructors.head.newInstance(name.toString,
expressions.head).asInstanceOf[Expression]
Review comment:
SGTM
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #26712: [SPARK-29883][SQL] Improve error messages when function name is an alias
cloud-fan commented on a change in pull request #26712: [SPARK-29883][SQL]
Improve error messages when function name is an alias
URL: https://github.com/apache/spark/pull/26712#discussion_r354122027
##
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionRegistry.scala
##
@@ -618,19 +618,36 @@ object FunctionRegistry {
}
throw new AnalysisException(invalidArgumentsMsg)
}
-Try(f.newInstance(expressions : _*).asInstanceOf[Expression]) match {
- case Success(e) => e
- case Failure(e) =>
-// the exception is an invocation exception. To get a meaningful
message, we need the
-// cause.
-throw new AnalysisException(e.getCause.getMessage)
+try {
+ f.newInstance(expressions : _*).asInstanceOf[Expression]
+} catch {
+ // the exception is an invocation exception. To get a meaningful
message, we need the
+ // cause.
+ case e: Exception => throw new
AnalysisException(e.getCause.getMessage)
}
}
}
(name, (expressionInfo[T](name), builder))
}
+ private def expressionWithAlias[T <: Expression](name: String)
+ (implicit tag: ClassTag[T]): (String, (ExpressionInfo, FunctionBuilder))
= {
+val constructors = tag.runtimeClass.getConstructors
+ .filter(_.getParameterTypes.head == classOf[String])
+assert(constructors.length == 1)
+val builder = (expressions: Seq[Expression]) => {
+ try {
+constructors.head.newInstance(name.toString,
expressions.head).asInstanceOf[Expression]
Review comment:
how is it done in `def expression`?
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #26712: [SPARK-29883][SQL] Improve error messages when function name is an alias
cloud-fan commented on a change in pull request #26712: [SPARK-29883][SQL]
Improve error messages when function name is an alias
URL: https://github.com/apache/spark/pull/26712#discussion_r353109771
##
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionRegistry.scala
##
@@ -631,6 +631,24 @@ object FunctionRegistry {
(name, (expressionInfo[T](name), builder))
}
+ private def expressionWithAlias[T <: Expression](name: String)
+ (implicit tag: ClassTag[T]): (String, (ExpressionInfo, FunctionBuilder))
= {
+val constructors = tag.runtimeClass.getConstructors
+ .filter(_.getParameterTypes.head == classOf[String])
+assert(constructors.length == 1)
+val builder = (expressions: Seq[Expression]) => {
+ Try(constructors.head.newInstance(name.toString,
expressions.head).asInstanceOf[Expression])
Review comment:
seems better to use the normal try catch?
```
try {
constructors.head.newInstance(name.toString,
expressions.head).asInstanceOf[Expression]
} catch {
// the original comment ...
case e => throw new AnalysisException(e.getCause.getMessage)
}
```
We can update `def expression` as well.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #26712: [SPARK-29883][SQL] Improve error messages when function name is an alias
cloud-fan commented on a change in pull request #26712: [SPARK-29883][SQL]
Improve error messages when function name is an alias
URL: https://github.com/apache/spark/pull/26712#discussion_r353040270
##
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionRegistry.scala
##
@@ -600,7 +600,8 @@ object FunctionRegistry {
} 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 {
+val f = constructors.find(e => e.getParameterTypes.toSeq == params
+|| e.getParameterTypes.head == classOf[String]).getOrElse {
Review comment:
then we don't even need the `MultiNamedExpression` trait. We just need to
register `bool_and`, `bool_or` with `expressionWithAlias`
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #26712: [SPARK-29883][SQL] Improve error messages when function name is an alias
cloud-fan commented on a change in pull request #26712: [SPARK-29883][SQL]
Improve error messages when function name is an alias
URL: https://github.com/apache/spark/pull/26712#discussion_r353039726
##
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionRegistry.scala
##
@@ -600,7 +600,8 @@ object FunctionRegistry {
} 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 {
+val f = constructors.find(e => e.getParameterTypes.toSeq == params
+|| e.getParameterTypes.head == classOf[String]).getOrElse {
Review comment:
Seems like it's less hacky to create a new `expressionWithAlias` method,
with only the necessary logic
```
def expressionWithAlias ... = {
val constructors = tag.runtimeClass.getConstructors
.filter(c => e.getParameterTypes.head == classOf[String])
assert(constructors.length == 1)
try {
constructors.head.newInstance(name, expressions :
_*).asInstanceOf[Expression]
} ...
}
```
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #26712: [SPARK-29883][SQL] Improve error messages when function name is an alias
cloud-fan commented on a change in pull request #26712: [SPARK-29883][SQL]
Improve error messages when function name is an alias
URL: https://github.com/apache/spark/pull/26712#discussion_r353039726
##
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionRegistry.scala
##
@@ -600,7 +600,8 @@ object FunctionRegistry {
} 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 {
+val f = constructors.find(e => e.getParameterTypes.toSeq == params
+|| e.getParameterTypes.head == classOf[String]).getOrElse {
Review comment:
Seems like it's less hacky to create a new `expressionWithAlias` method,
with only the necessary logic
```
def expressionWithAlias ... = {
val constructors = tag.runtimeClass.getConstructors.filter(c =>
e.getParameterTypes.head == classOf[String])
assert(constructors.length == 1)
try {
constructors.head.newInstance(name, expressions :
_*).asInstanceOf[Expression]
} ...
}
```
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #26712: [SPARK-29883][SQL] Improve error messages when function name is an alias
cloud-fan commented on a change in pull request #26712: [SPARK-29883][SQL]
Improve error messages when function name is an alias
URL: https://github.com/apache/spark/pull/26712#discussion_r352452141
##
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/UnevaluableAggs.scala
##
@@ -53,7 +53,6 @@ abstract class UnevaluableBooleanAggBase(arg: Expression)
""",
since = "3.0.0")
case class BoolAnd(arg: Expression) extends UnevaluableBooleanAggBase(arg) {
Review comment:
how about
```
case class BoolAnd(functionName: String, arg: Expression) ... with
MultiNamesFunction {
def nodeName = functionName
}
```
We can update `FunctionRegistry.expression` to detect `MultiNamesFunction`
and pass name to the constructor.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #26712: [SPARK-29883][SQL] Improve error messages when function name is an alias
cloud-fan commented on a change in pull request #26712: [SPARK-29883][SQL]
Improve error messages when function name is an alias
URL: https://github.com/apache/spark/pull/26712#discussion_r352410699
##
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Expression.scala
##
@@ -286,6 +286,15 @@ abstract class Expression extends TreeNode[Expression] {
override def simpleStringWithNodeId(): String = {
throw new UnsupportedOperationException(s"$nodeName does not implement
simpleStringWithNodeId")
}
+
+ var functionAlias: String = ""
Review comment:
It's a bad practice to make the `Expression` mutable for trivial things like
this. How about using `RuntimeReplaceable`?
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
