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 2016/05/17 10:01:16 UTC

[GitHub] spark pull request: [SPARK-6320][SQL] Move planLater method into G...

GitHub user ueshin opened a pull request:

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

    [SPARK-6320][SQL] Move planLater method into GenericStrategy.

    ## What changes were proposed in this pull request?
    
    This PR moves `QueryPlanner.planLater()` method into `GenericStrategy` for extra strategies to be able to use `planLater` in its strategy.
    
    ## How was this patch tested?
    
    Existing tests.


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

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

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

    https://github.com/apache/spark/pull/13147.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 #13147
    
----
commit e7554f005b92b7cf57fb40e32b6fa7419a93b9d2
Author: Takuya UESHIN <ue...@happy-camper.st>
Date:   2016-05-17T09:20:40Z

    Move planLater method into GenericStrategy.

----


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-6320][SQL] Move planLater method into G...

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

Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/13147#issuecomment-222577825
  
    **[Test build #59627 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/59627/consoleFull)** for PR 13147 at commit [`254381d`](https://github.com/apache/spark/commit/254381d245cabf3cbad57f7ab06eec155ae79d96).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [WIP][SPARK-6320][SQL] Move planLater method i...

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

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

    https://github.com/apache/spark/pull/13147#discussion_r64972400
  
    --- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/planning/QueryPlanner.scala ---
    @@ -47,17 +55,27 @@ abstract class QueryPlanner[PhysicalPlan <: TreeNode[PhysicalPlan]] {
       /** A list of execution strategies that can be used by the planner */
       def strategies: Seq[GenericStrategy[PhysicalPlan]]
     
    -  /**
    -   * Returns a placeholder for a physical plan that executes `plan`. This placeholder will be
    -   * filled in automatically by the QueryPlanner using the other execution strategies that are
    -   * available.
    -   */
    -  protected def planLater(plan: LogicalPlan): PhysicalPlan = this.plan(plan).next()
    -
       def plan(plan: LogicalPlan): Iterator[PhysicalPlan] = {
         // Obviously a lot to do here still...
    -    val iter = strategies.view.flatMap(_(plan)).toIterator
    +    val iter = strategies.view.flatMap(_(plan)).toIterator.flatMap { physicalPlan =>
    +      val placeholders = collectPlaceholders(physicalPlan)
    +
    +      (Iterator(physicalPlan) /: placeholders.toIterator) {
    +        case (physicalPlans, (placeholder, logicalPlan)) =>
    +          val children = this.plan(logicalPlan)
    +          physicalPlans.flatMap { physicalPlan =>
    +            children.map { child =>
    +              physicalPlan.transformUp {
    +                case `placeholder` => child
    --- End diff --
    
    You might also explicitly spell out what is happening here.  It's doing the following right?
    
    ```scala
    case p if p == placeholder => child
    ```


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-6320][SQL] Move planLater method into G...

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

Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/13147#issuecomment-219673646
  
    **[Test build #58683 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/58683/consoleFull)** for PR 13147 at commit [`e7554f0`](https://github.com/apache/spark/commit/e7554f005b92b7cf57fb40e32b6fa7419a93b9d2).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-6320][SQL] Move planLater method into G...

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

Github user ueshin commented on the pull request:

    https://github.com/apache/spark/pull/13147#issuecomment-222403715
  
    @marmbrus Could you review this pr again and let me know if I have something to do to merge this.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-6320][SQL] Move planLater method into G...

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

Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/13147#issuecomment-219692871
  
    **[Test build #58683 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/58683/consoleFull)** for PR 13147 at commit [`e7554f0`](https://github.com/apache/spark/commit/e7554f005b92b7cf57fb40e32b6fa7419a93b9d2).
     * This patch passes all tests.
     * This patch merges cleanly.
     * This patch adds no public classes.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark issue #13147: [SPARK-6320][SQL] Move planLater method into GenericStra...

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

Github user marmbrus commented on the issue:

    https://github.com/apache/spark/pull/13147
  
    Thanks, merged to master.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [WIP][SPARK-6320][SQL] Move planLater method i...

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

Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/13147#issuecomment-222078986
  
    **[Test build #59474 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/59474/consoleFull)** for PR 13147 at commit [`85d412a`](https://github.com/apache/spark/commit/85d412aa2431333ecf0ba8de270777b0a994fcb3).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [WIP][SPARK-6320][SQL] Move planLater method i...

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

Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/13147#issuecomment-222124875
  
    Merged build finished. Test PASSed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [WIP][SPARK-6320][SQL] Move planLater method i...

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

Github user ueshin commented on the pull request:

    https://github.com/apache/spark/pull/13147#issuecomment-222078722
  
    This is a simple wip version with `PlanLater` placeholder.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark issue #13147: [SPARK-6320][SQL] Move planLater method into GenericStra...

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

Github user marmbrus commented on the issue:

    https://github.com/apache/spark/pull/13147
  
    test this please


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [WIP][SPARK-6320][SQL] Move planLater method i...

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

Github user marmbrus commented on the pull request:

    https://github.com/apache/spark/pull/13147#issuecomment-222263996
  
    This is great!  In order to commit this it would be good to explain what is happening in detail (I think I follow, but its very dense).  I'd also like to see some tests to make sure that we are finding the first plan without going down any of the possible branches.
    
    One consideration: we might want to make at least the API parts of this make it into 2.0 since it makes plugging in strategies much more powerful and it does change this (experimental) interface.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request #13147: [SPARK-6320][SQL] Move planLater method into Gene...

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

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

    https://github.com/apache/spark/pull/13147#discussion_r90375148
  
    --- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/planning/QueryPlanner.scala ---
    @@ -27,6 +27,14 @@ import org.apache.spark.sql.catalyst.trees.TreeNode
      * empty list should be returned.
      */
     abstract class GenericStrategy[PhysicalPlan <: TreeNode[PhysicalPlan]] extends Logging {
    +
    +  /**
    +   * Returns a placeholder for a physical plan that executes `plan`. This placeholder will be
    +   * filled in automatically by the QueryPlanner using the other execution strategies that are
    +   * available.
    +   */
    +  protected def planLater(plan: LogicalPlan): PhysicalPlan
    +
       def apply(plan: LogicalPlan): Seq[PhysicalPlan]
     }
     
    --- End diff --
    
    I see, should I send a pr to remove it or can I include it to #15927?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-6320][SQL] Move planLater method into G...

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

Github user marmbrus commented on the pull request:

    https://github.com/apache/spark/pull/13147#issuecomment-221968384
  
    Unfortunately, I don't have the code anymore, but I can try to sketch out what I think the right solution looks like.  Basically, the problem is that `planLater` is a lie.  It eagerly plans, taking the first available path.
    
    The reason that a `Strategy` is able to return more than one plan is to allow us to explore multiple valid options and evaluate their cost relative to the other options.  My proposal would be to change plan later from a function to a special linking node:
    
    ```scala
    case class PlanLater(tree: LogicalPlan) extends SparkPlan
    ```
    
    In order for this to work we will need to make the query planner into an iterator.  When you call `next()` it does a depth first exploration using the list of strategies until there are no more `PlanLater` nodes left.  Along the way it should remember the branches that it has not taken, so that they can be returned for subsequent `next()` calls.  What do you think?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [WIP][SPARK-6320][SQL] Move planLater method i...

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

Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/13147#issuecomment-222165013
  
    **[Test build #59491 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/59491/consoleFull)** for PR 13147 at commit [`42c18ab`](https://github.com/apache/spark/commit/42c18ab847ab8b1e112a3a3129bd7483b21a486c).
     * This patch passes all tests.
     * This patch merges cleanly.
     * This patch adds no public classes.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [WIP][SPARK-6320][SQL] Move planLater method i...

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

Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/13147#issuecomment-222339353
  
    Merged build finished. Test PASSed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [WIP][SPARK-6320][SQL] Move planLater method i...

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

Github user AmplabJenkins commented on the pull request:

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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request #13147: [SPARK-6320][SQL] Move planLater method into Gene...

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

Github user asfgit closed the pull request at:

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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-6320][SQL] Move planLater method into G...

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

Github user AmplabJenkins commented on the pull request:

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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request #13147: [SPARK-6320][SQL] Move planLater method into Gene...

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

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

    https://github.com/apache/spark/pull/13147#discussion_r90566125
  
    --- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/planning/QueryPlanner.scala ---
    @@ -27,6 +27,14 @@ import org.apache.spark.sql.catalyst.trees.TreeNode
      * empty list should be returned.
      */
     abstract class GenericStrategy[PhysicalPlan <: TreeNode[PhysicalPlan]] extends Logging {
    +
    +  /**
    +   * Returns a placeholder for a physical plan that executes `plan`. This placeholder will be
    +   * filled in automatically by the QueryPlanner using the other execution strategies that are
    +   * available.
    +   */
    +  protected def planLater(plan: LogicalPlan): PhysicalPlan
    +
       def apply(plan: LogicalPlan): Seq[PhysicalPlan]
     }
     
    --- End diff --
    
    I see, I'll include it. Thanks!


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-6320][SQL] Move planLater method into GenericStra...

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

Github user marmbrus commented on the pull request:

    https://github.com/apache/spark/pull/13147
  
    Quick question:  Since we are really close to Spark 2.0 this is probably too big of a change to merge into `branch-2.0` (though @rxin should probably make that call).  However, even though this is an experimental API, it would be nice to including this new version if we think thats the long term plan for this extension point.
    
    Is there some minimal version of this that add just the new API, but still just finds one plan?  We could put that in `branch-2.0` and this PR into `master`.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [WIP][SPARK-6320][SQL] Move planLater method i...

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

Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/13147#issuecomment-222283201
  
    **[Test build #59545 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/59545/consoleFull)** for PR 13147 at commit [`a3524cd`](https://github.com/apache/spark/commit/a3524cd220f840839a7f40c570fe2c5da4c7705b).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [WIP][SPARK-6320][SQL] Move planLater method i...

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

Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/13147#issuecomment-222342993
  
    **[Test build #59576 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/59576/consoleFull)** for PR 13147 at commit [`fbc9a9f`](https://github.com/apache/spark/commit/fbc9a9f08b65e1210e9db9eb92a9769bcfb693a9).
     * This patch passes all tests.
     * This patch merges cleanly.
     * This patch adds no public classes.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [WIP][SPARK-6320][SQL] Move planLater method i...

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

Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/13147#issuecomment-222285952
  
    Merged build finished. Test FAILed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark issue #13147: [SPARK-6320][SQL] Move planLater method into GenericStra...

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

Github user SparkQA commented on the issue:

    https://github.com/apache/spark/pull/13147
  
    **[Test build #60207 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/60207/consoleFull)** for PR 13147 at commit [`254381d`](https://github.com/apache/spark/commit/254381d245cabf3cbad57f7ab06eec155ae79d96).
     * This patch passes all tests.
     * This patch merges cleanly.
     * This patch adds no public classes.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [WIP][SPARK-6320][SQL] Move planLater method i...

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

Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/13147#issuecomment-222165313
  
    Merged build finished. Test PASSed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-6320][SQL] Move planLater method into G...

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

Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/13147#issuecomment-222403702
  
    **[Test build #59604 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/59604/consoleFull)** for PR 13147 at commit [`a61584b`](https://github.com/apache/spark/commit/a61584b9dfb8360ee4af66ca7a6edda9bbe22a78).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-6320][SQL] Move planLater method into G...

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

Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/13147#issuecomment-222409876
  
    Merged build finished. Test PASSed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [WIP][SPARK-6320][SQL] Move planLater method i...

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

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

    https://github.com/apache/spark/pull/13147#discussion_r64972461
  
    --- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/planning/QueryPlanner.scala ---
    @@ -47,17 +55,27 @@ abstract class QueryPlanner[PhysicalPlan <: TreeNode[PhysicalPlan]] {
       /** A list of execution strategies that can be used by the planner */
       def strategies: Seq[GenericStrategy[PhysicalPlan]]
     
    -  /**
    -   * Returns a placeholder for a physical plan that executes `plan`. This placeholder will be
    -   * filled in automatically by the QueryPlanner using the other execution strategies that are
    -   * available.
    -   */
    -  protected def planLater(plan: LogicalPlan): PhysicalPlan = this.plan(plan).next()
    -
       def plan(plan: LogicalPlan): Iterator[PhysicalPlan] = {
         // Obviously a lot to do here still...
    -    val iter = strategies.view.flatMap(_(plan)).toIterator
    +    val iter = strategies.view.flatMap(_(plan)).toIterator.flatMap { physicalPlan =>
    +      val placeholders = collectPlaceholders(physicalPlan)
    +
    +      (Iterator(physicalPlan) /: placeholders.toIterator) {
    +        case (physicalPlans, (placeholder, logicalPlan)) =>
    +          val children = this.plan(logicalPlan)
    +          physicalPlans.flatMap { physicalPlan =>
    +            children.map { child =>
    +              physicalPlan.transformUp {
    +                case `placeholder` => child
    +              }
    +            }
    +          }
    +      }
    +    }
    +    // TODO: We will need to prune bad plans to prevent from combinatorial explosion.
    --- End diff --
    
    We are safe for now though as long as we only call `next()` once right?  Is there some way to test that its doing the right thing?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-6320][SQL] Move planLater method into G...

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

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

    https://github.com/apache/spark/pull/13147#discussion_r65107682
  
    --- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/planning/QueryPlanner.scala ---
    @@ -47,17 +55,34 @@ abstract class QueryPlanner[PhysicalPlan <: TreeNode[PhysicalPlan]] {
       /** A list of execution strategies that can be used by the planner */
       def strategies: Seq[GenericStrategy[PhysicalPlan]]
     
    -  /**
    -   * Returns a placeholder for a physical plan that executes `plan`. This placeholder will be
    -   * filled in automatically by the QueryPlanner using the other execution strategies that are
    -   * available.
    -   */
    -  protected def planLater(plan: LogicalPlan): PhysicalPlan = this.plan(plan).next()
    -
       def plan(plan: LogicalPlan): Iterator[PhysicalPlan] = {
         // Obviously a lot to do here still...
    -    val iter = strategies.view.flatMap(_(plan)).toIterator
    -    assert(iter.hasNext, s"No plan for $plan")
    -    iter
    +    val iter = strategies.iterator.flatMap(_(plan)).flatMap { physicalPlan =>
    +      val placeholders = collectPlaceholders(physicalPlan)
    +
    +      // Plan logical plan marked as [[planLater]] and replace placeholders
    +      placeholders.iterator.foldLeft(Iterator(physicalPlan)) {
    +        case (physicalPlans, (placeholder, logicalPlan)) =>
    +          // Plan the logical plan for the placeholder
    +          val children = this.plan(logicalPlan)
    +          physicalPlans.flatMap { physicalPlan =>
    +            // Replace the placeholder by the child plans
    +            children.map { child =>
    +              physicalPlan.transformUp {
    +                case p if p == placeholder => child
    +              }
    +            }
    +          }
    +      }
    +    }
    +    val pruned = prunePlans(iter)
    +    assert(pruned.hasNext, s"No plan for $plan")
    +    pruned
       }
    +
    +  /** Collects placeholders marked as [[planLater]] by strategy and its [[LogicalPlan]]s */
    +  protected def collectPlaceholders(plan: PhysicalPlan): Seq[(PhysicalPlan, LogicalPlan)]
    +
    +  /** Prunes bad plans to prevent from combinatorial explosion. */
    --- End diff --
    
    NIT: remove `...from...`


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-6320][SQL] Move planLater method into G...

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

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

    https://github.com/apache/spark/pull/13147#discussion_r65107726
  
    --- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/SparkPlanner.scala ---
    @@ -42,6 +43,18 @@ class SparkPlanner(
           InMemoryScans ::
           BasicOperators :: Nil)
     
    +  override protected def collectPlaceholders(plan: SparkPlan): Seq[(SparkPlan, LogicalPlan)] = {
    +    plan.collect { case placeholder @ PlanLater(logicalPlan) =>
    --- End diff --
    
    Nit: Style place `case` on the next liner (can be a one-liner).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-6320][SQL] Move planLater method into G...

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

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

    https://github.com/apache/spark/pull/13147#discussion_r63567938
  
    --- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/planning/QueryPlanner.scala ---
    @@ -27,6 +27,16 @@ import org.apache.spark.sql.catalyst.trees.TreeNode
      * empty list should be returned.
      */
     abstract class GenericStrategy[PhysicalPlan <: TreeNode[PhysicalPlan]] extends Logging {
    +
    +  def planner: QueryPlanner[PhysicalPlan]
    +
    +  /**
    +   * Returns a placeholder for a physical plan that executes `plan`. This placeholder will be
    +   * filled in automatically by the QueryPlanner using the other execution strategies that are
    +   * available.
    +   */
    +  protected def planLater(plan: LogicalPlan): PhysicalPlan = planner.plan(plan).next()
    --- End diff --
    
    Calling `next()` here has always been a hack (there used to be a TODO).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [WIP][SPARK-6320][SQL] Move planLater method i...

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

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

    https://github.com/apache/spark/pull/13147#discussion_r64981719
  
    --- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/planning/QueryPlanner.scala ---
    @@ -47,17 +55,27 @@ abstract class QueryPlanner[PhysicalPlan <: TreeNode[PhysicalPlan]] {
       /** A list of execution strategies that can be used by the planner */
       def strategies: Seq[GenericStrategy[PhysicalPlan]]
     
    -  /**
    -   * Returns a placeholder for a physical plan that executes `plan`. This placeholder will be
    -   * filled in automatically by the QueryPlanner using the other execution strategies that are
    -   * available.
    -   */
    -  protected def planLater(plan: LogicalPlan): PhysicalPlan = this.plan(plan).next()
    -
       def plan(plan: LogicalPlan): Iterator[PhysicalPlan] = {
         // Obviously a lot to do here still...
    -    val iter = strategies.view.flatMap(_(plan)).toIterator
    +    val iter = strategies.view.flatMap(_(plan)).toIterator.flatMap { physicalPlan =>
    +      val placeholders = collectPlaceholders(physicalPlan)
    +
    +      (Iterator(physicalPlan) /: placeholders.toIterator) {
    +        case (physicalPlans, (placeholder, logicalPlan)) =>
    +          val children = this.plan(logicalPlan)
    +          physicalPlans.flatMap { physicalPlan =>
    +            children.map { child =>
    +              physicalPlan.transformUp {
    +                case `placeholder` => child
    +              }
    +            }
    +          }
    +      }
    +    }
    +    // TODO: We will need to prune bad plans to prevent from combinatorial explosion.
    --- End diff --
    
    We are not safe now if some strategy returns two or more plans.
    Should we take the first plan here for now?
    (I will introduce `prune` method and implement it in `SparkPlanner`.)


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [WIP][SPARK-6320][SQL] Move planLater method i...

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

Github user ueshin commented on the pull request:

    https://github.com/apache/spark/pull/13147#issuecomment-222143950
  
    @marmbrus Thank you for the design sketch.
    I'd like you to see my approach I was thinking after my first commit.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark issue #13147: [SPARK-6320][SQL] Move planLater method into GenericStra...

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

Github user ueshin commented on the issue:

    https://github.com/apache/spark/pull/13147
  
    @marmbrus Do you have any other thoughts on this?
    If so, let me know them and why don't we merge the minimal version same as for `branch-2.0` into `master` for now?
    I think the API difference between `master` and `branch-2.0` for a long time is not desirable.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-6320][SQL] Move planLater method into G...

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

Github user AmplabJenkins commented on the pull request:

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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark issue #13147: [SPARK-6320][SQL] Move planLater method into GenericStra...

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

Github user SparkQA commented on the issue:

    https://github.com/apache/spark/pull/13147
  
    **[Test build #60207 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/60207/consoleFull)** for PR 13147 at commit [`254381d`](https://github.com/apache/spark/commit/254381d245cabf3cbad57f7ab06eec155ae79d96).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark issue #13147: [SPARK-6320][SQL] Move planLater method into GenericStra...

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

Github user marmbrus commented on the issue:

    https://github.com/apache/spark/pull/13147
  
    LGTM, I'm going to merge this into master after it passes tests once more (just want to check to make sure its not stale)


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [WIP][SPARK-6320][SQL] Move planLater method i...

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

Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/13147#issuecomment-222285954
  
    Test FAILed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/59545/
    Test FAILed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-6320][SQL] Move planLater method into G...

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

Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/13147#issuecomment-219693076
  
    Merged build finished. Test PASSed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-6320][SQL] Move planLater method into GenericStra...

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

Github user ueshin commented on the pull request:

    https://github.com/apache/spark/pull/13147
  
    @marmbrus How about 85d412a and 0dccae8f (and `SparkPlannerSuite`?) for the minimal version of this.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [WIP][SPARK-6320][SQL] Move planLater method i...

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

Github user AmplabJenkins commented on the pull request:

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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [WIP][SPARK-6320][SQL] Move planLater method i...

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

Github user AmplabJenkins commented on the pull request:

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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-6320][SQL] Move planLater method into G...

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

Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/13147#issuecomment-222573444
  
    **[Test build #59625 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/59625/consoleFull)** for PR 13147 at commit [`d35d550`](https://github.com/apache/spark/commit/d35d55069d592f1d315607e2dc43a5425d436067).
     * This patch passes all tests.
     * This patch merges cleanly.
     * This patch adds no public classes.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [WIP][SPARK-6320][SQL] Move planLater method i...

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

Github user AmplabJenkins commented on the pull request:

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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-6320][SQL] Move planLater method into G...

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

Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/13147#issuecomment-222585527
  
    Merged build finished. Test PASSed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-6320][SQL] Move planLater method into G...

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

Github user AmplabJenkins commented on the pull request:

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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-6320][SQL] Move planLater method into G...

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

Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/13147#issuecomment-222409808
  
    **[Test build #59604 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/59604/consoleFull)** for PR 13147 at commit [`a61584b`](https://github.com/apache/spark/commit/a61584b9dfb8360ee4af66ca7a6edda9bbe22a78).
     * This patch passes all tests.
     * This patch merges cleanly.
     * This patch adds no public classes.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-6320][SQL] Move planLater method into G...

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

Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/13147#issuecomment-222567466
  
    **[Test build #59625 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/59625/consoleFull)** for PR 13147 at commit [`d35d550`](https://github.com/apache/spark/commit/d35d55069d592f1d315607e2dc43a5425d436067).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request #13147: [SPARK-6320][SQL] Move planLater method into Gene...

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

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

    https://github.com/apache/spark/pull/13147#discussion_r90560927
  
    --- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/planning/QueryPlanner.scala ---
    @@ -27,6 +27,14 @@ import org.apache.spark.sql.catalyst.trees.TreeNode
      * empty list should be returned.
      */
     abstract class GenericStrategy[PhysicalPlan <: TreeNode[PhysicalPlan]] extends Logging {
    +
    +  /**
    +   * Returns a placeholder for a physical plan that executes `plan`. This placeholder will be
    +   * filled in automatically by the QueryPlanner using the other execution strategies that are
    +   * available.
    +   */
    +  protected def planLater(plan: LogicalPlan): PhysicalPlan
    +
       def apply(plan: LogicalPlan): Seq[PhysicalPlan]
     }
     
    --- End diff --
    
    Fine to include it there!  I guess I need to review that :)


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [WIP][SPARK-6320][SQL] Move planLater method i...

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

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

    https://github.com/apache/spark/pull/13147#discussion_r64972060
  
    --- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/planning/QueryPlanner.scala ---
    @@ -47,17 +55,27 @@ abstract class QueryPlanner[PhysicalPlan <: TreeNode[PhysicalPlan]] {
       /** A list of execution strategies that can be used by the planner */
       def strategies: Seq[GenericStrategy[PhysicalPlan]]
     
    -  /**
    -   * Returns a placeholder for a physical plan that executes `plan`. This placeholder will be
    -   * filled in automatically by the QueryPlanner using the other execution strategies that are
    -   * available.
    -   */
    -  protected def planLater(plan: LogicalPlan): PhysicalPlan = this.plan(plan).next()
    -
       def plan(plan: LogicalPlan): Iterator[PhysicalPlan] = {
         // Obviously a lot to do here still...
    -    val iter = strategies.view.flatMap(_(plan)).toIterator
    +    val iter = strategies.view.flatMap(_(plan)).toIterator.flatMap { physicalPlan =>
    +      val placeholders = collectPlaceholders(physicalPlan)
    +
    +      (Iterator(physicalPlan) /: placeholders.toIterator) {
    --- End diff --
    
    Style: I'm pretty sure @rxin will cry if we use `/:` 😄 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [WIP][SPARK-6320][SQL] Move planLater method i...

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

Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/13147#issuecomment-222144767
  
    **[Test build #59491 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/59491/consoleFull)** for PR 13147 at commit [`42c18ab`](https://github.com/apache/spark/commit/42c18ab847ab8b1e112a3a3129bd7483b21a486c).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [WIP][SPARK-6320][SQL] Move planLater method i...

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

Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/13147#issuecomment-222094433
  
    **[Test build #59474 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/59474/consoleFull)** for PR 13147 at commit [`85d412a`](https://github.com/apache/spark/commit/85d412aa2431333ecf0ba8de270777b0a994fcb3).
     * This patch passes all tests.
     * This patch merges cleanly.
     * This patch adds the following public classes _(experimental)_:
      * `abstract class SparkStrategy extends GenericStrategy[SparkPlan] `


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [WIP][SPARK-6320][SQL] Move planLater method i...

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

Github user AmplabJenkins commented on the pull request:

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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [WIP][SPARK-6320][SQL] Move planLater method i...

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

Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/13147#issuecomment-222337092
  
    **[Test build #59573 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/59573/consoleFull)** for PR 13147 at commit [`7b3f4a1`](https://github.com/apache/spark/commit/7b3f4a1ac99a3e4ab2648288e3a0b71a7e2defd5).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark issue #13147: [SPARK-6320][SQL] Move planLater method into GenericStra...

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

Github user ueshin commented on the issue:

    https://github.com/apache/spark/pull/13147
  
    @marmbrus Thank you for merging this!


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-6320][SQL] Move planLater method into GenericStra...

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

Github user ueshin commented on the pull request:

    https://github.com/apache/spark/pull/13147
  
    @marmbrus I sent a PR #13426 as a test for the minimal version of this for `branch-2.0`.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [WIP][SPARK-6320][SQL] Move planLater method i...

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

Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/13147#issuecomment-222124696
  
    **[Test build #59484 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/59484/consoleFull)** for PR 13147 at commit [`0dccae8`](https://github.com/apache/spark/commit/0dccae8fe0c07d085880ea5feec6fafd9848dbc4).
     * This patch passes all tests.
     * This patch merges cleanly.
     * This patch adds no public classes.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-6320][SQL] Move planLater method into G...

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

Github user ueshin commented on the pull request:

    https://github.com/apache/spark/pull/13147#issuecomment-219938062
  
    @marmbrus Thank you for your comments.
    I see but apparently we are lacking the `planLater` functionality for extra strategies.
    Could you give me the earlier prototype snippet or the pointer to it if you still have it, as a code base of this issue?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [WIP][SPARK-6320][SQL] Move planLater method i...

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

Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/13147#issuecomment-222294640
  
    Merged build finished. Test PASSed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [WIP][SPARK-6320][SQL] Move planLater method i...

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

Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/13147#issuecomment-222294606
  
    **[Test build #59553 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/59553/consoleFull)** for PR 13147 at commit [`e5ee2c2`](https://github.com/apache/spark/commit/e5ee2c2b4c7351df42d9096b95f4ed3759f540d7).
     * This patch passes all tests.
     * This patch merges cleanly.
     * This patch adds the following public classes _(experimental)_:
      * `class SparkPlannerSuite extends SharedSQLContext `


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-6320][SQL] Move planLater method into G...

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

Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/13147#issuecomment-222585447
  
    **[Test build #59627 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/59627/consoleFull)** for PR 13147 at commit [`254381d`](https://github.com/apache/spark/commit/254381d245cabf3cbad57f7ab06eec155ae79d96).
     * This patch passes all tests.
     * This patch merges cleanly.
     * This patch adds no public classes.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [WIP][SPARK-6320][SQL] Move planLater method i...

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

Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/13147#issuecomment-222110844
  
    **[Test build #59484 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/59484/consoleFull)** for PR 13147 at commit [`0dccae8`](https://github.com/apache/spark/commit/0dccae8fe0c07d085880ea5feec6fafd9848dbc4).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-6320][SQL] Move planLater method into G...

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

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

    https://github.com/apache/spark/pull/13147#discussion_r63567867
  
    --- Diff: sql/core/src/main/scala/org/apache/spark/sql/ExperimentalMethods.scala ---
    @@ -42,7 +43,7 @@ class ExperimentalMethods private[sql]() {
        * @since 1.3.0
        */
       @Experimental
    -  @volatile var extraStrategies: Seq[Strategy] = Nil
    +  @volatile var extraStrategies: Seq[SparkPlanner => Strategy] = Nil
    --- End diff --
    
    I'm not sure if we want to change this interface and this feels a little hacky to me.  It seems like `planLater` might actually be the problem.  In an earlier prototype we actually use a node that indicated "planLater" and looped it back through the planner instead of doing it all eagerly.  This let us do plan space exploration, but I'd have to spend some time thinking about how exactly that could work.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [WIP][SPARK-6320][SQL] Move planLater method i...

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

Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/13147#issuecomment-222291941
  
    **[Test build #59553 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/59553/consoleFull)** for PR 13147 at commit [`e5ee2c2`](https://github.com/apache/spark/commit/e5ee2c2b4c7351df42d9096b95f4ed3759f540d7).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-6320][SQL] Move planLater method into G...

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

Github user AmplabJenkins commented on the pull request:

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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [WIP][SPARK-6320][SQL] Move planLater method i...

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

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

    https://github.com/apache/spark/pull/13147#discussion_r64982752
  
    --- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/planning/QueryPlanner.scala ---
    @@ -47,17 +55,27 @@ abstract class QueryPlanner[PhysicalPlan <: TreeNode[PhysicalPlan]] {
       /** A list of execution strategies that can be used by the planner */
       def strategies: Seq[GenericStrategy[PhysicalPlan]]
     
    -  /**
    -   * Returns a placeholder for a physical plan that executes `plan`. This placeholder will be
    -   * filled in automatically by the QueryPlanner using the other execution strategies that are
    -   * available.
    -   */
    -  protected def planLater(plan: LogicalPlan): PhysicalPlan = this.plan(plan).next()
    -
       def plan(plan: LogicalPlan): Iterator[PhysicalPlan] = {
         // Obviously a lot to do here still...
    -    val iter = strategies.view.flatMap(_(plan)).toIterator
    +    val iter = strategies.view.flatMap(_(plan)).toIterator.flatMap { physicalPlan =>
    +      val placeholders = collectPlaceholders(physicalPlan)
    +
    +      (Iterator(physicalPlan) /: placeholders.toIterator) {
    +        case (physicalPlans, (placeholder, logicalPlan)) =>
    +          val children = this.plan(logicalPlan)
    +          physicalPlans.flatMap { physicalPlan =>
    +            children.map { child =>
    +              physicalPlan.transformUp {
    +                case `placeholder` => child
    +              }
    +            }
    +          }
    +      }
    +    }
    +    // TODO: We will need to prune bad plans to prevent from combinatorial explosion.
    --- End diff --
    
    Ah, I'm sorry, you are right because the planner returns plans as Iterator so the any other branches should not be processed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark issue #13147: [SPARK-6320][SQL] Move planLater method into GenericStra...

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

Github user AmplabJenkins commented on the issue:

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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [WIP][SPARK-6320][SQL] Move planLater method i...

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

Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/13147#issuecomment-222341221
  
    **[Test build #59576 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/59576/consoleFull)** for PR 13147 at commit [`fbc9a9f`](https://github.com/apache/spark/commit/fbc9a9f08b65e1210e9db9eb92a9769bcfb693a9).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [WIP][SPARK-6320][SQL] Move planLater method i...

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

Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/13147#issuecomment-222285934
  
    **[Test build #59545 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/59545/consoleFull)** for PR 13147 at commit [`a3524cd`](https://github.com/apache/spark/commit/a3524cd220f840839a7f40c570fe2c5da4c7705b).
     * This patch **fails Spark unit tests**.
     * This patch merges cleanly.
     * This patch adds no public classes.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [WIP][SPARK-6320][SQL] Move planLater method i...

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

Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/13147#issuecomment-222094657
  
    Merged build finished. Test PASSed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-6320][SQL] Move planLater method into G...

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

Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/13147#issuecomment-222573545
  
    Merged build finished. Test PASSed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark issue #13147: [SPARK-6320][SQL] Move planLater method into GenericStra...

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

Github user AmplabJenkins commented on the issue:

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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-6320][SQL] Move planLater method into G...

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

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

    https://github.com/apache/spark/pull/13147#discussion_r65107597
  
    --- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/planning/QueryPlanner.scala ---
    @@ -47,17 +55,34 @@ abstract class QueryPlanner[PhysicalPlan <: TreeNode[PhysicalPlan]] {
       /** A list of execution strategies that can be used by the planner */
       def strategies: Seq[GenericStrategy[PhysicalPlan]]
     
    -  /**
    -   * Returns a placeholder for a physical plan that executes `plan`. This placeholder will be
    -   * filled in automatically by the QueryPlanner using the other execution strategies that are
    -   * available.
    -   */
    -  protected def planLater(plan: LogicalPlan): PhysicalPlan = this.plan(plan).next()
    -
       def plan(plan: LogicalPlan): Iterator[PhysicalPlan] = {
         // Obviously a lot to do here still...
    -    val iter = strategies.view.flatMap(_(plan)).toIterator
    -    assert(iter.hasNext, s"No plan for $plan")
    -    iter
    +    val iter = strategies.iterator.flatMap(_(plan)).flatMap { physicalPlan =>
    --- End diff --
    
    The could below is really terse. Could you break it up a little, and add some documentation?
    
    Start with splitting the two flatMaps on the current line.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [WIP][SPARK-6320][SQL] Move planLater method i...

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

Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/13147#issuecomment-222339329
  
    **[Test build #59573 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/59573/consoleFull)** for PR 13147 at commit [`7b3f4a1`](https://github.com/apache/spark/commit/7b3f4a1ac99a3e4ab2648288e3a0b71a7e2defd5).
     * This patch passes all tests.
     * This patch merges cleanly.
     * This patch adds no public classes.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [SPARK-6320][SQL] Move planLater method into G...

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

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

    https://github.com/apache/spark/pull/13147#discussion_r65111434
  
    --- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/planning/QueryPlanner.scala ---
    @@ -47,17 +55,34 @@ abstract class QueryPlanner[PhysicalPlan <: TreeNode[PhysicalPlan]] {
       /** A list of execution strategies that can be used by the planner */
       def strategies: Seq[GenericStrategy[PhysicalPlan]]
     
    -  /**
    -   * Returns a placeholder for a physical plan that executes `plan`. This placeholder will be
    -   * filled in automatically by the QueryPlanner using the other execution strategies that are
    -   * available.
    -   */
    -  protected def planLater(plan: LogicalPlan): PhysicalPlan = this.plan(plan).next()
    -
       def plan(plan: LogicalPlan): Iterator[PhysicalPlan] = {
         // Obviously a lot to do here still...
    -    val iter = strategies.view.flatMap(_(plan)).toIterator
    -    assert(iter.hasNext, s"No plan for $plan")
    -    iter
    +    val iter = strategies.iterator.flatMap(_(plan)).flatMap { physicalPlan =>
    --- End diff --
    
    @hvanhovell Thank you for your review.
    I modified the method.
    Please check it again.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [WIP][SPARK-6320][SQL] Move planLater method i...

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

Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/13147#issuecomment-222343015
  
    Merged build finished. Test PASSed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request: [WIP][SPARK-6320][SQL] Move planLater method i...

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

Github user AmplabJenkins commented on the pull request:

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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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

[GitHub] spark pull request #13147: [SPARK-6320][SQL] Move planLater method into Gene...

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

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

    https://github.com/apache/spark/pull/13147#discussion_r90317826
  
    --- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/planning/QueryPlanner.scala ---
    @@ -27,6 +27,14 @@ import org.apache.spark.sql.catalyst.trees.TreeNode
      * empty list should be returned.
      */
     abstract class GenericStrategy[PhysicalPlan <: TreeNode[PhysicalPlan]] extends Logging {
    +
    +  /**
    +   * Returns a placeholder for a physical plan that executes `plan`. This placeholder will be
    +   * filled in automatically by the QueryPlanner using the other execution strategies that are
    +   * available.
    +   */
    +  protected def planLater(plan: LogicalPlan): PhysicalPlan
    +
       def apply(plan: LogicalPlan): Seq[PhysicalPlan]
     }
     
    --- End diff --
    
    I just noticed, there is a TODO below in the comment that I think we can remove now :)
    
    >   * TODO: RIGHT NOW ONLY ONE PLAN IS RETURNED EVER...  PLAN SPACE EXPLORATION WILL BE IMPLEMENTED LATER.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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