[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