[SPARK-22487][SQL][followup] still keep spark.sql.hive.version #19719

cloud-fan · 2017-11-10T21:24:50Z

What changes were proposed in this pull request?

a followup of #19712 , adds back the spark.sql.hive.version, so that if users try to read this config, they can still get a default value instead of null.

How was this patch tested?

N/A

cloud-fan · 2017-11-10T21:25:01Z

cc @gatorsmile @yaooqinn

gatorsmile · 2017-11-10T21:26:58Z

Add back the test cases?

cloud-fan · 2017-11-10T21:41:14Z

reverted HiveThriftServer2Suites.scala from #19712

SparkQA · 2017-11-10T21:50:11Z

Test build #83706 has finished for PR 19719 at commit dd1e344.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2017-11-10T22:46:10Z

Test build #83708 has finished for PR 19719 at commit 0ce241d.

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

SparkQA · 2017-11-10T22:56:39Z

Test build #83705 has finished for PR 19719 at commit 7b767bf.

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

gatorsmile · 2017-11-10T23:18:00Z

...ftserver/src/test/scala/org/apache/spark/sql/hive/thriftserver/HiveThriftServer2Suites.scala

+        conf += resultSet.getString(1) -> resultSet.getString(2)
+      }
+
+      assert(conf.get("spark.sql.hive.version") === Some("1.2.1"))


If we do not set it explicitly, this will not be returned.

a default value will be returned, isn't it?

Nope, I think. Try it.

In Hive, "SET" returns all changed properties while "SET -v" returns all properties. In Spark, SET queries all key-value pairs that are set in the SQLConf of the sparkSession.

yaooqinn · 2017-11-11T00:17:57Z

sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveUtils.scala

