[SPARK-19572][SPARKR] Allow to disable hive in sparkR shell by zjffdu · Pull Request #16907 · apache/spark

zjffdu · 2017-02-13T05:54:17Z

What changes were proposed in this pull request?

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

How was this patch tested?

Tested it manually, run bin/sparkR --master local --conf spark.sql.catalogImplementation=in-memory and verify hive is not enabled.

SparkQA · 2017-02-13T05:58:37Z

Test build #72801 has started for PR 16907 at commit 8329be6.

SparkQA · 2017-02-13T10:46:41Z

Test build #72810 has finished for PR 16907 at commit 9a3aea6.

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

zjffdu · 2017-02-13T11:24:22Z

@felixcheung Please help review.

felixcheung · 2017-02-13T18:03:55Z

 } else {
- if (enableHiveSupport) {
+ if (enableHiveSupport
+ && jsc.sc.conf.get(CATALOG_IMPLEMENTATION.key, "hive") == "hive") {


~~I don't think we should check for this for this message~~
I think we should have a message for hive support being turned off by conf. Otherwise it will be confusing for someone running sparkR.session(enableHiveSupport = TRUE) and didn't get it.

you need to toLowerCase like here https://github.com/apache/spark/blob/master/repl/scala-2.11/src/main/scala/org/apache/spark/repl/Main.scala#L91

felixcheung · 2017-02-13T18:05:32Z

We should discuss other uses outside of sparkR shell - shell.R is not intended to be an API for reuse outside of the sparkR shell - we have other assumptions in the code that could be easily broken that way.

felixcheung · 2017-02-13T18:09:10Z

 } else {
- if (enableHiveSupport) {
+ if (enableHiveSupport
+ && jsc.sc.conf.get(CATALOG_IMPLEMENTATION.key, "hive") == "hive") {


you need to toLowerCase like here https://github.com/apache/spark/blob/master/repl/scala-2.11/src/main/scala/org/apache/spark/repl/Main.scala#L91

zjffdu · 2017-02-14T02:03:43Z

Address the comments. @felixcheung, correct, shell.R is not supposed to be used outside. This ticket is mainly for disabling hive in sparkR shell, sparkR batch mode already support this feature.

felixcheung · 2017-02-14T03:25:07Z

I'd be specific to say instead of Hive is disabled via ${CATALOG_IMPLEMENTATION.key}
say ${CATALOG_IMPLEMENTATION.key} is not set to "hive";

Ping @felixcheung, message is updated.

test failed? #16907 (comment)

SparkQA · 2017-02-14T04:30:24Z

Test build #72842 has finished for PR 16907 at commit 6839c02.

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

SparkQA · 2017-02-15T04:19:20Z

Test build #72915 has finished for PR 16907 at commit ee20be3.

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

zjffdu · 2017-02-24T08:33:10Z

Seems a flaky test, let me trigger the build

SparkQA · 2017-02-24T10:35:37Z

Test build #73416 has finished for PR 16907 at commit 68b5823.

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

felixcheung

LGTM.
You want this in branch-2.1 and master, yes?

zjffdu · 2017-02-25T00:57:04Z

Yeah, it would be nice to be merged into 2.1 as well. Thanks

felixcheung · 2017-02-28T18:15:16Z

+ && jsc.sc.conf.get(CATALOG_IMPLEMENTATION.key, "hive").toLowerCase == "hive") {
 logWarning("SparkR: enableHiveSupport is requested for SparkSession but " +
- "Spark is not built with Hive; falling back to without Hive support.")
+ s"Spark is not built with Hive or ${CATALOG_IMPLEMENTATION.key} is not set to 'hive', " +


actually, I notice this when reviewing before merge, I think this check is not matching with the message - in this case we already know CATALOG_IMPLEMENTATION == "hive"

Good catch, I updated the if condition to match the message.

SparkQA · 2017-03-01T04:19:30Z

Test build #73642 has finished for PR 16907 at commit 89a8752.

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

## What changes were proposed in this pull request? SPARK-15236 do this for scala shell, this ticket is for sparkR shell. This is not only for sparkR itself, but can also benefit downstream project like livy which use shell.R for its interactive session. For now, livy has no control of whether enable hive or not. ## How was this patch tested? Tested it manually, run `bin/sparkR --master local --conf spark.sql.catalogImplementation=in-memory` and verify hive is not enabled. Author: Jeff Zhang <zjffdu@apache.org> Closes#16907 from zjffdu/SPARK-19572. (cherry picked from commit 7315880) Signed-off-by: Felix Cheung <felixcheung@apache.org>

felixcheung · 2017-03-01T06:22:20Z

merged to master and branch-2.1

zjffdu changed the title ~~[SPARK-19582][SPARKR] Allow to disable hive in sparkR shell~~[SPARK-19572][SPARKR] Allow to disable hive in sparkR shellFeb 13, 2017

[SPARK-19572][SPARKR] Allow to disable hive in sparkR shell
9a3aea6

zjffdu force-pushed the SPARK-19572 branch from 8329be6 to 9a3aea6Compare February 13, 2017 08:20

felixcheung reviewed Feb 13, 2017
View reviewed changes

felixcheung requested changes Feb 13, 2017
View reviewed changes

zjffdu force-pushed the SPARK-19572 branch from 645e9a6 to 6839c02Compare February 14, 2017 02:04

felixcheung requested changes Feb 14, 2017
View reviewed changes

zjffdu force-pushed the SPARK-19572 branch from 6839c02 to ee20be3Compare February 15, 2017 02:30

address comments
68b5823

zjffdu force-pushed the SPARK-19572 branch from ee20be3 to 68b5823Compare February 24, 2017 08:33

felixcheung approved these changes Feb 24, 2017
View reviewed changes

felixcheung requested changes Feb 28, 2017
View reviewed changes

update the if condition
89a8752

asfgit closed this in 7315880Mar 1, 2017

felixcheung mentioned this pull request Mar 21, 2017
[SPARK-19570][PYSPARK] Allow to disable hive in pyspark shell #16906
Closed

Uh oh!

Conversation

zjffdu commented Feb 13, 2017

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

SparkQA commented Feb 13, 2017

Uh oh!

SparkQA commented Feb 13, 2017

Uh oh!

zjffdu commented Feb 13, 2017

Uh oh!

felixcheungFeb 13, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felixcheungFeb 13, 2017

Choose a reason for hiding this comment

Uh oh!

felixcheung commented Feb 13, 2017

Uh oh!

felixcheungFeb 13, 2017

Choose a reason for hiding this comment

Uh oh!

zjffdu commented Feb 14, 2017

Uh oh!

felixcheungFeb 14, 2017

Choose a reason for hiding this comment

Uh oh!

zjffduFeb 24, 2017

Choose a reason for hiding this comment

Uh oh!

felixcheungFeb 24, 2017

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Feb 14, 2017

Uh oh!

SparkQA commented Feb 15, 2017

Uh oh!

zjffdu commented Feb 24, 2017

Uh oh!

SparkQA commented Feb 24, 2017

Uh oh!

felixcheung left a comment

Choose a reason for hiding this comment

Uh oh!

zjffdu commented Feb 25, 2017

Uh oh!

felixcheungFeb 28, 2017

Choose a reason for hiding this comment

Uh oh!

zjffduMar 1, 2017

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Mar 1, 2017

Uh oh!

felixcheung commented Mar 1, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

felixcheungFeb 13, 2017•
edited
Loading