You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@spark.apache.org by ueshin <gi...@git.apache.org> on 2018/08/17 09:04:24 UTC

[GitHub] spark pull request #22131: [SPARK-25141][SQL][TEST] Modify tests for higher-...

GitHub user ueshin opened a pull request:

    https://github.com/apache/spark/pull/22131

    [SPARK-25141][SQL][TEST] Modify tests for higher-order functions to check bind method.

    ## What changes were proposed in this pull request?
    
    We should also check `HigherOrderFunction.bind` method passes expected parameters.
    This pr modifies tests for higher-order functions to check `bind` method.
    
    ## How was this patch tested?
    
    Modified tests.


You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/ueshin/apache-spark issues/SPARK-25141/bind_test

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/spark/pull/22131.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #22131
    
----
commit 8154bf49d6cd14a681f037b1122024a1390f626e
Author: Takuya UESHIN <ue...@...>
Date:   2018-08-17T08:45:49Z

    Modify tests to check bind method.

----


---

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

[GitHub] spark issue #22131: [SPARK-25141][SQL][TEST] Modify tests for higher-order f...

Posted by ueshin <gi...@git.apache.org>.

Github user ueshin commented on the issue:

    https://github.com/apache/spark/pull/22131
  
    cc @mn-mikke @mgaido91 


---

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

[GitHub] spark issue #22131: [SPARK-25141][SQL][TEST] Modify tests for higher-order f...

Posted by SparkQA <gi...@git.apache.org>.

Github user SparkQA commented on the issue:

    https://github.com/apache/spark/pull/22131
  
    **[Test build #94919 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/94919/testReport)** for PR 22131 at commit [`6f9660d`](https://github.com/apache/spark/commit/6f9660d79e2ae8b7c64dbfea850c514ad3404f37).


---

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

[GitHub] spark issue #22131: [SPARK-25141][SQL][TEST] Modify tests for higher-order f...

Posted by SparkQA <gi...@git.apache.org>.

Github user SparkQA commented on the issue:

    https://github.com/apache/spark/pull/22131
  
    **[Test build #94888 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/94888/testReport)** for PR 22131 at commit [`22c3f48`](https://github.com/apache/spark/commit/22c3f484f611d75f4fd06ca9178fb0eb16d69692).
     * This patch passes all tests.
     * This patch merges cleanly.
     * This patch adds no public classes.


---

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

[GitHub] spark issue #22131: [SPARK-25141][SQL][TEST] Modify tests for higher-order f...

Posted by ueshin <gi...@git.apache.org>.

Github user ueshin commented on the issue:

    https://github.com/apache/spark/pull/22131
  
    Thanks! I'd use this one. merging to master.


---

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

[GitHub] spark issue #22131: [SPARK-25141][SQL][TEST] Modify tests for higher-order f...

Posted by AmplabJenkins <gi...@git.apache.org>.

Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/22131
  
    Merged build finished. Test PASSed.


---

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

[GitHub] spark issue #22131: [SPARK-25141][SQL][TEST] Modify tests for higher-order f...

Posted by mn-mikke <gi...@git.apache.org>.

Github user mn-mikke commented on the issue:

    https://github.com/apache/spark/pull/22131
  
    LGTM too


---

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

[GitHub] spark issue #22131: [SPARK-25141][SQL][TEST] Modify tests for higher-order f...

Posted by AmplabJenkins <gi...@git.apache.org>.

Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/22131
  
    Merged build finished. Test PASSed.


---

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

[GitHub] spark issue #22131: [SPARK-25141][SQL][TEST] Modify tests for higher-order f...

Posted by AmplabJenkins <gi...@git.apache.org>.

Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/22131
  
    Merged build finished. Test PASSed.


---

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

[GitHub] spark issue #22131: [SPARK-25141][SQL][TEST] Modify tests for higher-order f...

Posted by mgaido91 <gi...@git.apache.org>.

Github user mgaido91 commented on the issue:

    https://github.com/apache/spark/pull/22131
  
    yes, this is my worry too... But I don't have any suggestion for that.... If you have anyone, that's great :)


---

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

[GitHub] spark issue #22131: [SPARK-25141][SQL][TEST] Modify tests for higher-order f...

Posted by AmplabJenkins <gi...@git.apache.org>.

Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/22131
  
    Test PASSed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution-unified/2275/
    Test PASSed.


---

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

[GitHub] spark issue #22131: [SPARK-25141][SQL][TEST] Modify tests for higher-order f...

Posted by ueshin <gi...@git.apache.org>.

Github user ueshin commented on the issue:

    https://github.com/apache/spark/pull/22131
  
    Sounds good. I just hope we will never miss to wrap new functions with it. Thanks!


---

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

[GitHub] spark issue #22131: [SPARK-25141][SQL][TEST] Modify tests for higher-order f...

Posted by AmplabJenkins <gi...@git.apache.org>.

Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/22131
  
    Test PASSed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/94883/
    Test PASSed.


---

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

[GitHub] spark issue #22131: [SPARK-25141][SQL][TEST] Modify tests for higher-order f...

Posted by AmplabJenkins <gi...@git.apache.org>.

Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/22131
  
    Test PASSed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution-unified/2274/
    Test PASSed.


---

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

[GitHub] spark issue #22131: [SPARK-25141][SQL][TEST] Modify tests for higher-order f...

Posted by ueshin <gi...@git.apache.org>.

Github user ueshin commented on the issue:

    https://github.com/apache/spark/pull/22131
  
    Actually one of my motivations is to prevent a mistake like #22126. To create a test, we needed to do the same thing in `bind`. The other is I wanted to check the exact values each function passes to bind function as in the description, which we can't know with the end-to-end tests.


---

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

[GitHub] spark issue #22131: [SPARK-25141][SQL][TEST] Modify tests for higher-order f...

Posted by AmplabJenkins <gi...@git.apache.org>.

Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/22131
  
    Merged build finished. Test PASSed.


---

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

[GitHub] spark issue #22131: [SPARK-25141][SQL][TEST] Modify tests for higher-order f...

Posted by SparkQA <gi...@git.apache.org>.

Github user SparkQA commented on the issue:

    https://github.com/apache/spark/pull/22131
  
    **[Test build #94883 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/94883/testReport)** for PR 22131 at commit [`8154bf4`](https://github.com/apache/spark/commit/8154bf49d6cd14a681f037b1122024a1390f626e).
     * This patch passes all tests.
     * This patch merges cleanly.
     * This patch adds no public classes.


---

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

[GitHub] spark issue #22131: [SPARK-25141][SQL][TEST] Modify tests for higher-order f...

Posted by mgaido91 <gi...@git.apache.org>.

Github user mgaido91 commented on the issue:

    https://github.com/apache/spark/pull/22131
  
    I am not sure about this. I think that the `bind` method itself is tested with the end-to-end tests which have been added. This complicates quite this part which is meant only to test the execution of the functions. I don't see much value added by this tests, but I may miss something. Can you enlighten me @ueshin ? Thanks.


---

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

[GitHub] spark issue #22131: [SPARK-25141][SQL][TEST] Modify tests for higher-order f...

Posted by mgaido91 <gi...@git.apache.org>.

Github user mgaido91 commented on the issue:

    https://github.com/apache/spark/pull/22131
  
    I preferred the previous one (but yes, definitely good to add the check about the args size), but I am fine with this one too. Thanks @ueshin 


---

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

[GitHub] spark issue #22131: [SPARK-25141][SQL][TEST] Modify tests for higher-order f...

Posted by AmplabJenkins <gi...@git.apache.org>.

Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/22131
  
    Test PASSed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution-unified/2272/
    Test PASSed.


---

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

[GitHub] spark issue #22131: [SPARK-25141][SQL][TEST] Modify tests for higher-order f...

Posted by mgaido91 <gi...@git.apache.org>.

Github user mgaido91 commented on the issue:

    https://github.com/apache/spark/pull/22131
  
    sorry, I wasn't very specific in my previous comment. I meant adding a method like:
    
    ```
    def validateBinding(e: HigherOrderFunction): Expression = {
        e.bind { case (f: LambdaFunction, dataTypes) =>
          f.arguments.zip(dataTypes).foreach {
            case (arg, (dt, n)) =>
              assert(arg.dataType == dt)
              assert(arg.nullable == n)
          }
          f
        }
      }
    ```
    
    and then validate all the expressions, so use it like:
    ```
    validateBinding(ArrayTransform(....))
    ```
    What do you think about this?


---

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

[GitHub] spark issue #22131: [SPARK-25141][SQL][TEST] Modify tests for higher-order f...

Posted by AmplabJenkins <gi...@git.apache.org>.

Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/22131
  
    Test PASSed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution-unified/2268/
    Test PASSed.


---

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

[GitHub] spark issue #22131: [SPARK-25141][SQL][TEST] Modify tests for higher-order f...

Posted by SparkQA <gi...@git.apache.org>.

Github user SparkQA commented on the issue:

    https://github.com/apache/spark/pull/22131
  
    **[Test build #94887 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/94887/testReport)** for PR 22131 at commit [`dec6978`](https://github.com/apache/spark/commit/dec6978afe874d556be786e1c81e02a413440733).
     * This patch passes all tests.
     * This patch merges cleanly.
     * This patch adds no public classes.


---

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

[GitHub] spark issue #22131: [SPARK-25141][SQL][TEST] Modify tests for higher-order f...

Posted by AmplabJenkins <gi...@git.apache.org>.

Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/22131
  
    Merged build finished. Test PASSed.


---

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

[GitHub] spark issue #22131: [SPARK-25141][SQL][TEST] Modify tests for higher-order f...

Posted by AmplabJenkins <gi...@git.apache.org>.

Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/22131
  
    Test PASSed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/94888/
    Test PASSed.


---

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

[GitHub] spark issue #22131: [SPARK-25141][SQL][TEST] Modify tests for higher-order f...

Posted by mgaido91 <gi...@git.apache.org>.

Github user mgaido91 commented on the issue:

    https://github.com/apache/spark/pull/22131
  
    I see now, thanks. The only thing which I am a bit concerned about is the complexity introduced by this change, in the sense that it is relying very much on the `bind` implementation of each expression. I think it would be great if we could generalize it.
    
    Can we add a generic function similar to the `createLambda` in the analyzer for this?


---

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

[GitHub] spark issue #22131: [SPARK-25141][SQL][TEST] Modify tests for higher-order f...

Posted by AmplabJenkins <gi...@git.apache.org>.

Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/22131
  
    Test PASSed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/94887/
    Test PASSed.


---

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