[SPARK-21574][SQL] Point out user to set hive config before SparkSession is initialized #18769

wangyum · 2017-07-29T10:52:56Z

What changes were proposed in this pull request?

Since Spark 2.0.0, SET hive config commands do not pass the values to HiveClient, this PR point out user to set hive config before SparkSession is initialized when they try to set hive config.

How was this patch tested?

manual tests

SparkQA · 2017-07-29T12:29:34Z

Test build #80042 has finished for PR 18769 at commit 4bdc9ad.

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

SparkQA · 2017-07-30T07:04:52Z

Test build #80050 has finished for PR 18769 at commit cee6838.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

wangyum · 2017-07-30T07:58:51Z

retest this please

SparkQA · 2017-07-30T09:34:30Z

Test build #80051 has finished for PR 18769 at commit cee6838.

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

viirya · 2017-07-30T09:48:43Z

Please describe the issue to fix in the PR description, rather than just the codes to reproduce.

viirya · 2017-07-30T14:37:34Z

sql/hive/src/main/scala/org/apache/spark/sql/hive/client/HiveClientImpl.scala

    conf.get(key, defaultValue)
  }

+  override def setConf(key: String, value: String): Unit = {


Add clientLoader.synchronized?

viirya · 2017-07-30T14:38:11Z

sql/hive/src/main/scala/org/apache/spark/sql/hive/client/HiveClient.scala

  /** Returns the configuration for the given key in the current session. */
  def getConf(key: String, defaultValue: String): String

+  /** Set the given configuration property. */


nit: ... in the current session.

viirya · 2017-07-30T14:42:42Z

sql/hive/src/test/scala/org/apache/spark/sql/hive/InsertIntoHiveTableSuite.scala

+    val sourceTable = "sourceTable"
+    val targetTable = "targetTable"
+    (0 until cnt).map(i => (i, i)).toDF("c1", "c2").createOrReplaceTempView(sourceTable)
+    sql(s"create table $targetTable(c1 int) PARTITIONED BY(c2 int)")


nit: Usually we use upper case for SQL keywords like SELECT, CREATE TABLE, etc.

wangyum · 2017-07-31T09:49:38Z

@viirya Spark does not support that. see:#17223 (comment)
@dongjoon-hyun How about throw exception when users try to change them as @cloud-fan said?
or document it?
I have test --conf spark.hadoop.hive.exec.max.dynamic.partitions=1001 both works for spark-shell and spark-sql, --hiveconf hive.exec.max.dynamic.partitions=1001 works for spark-sql.

dongjoon-hyun · 2017-07-31T17:53:02Z

Yep. The workaround has been the solution for this.
And, +1 for the exception according to @cloud-fan 's suggestion, too.

wangyum · 2017-08-01T09:23:38Z

SetCommand.scala throws exception seems roughly. InsertIntoHiveTable throws exception seems too late. so I logWarning at SetCommand.scala

SparkQA · 2017-08-01T11:32:27Z

Test build #80114 has finished for PR 18769 at commit 506050a.

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

viirya · 2017-08-02T07:13:54Z

From the screenshot, it can't tell if the setting works or not. Please explicitly log that the setting doesn't work.

SparkQA · 2017-08-02T10:09:48Z

Test build #80155 has finished for PR 18769 at commit 2eb39cb.

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

gatorsmile · 2017-08-06T05:47:29Z

sql/core/src/main/scala/org/apache/spark/sql/execution/command/SetCommand.scala

    case Some((key, Some(value))) =>
      val runFunc = (sparkSession: SparkSession) => {
+        if (sparkSession.conf.get(CATALOG_IMPLEMENTATION.key).equals("hive")
+          && key.startsWith("hive.")) {


Nit: Could you move this && to the line 91?

Line length exceed 100:

if (sparkSession.conf.get(CATALOG_IMPLEMENTATION.key).equals("hive") && key.startsWith("hive.")) {

gatorsmile · 2017-08-06T06:00:13Z

sql/core/src/main/scala/org/apache/spark/sql/execution/command/SetCommand.scala

+          logWarning(s"SET $key=$value doesn't work, " +
+            s"because Spark doesn't support set hive config dynamically. " +
+            s"Please set hive config through " +
+            s"--conf spark.hadoop.$key=$value before SparkSession is initialized.")


How about?

'SET $key=$value' might not work, since Spark doesn't support changing the Hive config dynamically. Please passing the Hive-specific config by adding the prefix spark.hadoop (e.g.,spark.hadoop.$key) when starting a Spark application. For details, see the link: https://spark.apache.org/docs/latest/configuration.html#dynamically-loading-spark-properties.

gatorsmile · 2017-08-06T06:02:36Z

@wangyum Could you help us submit a PR to fix the doc generation in https://spark.apache.org/docs/latest/configuration.html?

See many annoying syntax issues.

wangyum · 2017-08-06T06:25:24Z

OK, I will try.

gatorsmile · 2017-08-06T06:26:28Z

sql/core/src/main/scala/org/apache/spark/sql/execution/command/SetCommand.scala

-            s"Please set hive config through " +
-            s"--conf spark.hadoop.$key=$value before SparkSession is initialized.")
+        if (sparkSession.conf.get(CATALOG_IMPLEMENTATION.key).equals("hive") &&
+          key.startsWith("hive.")) {


Nit: could you add two more spaces before key?

gatorsmile · 2017-08-06T06:27:20Z

sql/core/src/main/scala/org/apache/spark/sql/execution/command/SetCommand.scala

+            s"the Hive config dynamically. Please passing the Hive-specific config by adding the " +
+            s"prefix spark.hadoop (e.g.,spark.hadoop.$key) when starting a Spark application. " +
+            s"For details, see the link: https://spark.apache.org/docs/latest/configuration.html#" +
+            s"dynamically-loading-spark-properties.")


Except the line 93 and 95, the other lines do not need s

gatorsmile · 2017-08-06T06:28:11Z

sql/core/src/main/scala/org/apache/spark/sql/execution/command/SetCommand.scala

+          key.startsWith("hive.")) {
+          logWarning(s"'SET $key=$value' might not work, since Spark doesn't support changing " +
+            s"the Hive config dynamically. Please passing the Hive-specific config by adding the " +
+            s"prefix spark.hadoop (e.g.,spark.hadoop.$key) when starting a Spark application. " +


Please add a space after e.g.,

gatorsmile · 2017-08-06T06:29:14Z

You can refer the description in https://github.com/apache/spark/tree/master/docs to build the documentation. Thanks!

gatorsmile · 2017-08-06T06:30:13Z

LGTM pending a few minor comments.

SparkQA · 2017-08-06T07:04:51Z

Test build #80297 has finished for PR 18769 at commit 8787cbc.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2017-08-06T07:04:51Z

Test build #80298 has finished for PR 18769 at commit 03f5753.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

wangyum · 2017-08-06T08:27:05Z

retest this please

SparkQA · 2017-08-06T10:55:27Z

Test build #80299 has finished for PR 18769 at commit 03f5753.

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

wangyum · 2017-08-06T11:36:03Z

retest this please

SparkQA · 2017-08-06T14:16:38Z

Test build #80302 has finished for PR 18769 at commit 03f5753.

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

gatorsmile · 2017-08-06T17:09:03Z

Thanks! Merging to master.

wangyum · 2017-08-07T03:49:17Z

@gatorsmile Docs syntax issues was fixed by #18793.

Fix set hive.exec.max.dynamic.partitions lose effect.

4bdc9ad

Set conf to catalog when it's hive.

cee6838

viirya reviewed Jul 30, 2017

View reviewed changes

Point out user to set hive config before SparkSession is initialized.

506050a

wangyum changed the title ~~[SPARK-21574][SQL] Fix set hive.exec.max.dynamic.partitions lose effect.~~ [SPARK-21574][SQL] Point out user to set hive config before SparkSession is initialized Aug 1, 2017

improve log

2eb39cb

gatorsmile reviewed Aug 6, 2017

View reviewed changes

Update log message

8787cbc

gatorsmile reviewed Aug 6, 2017

View reviewed changes

gatorsmile approved these changes Aug 6, 2017

View reviewed changes

Remove redundant s

03f5753

asfgit closed this in 10b3ca3 Aug 6, 2017

[SPARK-21574][SQL] Point out user to set hive config before SparkSession is initialized #18769

[SPARK-21574][SQL] Point out user to set hive config before SparkSession is initialized #18769

Uh oh!

Conversation

wangyum commented Jul 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

SparkQA commented Jul 29, 2017

Uh oh!

SparkQA commented Jul 30, 2017

Uh oh!

wangyum commented Jul 30, 2017

Uh oh!

SparkQA commented Jul 30, 2017

Uh oh!

viirya commented Jul 30, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wangyum commented Jul 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dongjoon-hyun commented Jul 31, 2017

Uh oh!

wangyum commented Aug 1, 2017

Uh oh!

SparkQA commented Aug 1, 2017

Uh oh!

viirya commented Aug 2, 2017

Uh oh!

SparkQA commented Aug 2, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gatorsmile commented Aug 6, 2017

Uh oh!

wangyum commented Aug 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gatorsmile Aug 6, 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!

Choose a reason for hiding this comment

Uh oh!

gatorsmile commented Aug 6, 2017

Uh oh!

gatorsmile commented Aug 6, 2017

Uh oh!

SparkQA commented Aug 6, 2017

Uh oh!

SparkQA commented Aug 6, 2017

Uh oh!

wangyum commented Aug 6, 2017

Uh oh!

SparkQA commented Aug 6, 2017

Uh oh!

wangyum commented Aug 6, 2017

Uh oh!

SparkQA commented Aug 6, 2017

Uh oh!

gatorsmile commented Aug 6, 2017

Uh oh!

wangyum commented Aug 7, 2017

Uh oh!

Reviewers

Assignees

wangyum commented Jul 29, 2017 •

edited

Loading

wangyum commented Jul 31, 2017 •

edited

Loading

gatorsmile Aug 6, 2017 •

edited

Loading