[GitHub] [spark] amanomer commented on a change in pull request #26712: [SPARK-29883][SQL] Improve error messages when function name is an alias
amanomer 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_r354728758
##
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:
Okay. I will update.
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] amanomer commented on a change in pull request #26712: [SPARK-29883][SQL] Improve error messages when function name is an alias
amanomer 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_r354673653
##
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 are checking only # of arguments. Because till here, every argument
is of type Expression and also every function's constructor takes Expression(s)
as argument. Except `BoolAnd` and `BoolOr`, for which `classOf[String]` is
prepended to Seq.
`expression` is also checking only # of arguments. Validating arguments
types is done by `checkInputDataTypes()`.
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] amanomer commented on a change in pull request #26712: [SPARK-29883][SQL] Improve error messages when function name is an alias
amanomer 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_r354274300
##
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:
It works. Updated in latest commit. cc @cloud-fan
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] amanomer commented on a change in pull request #26712: [SPARK-29883][SQL] Improve error messages when function name is an alias
amanomer 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_r354274300
##
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:
It works
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] amanomer commented on a change in pull request #26712: [SPARK-29883][SQL] Improve error messages when function name is an alias
amanomer 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_r354207829
##
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:
> is it possible to do newInstance(name.toString, expressions: _*)?
No, compilation 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.
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] amanomer commented on a change in pull request #26712: [SPARK-29883][SQL] Improve error messages when function name is an alias
amanomer 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_r354127355
##
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:
cc @cloud-fan
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] amanomer commented on a change in pull request #26712: [SPARK-29883][SQL] Improve error messages when function name is an alias
amanomer 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_r354125022
##
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:
Since, in `expressionWithAlias` we are always passing `expressions.head` to
function's constructor. We can use assert statement
```
...
val builder = (expressions: Seq[Expression]) => {
assert(expressions.size == 1,
s"Invalid number of arguments for function $name. " +
s"Expected: 1; Found: ${expressions.size}")
assert(expressions.head == classOf[Expression],
s"Invalid arguments for function $name")
try {
constructors.head.newInstance(name.toString,
expressions.head).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] amanomer commented on a change in pull request #26712: [SPARK-29883][SQL] Improve error messages when function name is an alias
amanomer 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_r354122496
##
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:
https://github.com/apache/spark/blob/ebd83a544e0eb9fe03e9c1c879e00b50d947a761/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionRegistry.scala#L602-L620
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] amanomer commented on a change in pull request #26712: [SPARK-29883][SQL] Improve error messages when function name is an alias
amanomer 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_r354116842
##
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:
We can validate arguments with assert or as used in `expression`?
cc @cloud-fan @HyukjinKwon
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] amanomer commented on a change in pull request #26712: [SPARK-29883][SQL] Improve error messages when function name is an alias
amanomer 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_r354116036
##
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:
Since, we are not validating arguments, queries like
`SELECT EVERY(true, false);` will result `true`.
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] amanomer commented on a change in pull request #26712: [SPARK-29883][SQL] Improve error messages when function name is an alias
amanomer 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_r353218007
##
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:
Updated in latest commit.
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] amanomer commented on a change in pull request #26712: [SPARK-29883][SQL] Improve error messages when function name is an alias
amanomer 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_r353054610
##
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:
@cloud-fan updated as per your suggestions.
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] amanomer commented on a change in pull request #26712: [SPARK-29883][SQL] Improve error messages when function name is an alias
amanomer 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_r352688792
##
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:
Updated PR. cc @cloud-fan
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] amanomer commented on a change in pull request #26712: [SPARK-29883][SQL] Improve error messages when function name is an alias
amanomer 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_r352431228
##
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:
Argument's data types are matched in Analysis phase of planning and its
optimizer's task to replace `RuntimeReplaceable`, correct me if I'm wrong.
So, optimization rules (`ReplaceExpressions`) will never be applied on
queries like `select every('true')`.
https://github.com/apache/spark/blob/85cb388ae3f25b0e6a7fc1a2d78fd1c3ec03f341/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/Optimizer.scala#L130
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] amanomer commented on a change in pull request #26712: [SPARK-29883][SQL] Improve error messages when function name is an alias
amanomer 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_r352273368
##
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionRegistry.scala
##
@@ -118,7 +118,7 @@ class SimpleFunctionRegistry extends FunctionRegistry with
Logging {
throw new AnalysisException(s"undefined function $name")
}
}
-func(children)
+func(children).setFuncName(name.funcName)
Review comment:
`every` here does not have any node. It will be resolved as a `BoolAnd`.
https://github.com/apache/spark/blob/32af7004a254107340df908e8dd91a0bc22e068e/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionRegistry.scala#L316-L320
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] amanomer commented on a change in pull request #26712: [SPARK-29883][SQL] Improve error messages when function name is an alias
amanomer 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_r352193751
##
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionRegistry.scala
##
@@ -118,7 +118,7 @@ class SimpleFunctionRegistry extends FunctionRegistry with
Logging {
throw new AnalysisException(s"undefined function $name")
}
}
-func(children)
+func(children).setFuncName(name.funcName)
Review comment:
> do functions not already have names somewhere to use, that can already be
set differently per alias?
I think, No? Currently, when we use alias of bool_and (i.e, every), it will
be resolved as a constructor of `BoolAnd` using `FunctionRegistry#expressions`,
which sets `bool_and' as a nodeName.
https://github.com/apache/spark/blob/9351e3e76fb11e9fdaf39aef5aea86fdeccd6f28/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/UnevaluableAggs.scala#L55-L57
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
