Skip to content

Conversation

@dongjoon-hyun
Copy link
Member

What changes were proposed in this pull request?

This is a python port of corresponding Scala builder pattern code. sql.py is modified as a target example case.

How was this patch tested?

Manual.

@dongjoon-hyun
Copy link
Member Author

@rxin .
This is the initial commit to confirm the direction. Could you give me some advice?

Copy link
Contributor

Choose a reason for hiding this comment

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

we should update SparkSession's doc itself to indicate how to create it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep. It's updated.

@rxin
Copy link
Contributor

rxin commented May 3, 2016

cc @davies can you take a look at the builder API?

@SparkQA
Copy link

SparkQA commented May 3, 2016

Test build #57589 has finished for PR 12860 at commit 2b55814.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • class Builder(object):

@SparkQA
Copy link

SparkQA commented May 3, 2016

Test build #57592 has finished for PR 12860 at commit 07a926b.

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

@SparkQA
Copy link

SparkQA commented May 3, 2016

Test build #57593 has finished for PR 12860 at commit 1fe7fb0.

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

@SparkQA
Copy link

SparkQA commented May 3, 2016

Test build #57602 has finished for PR 12860 at commit 356bc85.

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

Copy link
Contributor

Choose a reason for hiding this comment

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

We could create a builder here, then we can use it like this:

SparkSession.builder.master().getOrCreater()

Copy link
Contributor

Choose a reason for hiding this comment

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

nvm, we also create a Builder every time in Scala.

@dongjoon-hyun
Copy link
Member Author

Thank you for review, @davies . I'll update soon.

@dongjoon-hyun
Copy link
Member Author

@davies . I addressed two comments, but I'm not sure about the first one.
We need to change ScalaSession.Builder first if we want to change.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's weird to use SQLContext to create an SparkSession, can't we create an SparkSession directly?

Copy link
Contributor

Choose a reason for hiding this comment

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

We also use that in Scala, it's OK for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

if we just call scala's getOrCreate here then we don't need to fix this in the future

@andrewor14
Copy link
Contributor

Looks good otherwise

@SparkQA
Copy link

SparkQA commented May 3, 2016

Test build #57649 has finished for PR 12860 at commit 6cd7740.

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

@dongjoon-hyun
Copy link
Member Author

Thank you, @davies and @andrewor14 .
Ya, it's still evolving! No problem. After merging #12873 , I'll update accordingly again.

@rxin
Copy link
Contributor

rxin commented May 3, 2016

It's been merged!

@dongjoon-hyun
Copy link
Member Author

Great! Thank you, @rxin

@SparkQA
Copy link

SparkQA commented May 3, 2016

Test build #57683 has finished for PR 12860 at commit ac5bc68.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • class Builder(object):

@SparkQA
Copy link

SparkQA commented May 3, 2016

Test build #57684 has finished for PR 12860 at commit 589cba8.

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

@dongjoon-hyun
Copy link
Member Author

Hi, @davies and @andrewor14 . Now, it's updated.

  • Add stop in SparkSession
  • Update builder pattern according to the Scala versions.
    • One builder per one SparkSession.
  • Clean up docs.

For the calling Scala's .getOrCreate, I prefer to keep Python builder similar to Scala one. After updating Scala builder, I hope to update this in the same manner easily.

@andrewor14
Copy link
Contributor

Thanks, merging into master 2.0

asfgit pushed a commit that referenced this pull request May 4, 2016
… in PySpark.

## What changes were proposed in this pull request?

This is a python port of corresponding Scala builder pattern code. `sql.py` is modified as a target example case.

## How was this patch tested?

Manual.

Author: Dongjoon Hyun <[email protected]>

Closes #12860 from dongjoon-hyun/SPARK-15084.

(cherry picked from commit 0903a18)
Signed-off-by: Andrew Or <[email protected]>
@asfgit asfgit closed this in 0903a18 May 4, 2016
@dongjoon-hyun
Copy link
Member Author

Thank you, @andrewor14 !

@dongjoon-hyun dongjoon-hyun deleted the SPARK-15084 branch May 12, 2016 01:01
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.

5 participants