Github user drewrobb commented on a diff in the pull request:

    https://github.com/apache/spark/pull/23062#discussion_r234834125
  
    --- Diff: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/ScalaReflection.scala 
---
    @@ -788,12 +788,37 @@ object ScalaReflection extends ScalaReflection {
       }
     
       /**
    -   * Finds an accessible constructor with compatible parameters. This is a 
more flexible search
    -   * than the exact matching algorithm in `Class.getConstructor`. The 
first assignment-compatible
    -   * matching constructor is returned. Otherwise, it returns `None`.
    +   * Finds an accessible constructor with compatible parameters. This is a 
more flexible search than
    +   * the exact matching algorithm in `Class.getConstructor`. The first 
assignment-compatible
    +   * matching constructor is returned if it exists. Otherwise, we check 
for additional compatible
    +   * constructors defined in the companion object as `apply` methods. 
Otherwise, it returns `None`.
        */
    -  def findConstructor(cls: Class[_], paramTypes: Seq[Class[_]]): 
Option[Constructor[_]] = {
    -    Option(ConstructorUtils.getMatchingAccessibleConstructor(cls, 
paramTypes: _*))
    +  def findConstructor(cls: Class[_], paramTypes: Seq[Class[_]]): 
Option[Seq[AnyRef] => Any] = {
    --- End diff --
    
    Real fast I get:
    ```
    [error] 
/home/drew/spark/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/ScalaReflection.scala:798:
 type mismatch;
    [error]  found   : _$31 where type _$31
    [error]  required: T
    [error]       case Some(c) => Some((x: Seq[AnyRef]) => c.newInstance(x: _*))
    [error]                                                             ^
    [error] 
/home/drew/spark/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/ScalaReflection.scala:818:
 type mismatch;
    [error]  found   : Any
    [error]  required: T
    [error]             method.apply(args: _*)
    [error]  
    ```
    
    `Constructor` has a type parameter, but  the signature of `newInstance` 
doesn't use it, and in the second case `method.apply` method returned by 
reflection has a return type of `Any`.  I think that can only be resolved with 
adding `asInstanceOf[Seq[AnyRef] => T]`, which I don't think would be any 
better (unless I'm missing something)?


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

Reply via email to