spark git commit: [SPARK-20569][SQL] RuntimeReplaceable functions should not take extra parameters
Repository: spark Updated Branches: refs/heads/branch-2.2 80a57fa90 -> dd9e3b2c9 [SPARK-20569][SQL] RuntimeReplaceable functions should not take extra parameters ## What changes were proposed in this pull request? `RuntimeReplaceable` always has a constructor with the expression to replace with, and this constructor should not be the function builder. ## How was this patch tested? new regression test Author: Wenchen Fan Closes #17876 from cloud-fan/minor. (cherry picked from commit b4c99f43690f8cfba414af90fa2b3998a510bba8) Signed-off-by: Xiao Li Project: http://git-wip-us.apache.org/repos/asf/spark/repo Commit: http://git-wip-us.apache.org/repos/asf/spark/commit/dd9e3b2c Tree: http://git-wip-us.apache.org/repos/asf/spark/tree/dd9e3b2c Diff: http://git-wip-us.apache.org/repos/asf/spark/diff/dd9e3b2c Branch: refs/heads/branch-2.2 Commit: dd9e3b2c976a4ef3b4837590a2ba0954cf73860d Parents: 80a57fa Author: Wenchen Fan Authored: Thu May 11 00:41:15 2017 -0700 Committer: Xiao Li Committed: Thu May 11 00:41:35 2017 -0700 -- .../catalyst/analysis/FunctionRegistry.scala| 20 ++-- .../org/apache/spark/sql/SQLQuerySuite.scala| 5 + 2 files changed, 19 insertions(+), 6 deletions(-) -- http://git-wip-us.apache.org/repos/asf/spark/blob/dd9e3b2c/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionRegistry.scala -- diff --git a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionRegistry.scala b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionRegistry.scala index e1d83a8..6fc154f 100644 --- a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionRegistry.scala +++ b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionRegistry.scala @@ -17,6 +17,8 @@ package org.apache.spark.sql.catalyst.analysis +import java.lang.reflect.Modifier + import scala.language.existentials import scala.reflect.ClassTag import scala.util.{Failure, Success, Try} @@ -455,8 +457,17 @@ object FunctionRegistry { private def expression[T <: Expression](name: String) (implicit tag: ClassTag[T]): (String, (ExpressionInfo, FunctionBuilder)) = { +// For `RuntimeReplaceable`, skip the constructor with most arguments, which is the main +// constructor and contains non-parameter `child` and should not be used as function builder. +val constructors = if (classOf[RuntimeReplaceable].isAssignableFrom(tag.runtimeClass)) { + val all = tag.runtimeClass.getConstructors + val maxNumArgs = all.map(_.getParameterCount).max + all.filterNot(_.getParameterCount == maxNumArgs) +} else { + tag.runtimeClass.getConstructors +} // See if we can find a constructor that accepts Seq[Expression] -val varargCtor = Try(tag.runtimeClass.getDeclaredConstructor(classOf[Seq[_]])).toOption +val varargCtor = constructors.find(_.getParameterTypes.toSeq == Seq(classOf[Seq[_]])) val builder = (expressions: Seq[Expression]) => { if (varargCtor.isDefined) { // If there is an apply method that accepts Seq[Expression], use that one. @@ -470,11 +481,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 = Try(tag.runtimeClass.getDeclaredConstructor(params : _*)) match { - case Success(e) => -e - case Failure(e) => -throw new AnalysisException(s"Invalid number of arguments for function $name") +val f = constructors.find(_.getParameterTypes.toSeq == params).getOrElse { + throw new AnalysisException(s"Invalid number of arguments for function $name") } Try(f.newInstance(expressions : _*).asInstanceOf[Expression]) match { case Success(e) => e http://git-wip-us.apache.org/repos/asf/spark/blob/dd9e3b2c/sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala -- diff --git a/sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala b/sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala index 3ecbf96..cd14d24 100644 --- a/sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala +++ b/sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala @@ -2619,4 +2619,9 @@ class SQLQuerySuite extends QueryTest with SharedSQLContext { new URL(jarFromInvalidFs) } } + + test("RuntimeReplaceable functions should not take extra parameters") { +val e = intercept[AnalysisException](sql("SELECT nvl(1, 2, 3)")) +assert(e.message.contains("Invalid number of
spark git commit: [SPARK-20569][SQL] RuntimeReplaceable functions should not take extra parameters
Repository: spark Updated Branches: refs/heads/master 65accb813 -> b4c99f436 [SPARK-20569][SQL] RuntimeReplaceable functions should not take extra parameters ## What changes were proposed in this pull request? `RuntimeReplaceable` always has a constructor with the expression to replace with, and this constructor should not be the function builder. ## How was this patch tested? new regression test Author: Wenchen Fan Closes #17876 from cloud-fan/minor. Project: http://git-wip-us.apache.org/repos/asf/spark/repo Commit: http://git-wip-us.apache.org/repos/asf/spark/commit/b4c99f43 Tree: http://git-wip-us.apache.org/repos/asf/spark/tree/b4c99f43 Diff: http://git-wip-us.apache.org/repos/asf/spark/diff/b4c99f43 Branch: refs/heads/master Commit: b4c99f43690f8cfba414af90fa2b3998a510bba8 Parents: 65accb8 Author: Wenchen Fan Authored: Thu May 11 00:41:15 2017 -0700 Committer: Xiao Li Committed: Thu May 11 00:41:15 2017 -0700 -- .../catalyst/analysis/FunctionRegistry.scala| 20 ++-- .../org/apache/spark/sql/SQLQuerySuite.scala| 5 + 2 files changed, 19 insertions(+), 6 deletions(-) -- http://git-wip-us.apache.org/repos/asf/spark/blob/b4c99f43/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionRegistry.scala -- diff --git a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionRegistry.scala b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionRegistry.scala index e1d83a8..6fc154f 100644 --- a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionRegistry.scala +++ b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionRegistry.scala @@ -17,6 +17,8 @@ package org.apache.spark.sql.catalyst.analysis +import java.lang.reflect.Modifier + import scala.language.existentials import scala.reflect.ClassTag import scala.util.{Failure, Success, Try} @@ -455,8 +457,17 @@ object FunctionRegistry { private def expression[T <: Expression](name: String) (implicit tag: ClassTag[T]): (String, (ExpressionInfo, FunctionBuilder)) = { +// For `RuntimeReplaceable`, skip the constructor with most arguments, which is the main +// constructor and contains non-parameter `child` and should not be used as function builder. +val constructors = if (classOf[RuntimeReplaceable].isAssignableFrom(tag.runtimeClass)) { + val all = tag.runtimeClass.getConstructors + val maxNumArgs = all.map(_.getParameterCount).max + all.filterNot(_.getParameterCount == maxNumArgs) +} else { + tag.runtimeClass.getConstructors +} // See if we can find a constructor that accepts Seq[Expression] -val varargCtor = Try(tag.runtimeClass.getDeclaredConstructor(classOf[Seq[_]])).toOption +val varargCtor = constructors.find(_.getParameterTypes.toSeq == Seq(classOf[Seq[_]])) val builder = (expressions: Seq[Expression]) => { if (varargCtor.isDefined) { // If there is an apply method that accepts Seq[Expression], use that one. @@ -470,11 +481,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 = Try(tag.runtimeClass.getDeclaredConstructor(params : _*)) match { - case Success(e) => -e - case Failure(e) => -throw new AnalysisException(s"Invalid number of arguments for function $name") +val f = constructors.find(_.getParameterTypes.toSeq == params).getOrElse { + throw new AnalysisException(s"Invalid number of arguments for function $name") } Try(f.newInstance(expressions : _*).asInstanceOf[Expression]) match { case Success(e) => e http://git-wip-us.apache.org/repos/asf/spark/blob/b4c99f43/sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala -- diff --git a/sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala b/sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala index 3ecbf96..cd14d24 100644 --- a/sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala +++ b/sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala @@ -2619,4 +2619,9 @@ class SQLQuerySuite extends QueryTest with SharedSQLContext { new URL(jarFromInvalidFs) } } + + test("RuntimeReplaceable functions should not take extra parameters") { +val e = intercept[AnalysisException](sql("SELECT nvl(1, 2, 3)")) +assert(e.message.contains("Invalid number of arguments")) + } } - To unsubs