Skip to content

Conversation

@zjffdu
Copy link
Contributor

@zjffdu zjffdu commented Feb 13, 2017

What changes were proposed in this pull request?

SPARK-15236 do this for scala shell, this ticket is for pyspark shell. This is not only for pyspark itself, but can also benefit downstream project like livy which use shell.py for its interactive session. For now, livy has no control of whether enable hive or not.

How was this patch tested?

I didn't find a way to add test for it. Just manually test it.
Run bin/pyspark --master local --conf spark.sql.catalogImplementation=in-memory and verify hive is not enabled.

@SparkQA
Copy link

SparkQA commented Feb 13, 2017

Test build #72796 has finished for PR 16906 at commit 431bcf8.

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

@zjffdu
Copy link
Contributor Author

zjffdu commented Feb 13, 2017

@holdenk Please help review

@holdenk
Copy link
Contributor

holdenk commented Feb 13, 2017

Let me take a look tomorrow.

@holdenk
Copy link
Contributor

holdenk commented Feb 13, 2017

Ah interesting, thanks for working on this. I've been working on a JIRA which also requires "stand alone" style testing, I'll make the (WIP) PR for that and maybe you can take a look at it. Perhaps we could generalize that one a bit and make something for testing things like this (provided the overhead isn't too high).

@felixcheung
Copy link
Member

I think we need to lowercase the conf value as in #16907

@zjffdu
Copy link
Contributor Author

zjffdu commented Mar 21, 2017

Yeah, make sense. Fixed it.

@SparkQA
Copy link

SparkQA commented Mar 21, 2017

Test build #74944 has finished for PR 16906 at commit ba10538.

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

@SparkQA
Copy link

SparkQA commented Mar 31, 2017

Test build #75428 has finished for PR 16906 at commit 5322189.

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

Copy link
Member

@felixcheung felixcheung left a comment

Choose a reason for hiding this comment

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

LGTM, @holdenk do you have any comment on this?

@holdenk
Copy link
Contributor

holdenk commented Apr 3, 2017

I think this looks reasonable, although it would maybe make sense to add a warning if the user has explicitly requested hive support and we are falling through to non-hive support (e.g. in the except side of the try block).

@felixcheung
Copy link
Member

+1 on that, we do have the log on the R side.

@SparkQA
Copy link

SparkQA commented Apr 6, 2017

Test build #75562 has finished for PR 16906 at commit 63942c1.

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

@zjffdu
Copy link
Contributor Author

zjffdu commented Apr 6, 2017

Thanks @holdenk @felixcheung for review, I just added the warning log

@zjffdu
Copy link
Contributor Author

zjffdu commented Apr 11, 2017

Kindly ping @holdenk

@holdenk
Copy link
Contributor

holdenk commented Apr 12, 2017

LGTM, thanks for working on this @zjffdu

@asfgit asfgit closed this in 99a9473 Apr 12, 2017
@holdenk
Copy link
Contributor

holdenk commented Apr 12, 2017

Merged to master

peter-toth pushed a commit to peter-toth/spark that referenced this pull request Oct 6, 2018
## What changes were proposed in this pull request?

SPARK-15236 do this for scala shell, this ticket is for pyspark shell. This is not only for pyspark itself, but can also benefit downstream project like livy which use shell.py for its interactive session. For now, livy has no control of whether enable hive or not.

## How was this patch tested?

I didn't find a way to add test for it. Just manually test it.
Run `bin/pyspark --master local --conf spark.sql.catalogImplementation=in-memory` and verify hive is not enabled.

Author: Jeff Zhang <[email protected]>

Closes apache#16906 from zjffdu/SPARK-19570.
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