@@ -66,6 +66,12 @@ private[spark] object HiveUtils extends Logging {
    .stringConf
    .createWithDefault(builtinHiveVersion)

+  // A deprecated config which is only used to provide a default value, in case some existing
+  // applications depend on this config, e.g. Spark SQL ODBC driver.
+  val HIVE_EXECUTION_VERSION = buildConf("spark.sql.hive.version")


doc may needed

This config is a kind of a fake one, I think comments are enough for it.

or maybe more meaningful name

yaooqinn · 2017-11-11T00:19:27Z

sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveUtils.scala

@@ -66,6 +66,12 @@ private[spark] object HiveUtils extends Logging {
    .stringConf
    .createWithDefault(builtinHiveVersion)

+  // A deprecated config which is only used to provide a default value, in case some existing
+  // applications depend on this config, e.g. Spark SQL ODBC driver.


i guess the jdbc driver do not depend on it. it used to only return 1.2.1， i can't imagine it a meaningful value, and i think that jdbc server may work fine with other hive versions too

it's ODBC. Actually it's something out of Spark's control, we must be careful and avoid behavior changes, like conf.get("spark.sql.hive.version") should still return 1.2.1 instead of null.

if Thrift Server only works for 1.2.1，I guess we don’t need an isolated classload for its hive client like SparkSQLCLIDriver

yaooqinn · 2017-11-11T00:30:29Z

make it an AlternativeConfig maybe better

SparkQA · 2017-11-11T01:01:02Z

Test build #83712 has finished for PR 19719 at commit 28fab02.

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

yaooqinn · 2017-11-11T01:12:21Z

sql/hive-thriftserver/src/main/scala/org/apache/spark/sql/hive/thriftserver/SparkSQLEnv.scala

@@ -55,6 +55,7 @@ private[hive] object SparkSQLEnv extends Logging {
      metadataHive.setOut(new PrintStream(System.out, true, "UTF-8"))
      metadataHive.setInfo(new PrintStream(System.err, true, "UTF-8"))
      metadataHive.setError(new PrintStream(System.err, true, "UTF-8"))
+      sparkSession.conf.set(HiveUtils.HIVE_EXECUTION_VERSION, HiveUtils.builtinHiveVersion)


HiveUtils.builtinHiveVersion => HiveUtils.HIVE_EXECUTION_VERSION?

yaooqinn · 2017-11-11T01:16:10Z

...ftserver/src/test/scala/org/apache/spark/sql/hive/thriftserver/HiveThriftServer2Suites.scala

+    }
+  }
+
+  test("Checks Hive version via SET") {


i might be a session level conf

SparkQA · 2017-11-11T01:16:33Z

Test build #83713 has finished for PR 19719 at commit 42d05a7.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2017-11-11T11:55:24Z

Test build #83723 has finished for PR 19719 at commit e37c1a8.

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

SparkQA · 2017-11-11T20:16:14Z

Test build #83732 has finished for PR 19719 at commit eb3137b.

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

SparkQA · 2017-11-12T01:28:47Z

Test build #83733 has finished for PR 19719 at commit 2088d6b.

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

gatorsmile · 2017-11-12T07:12:08Z

sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveUtils.scala

@@ -66,6 +66,14 @@ private[spark] object HiveUtils extends Logging {
    .stringConf
    .createWithDefault(builtinHiveVersion)

+  // A fake config which is only here for backward compatibility reasons. This config has no affect


affect -> effect

gatorsmile · 2017-11-12T07:12:38Z

sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveUtils.scala

@@ -66,6 +66,14 @@ private[spark] object HiveUtils extends Logging {
    .stringConf
    .createWithDefault(builtinHiveVersion)

+  // A fake config which is only here for backward compatibility reasons. This config has no affect
+  // to Spark, just for reporting the builtin Hive version of Spark to existing applications that
+  // already reply on this config.


reply -> rely

gatorsmile · 2017-11-12T07:16:26Z

Do something like the other deprecated conf MAPRED_REDUCE_TASKS ?
https://github.com/apache/spark/blob/master/sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala#L980-L986

gatorsmile · 2017-11-12T07:17:44Z

LGTM except a few comments

yaooqinn · 2017-11-12T15:59:57Z

LGTM

cloud-fan · 2017-11-12T17:40:27Z

Looking at SQLConf.Deprecated.MAPRED_REDUCE_TASKS, it needs to change many places, I don't think it worth for this config.

SparkQA · 2017-11-12T19:47:43Z

Test build #83748 has finished for PR 19719 at commit b172ce9.

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

gatorsmile · 2017-11-13T21:10:52Z

Thanks! Merged to master.

still keep spark.sql.hive.version

7b767bf

cloud-fan force-pushed the minor branch from dd1e344 to 0ce241d Compare November 10, 2017 22:23

gatorsmile reviewed Nov 10, 2017

View reviewed changes

yaooqinn reviewed Nov 11, 2017

View reviewed changes

cloud-fan force-pushed the minor branch from 0ce241d to 28fab02 Compare November 11, 2017 00:41

cloud-fan force-pushed the minor branch from 28fab02 to 42d05a7 Compare November 11, 2017 01:05

yaooqinn reviewed Nov 11, 2017

View reviewed changes

cloud-fan force-pushed the minor branch from 42d05a7 to e37c1a8 Compare November 11, 2017 11:37

cloud-fan force-pushed the minor branch from e37c1a8 to eb3137b Compare November 11, 2017 19:54

test

2088d6b

cloud-fan force-pushed the minor branch from eb3137b to 2088d6b Compare November 11, 2017 23:17

gatorsmile reviewed Nov 12, 2017

View reviewed changes

fix typo

b172ce9

asfgit closed this in f7534b3 Nov 13, 2017

[SPARK-22487][SQL][followup] still keep spark.sql.hive.version #19719

[SPARK-22487][SQL][followup] still keep spark.sql.hive.version #19719

Uh oh!

Conversation

cloud-fan commented Nov 10, 2017

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

cloud-fan commented Nov 10, 2017

Uh oh!

gatorsmile commented Nov 10, 2017

Uh oh!

cloud-fan commented Nov 10, 2017

Uh oh!

SparkQA commented Nov 10, 2017

Uh oh!

SparkQA commented Nov 10, 2017

Uh oh!

SparkQA commented Nov 10, 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!

yaooqinn Nov 11, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yaooqinn Nov 11, 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!

Choose a reason for hiding this comment

Uh oh!

yaooqinn commented Nov 11, 2017

Uh oh!

SparkQA commented Nov 11, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Nov 11, 2017

Uh oh!

SparkQA commented Nov 11, 2017

Uh oh!

SparkQA commented Nov 11, 2017

Uh oh!

SparkQA commented Nov 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gatorsmile commented Nov 12, 2017

Uh oh!

gatorsmile commented Nov 12, 2017

Uh oh!

yaooqinn commented Nov 12, 2017

Uh oh!

cloud-fan commented Nov 12, 2017

Uh oh!

SparkQA commented Nov 12, 2017

Uh oh!

gatorsmile commented Nov 13, 2017

Uh oh!

Uh oh!

yaooqinn Nov 11, 2017 •

edited

Loading

yaooqinn Nov 11, 2017 •

edited

Loading