spark git commit: [SPARK-20569][SQL] RuntimeReplaceable functions should not take extra parameters

2017-05-11 Thread lixiao 
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 =

spark git commit: [SPARK-20569][SQL] RuntimeReplaceable functions should not take extra parameters

2017-05-11 Thread lixiao 
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"))
+  }
 }