Skip to content

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

Closed
ueshin wants to merge 4 commits intoapache:branch-2.0from
ueshin:issues/SPARK-6320_2.0
Closed

[SPARK-6320][SQL] Move planLater method into GenericStrategy.#13426
ueshin wants to merge 4 commits intoapache:branch-2.0from
ueshin:issues/SPARK-6320_2.0

Conversation

@ueshin
Copy link
Member

@ueshin ueshin commented May 31, 2016

What changes were proposed in this pull request?

This PR is the minimal version of #13147 for branch-2.0.

How was this patch tested?

Picked SparkPlannerSuite from #13147.

@ueshin ueshin mentioned this pull request May 31, 2016
Closed
@marmbrus
Copy link
Contributor

marmbrus commented May 31, 2016

@rxin thoughts on including this in 2.0? Seems safe to me.

rxin
rxin reviewed May 31, 2016
View reviewed changes
sql/core/src/main/scala/org/apache/spark/sql/execution/SparkStrategies.scala
@DeveloperApi
abstract class SparkStrategy extends GenericStrategy[SparkPlan] {

override protected def planLater(plan: LogicalPlan): SparkPlan = PlanLater(plan)
Copy link
Contributor

@rxin rxin May 31, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can anybody implement this without planlater being public?

Copy link
Member Author

@ueshin ueshin Jun 1, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, because it's a protected method.
I didn't understand your true meaning but do you think it should be more open like public or closer like private[sql] or final here?

Copy link
Contributor

@rxin rxin Jun 1, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was referring to case class PlanLater

Copy link
Member Author

@ueshin ueshin Jun 1, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see.
Yes, we can implement this without PlanLater being public.

@rxin
Copy link
Contributor

rxin commented May 31, 2016

Looks fine.

@SparkQA
Copy link

SparkQA commented Jun 1, 2016

Test build #59693 has finished for PR 13426 at commit 4c7ebb6.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@marmbrus
Copy link
Contributor

marmbrus commented Jun 1, 2016

I'm going to go ahead and merge this so we get the API changes in before we cut any RCs. Thanks!

asfgit pushed a commit that referenced this pull request Jun 1, 2016
@ueshin @marmbrus
[SPARK-6320][SQL] Move planLater method into GenericStrategy.
71e8aae 
## What changes were proposed in this pull request?

This PR is the minimal version of #13147 for `branch-2.0`.

## How was this patch tested?

Picked `SparkPlannerSuite` from #13147.

Author: Takuya UESHIN <ueshin@happy-camper.st>

Closes #13426 from ueshin/issues/SPARK-6320_2.0.
@marmbrus
Copy link
Contributor

marmbrus commented Jun 1, 2016

@ueshin can you close this (PRs not against master don't auto close)

@ueshin
Copy link
Member Author

ueshin commented Jun 2, 2016

Sure.
Thank you for merging this!

@ueshin ueshin closed this Jun 2, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@ueshin @marmbrus @rxin @SparkQA