Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SPARK-10612][SQL] Add prepare to LocalNode. #8761

Closed
wants to merge 1 commit into from

Conversation

rxin
Copy link
Contributor

@rxin rxin commented Sep 15, 2015

The idea is that we should separate the function call that does memory reservation (i.e. prepare) from the function call that consumes the input (e.g. open()), so all operators can be a chance to reserve memory before they are all consumed.

@SparkQA
Copy link

SparkQA commented Sep 15, 2015

Test build #42475 has finished for PR 8761 at commit f2d4a1d.

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

@@ -27,6 +27,10 @@ case class ConvertToSafeNode(conf: SQLConf, child: LocalNode) extends UnaryLocal

private[this] var convertToSafe: Projection = _

override def prepare(): Unit = {
child.prepare()
Copy link
Member

Choose a reason for hiding this comment

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

Why not implement this one in UnaryLocalNode? Then we don't need to duplicate them in all subclasses of ``UnaryLocalNode`.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually then we could just implement it in LocalNode itself since it can traverse the children.

@rxin
Copy link
Contributor Author

rxin commented Sep 15, 2015

OK updated - I put everything in LocalNode.

I was initially thinking it might be better to be more explicit - but it seems vast majority of the operators don't need to do anything.

@SparkQA
Copy link

SparkQA commented Sep 15, 2015

Test build #42493 has finished for PR 8761 at commit 90f7943.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@andrewor14
Copy link
Contributor

retest this please

@andrewor14
Copy link
Contributor

Actually this patch is essentially a no-op so I'm just going to merge it. This is going to master.

@asfgit asfgit closed this in a63cdc7 Sep 15, 2015
@SparkQA
Copy link

SparkQA commented Sep 16, 2015

Test build #42512 has finished for PR 8761 at commit 90f7943.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants