Skip to content

Conversation

@andrewor14
Copy link
Contributor

What changes were proposed in this pull request?

Users should use the builder pattern instead.

How was this patch tested?

Jenks.

@SparkQA
Copy link

SparkQA commented May 3, 2016

Test build #57651 has finished for PR 12873 at commit 5f8c9bf.

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

@andrewor14
Copy link
Contributor Author

retest this please

@rxin
Copy link
Contributor

rxin commented May 3, 2016

LGTM pending Jenkins.

@SparkQA
Copy link

SparkQA commented May 3, 2016

Test build #57655 has finished for PR 12873 at commit 5f8c9bf.

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

extends Serializable with Logging { self =>

def this(sc: SparkContext) {
private[sql] def this(sc: SparkContext) {
Copy link
Contributor

Choose a reason for hiding this comment

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

actually is it possible to just make this private?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we use this in SQLContext

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe we should change that and have sqlcontext always getting it from sparksession tiself?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah I think the problem is that in getOrCreate right now we go the other way, i.e. getting the session from the context

@rxin
Copy link
Contributor

rxin commented May 3, 2016

Merging in master/branch-2.0.

asfgit pushed a commit that referenced this pull request May 3, 2016
## What changes were proposed in this pull request?

Users should use the builder pattern instead.

## How was this patch tested?

Jenks.

Author: Andrew Or <[email protected]>

Closes #12873 from andrewor14/spark-session-constructor.

(cherry picked from commit 588cac4)
Signed-off-by: Reynold Xin <[email protected]>
@asfgit asfgit closed this in 588cac4 May 3, 2016
@andrewor14 andrewor14 deleted the spark-session-constructor branch May 3, 2016 21:21
def createSparkSession(): SparkSession = {
if (SparkSession.hiveClassesArePresent) {
sparkSession = SparkSession.withHiveSupport(sparkContext)
sparkSession = SparkSession.builder.enableHiveSupport().getOrCreate()
Copy link
Contributor

Choose a reason for hiding this comment

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

They are not identical. The previous one will create a new spark session anyway, but the new one may not create a new spark session if sparkContext already exists. Then we can't guarantee the returned spark session supports hive.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch

asfgit pushed a commit that referenced this pull request May 4, 2016
…rkContext from it

## What changes were proposed in this pull request?

see #12873 (comment). The problem is, if we create `SparkContext` first and then call `SparkSession.builder.enableHiveSupport().getOrCreate()`, we will reuse the existing `SparkContext` and the hive flag won't be set.

## How was this patch tested?

verified it locally.

Author: Wenchen Fan <[email protected]>

Closes #12890 from cloud-fan/repl.
asfgit pushed a commit that referenced this pull request May 4, 2016
…rkContext from it

## What changes were proposed in this pull request?

see #12873 (comment). The problem is, if we create `SparkContext` first and then call `SparkSession.builder.enableHiveSupport().getOrCreate()`, we will reuse the existing `SparkContext` and the hive flag won't be set.

## How was this patch tested?

verified it locally.

Author: Wenchen Fan <[email protected]>

Closes #12890 from cloud-fan/repl.
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