[SPARK-31751][SQL]Serde property `path` overwrites hive table property location #28882

TJX2014 · 2020-06-21T06:55:41Z

What changes were proposed in this pull request?

1.Add a UT in org.apache.spark.sql.hive.HiveExternalCatalogSuite
2.Throw AnalysisException when table storage location url not equal to storage path properties.

Why are the changes needed?

Step 1,When hive support is enabled and we use sparksql create and save hive table as fellow：
df = spark.createDataFrame([{"a": "x", "b": "y", "c": "3"}])
df.write.format("orc").option("compression", "ZLIB").mode("overwrite").saveAsTable('test_spark');
Step 2,When we alter table name through hive interface not spark like below：
alter table test_spark rename to test_spark2;
The location of test_spark is changed to test_spark2 but spark maintain the path serde test_spark，this lead to spark read test_spark while test_spark2 is expected
Step 3, Since we can not forbidden alter table from hive side, once this happens, we need to info users.

Does this PR introduce any user-facing change?

Yes，once users alter table in hive side, users will get an AnalysisException to maintain consistent location and path property.

How was this patch tested?

Unit test.

TJX2014 · 2020-06-24T12:35:54Z

@cloud-fan Could you please help me check this PR. :-)

dongjoon-hyun

Could you provide a test case for this, @TJX2014 ? Thanks!

TJX2014 · 2020-06-28T07:54:26Z

Could you provide a test case for this, @TJX2014 ? Thanks!

Kindly ping @dongjoon-hyun Thanks for your suggestion, I have added a UT for spark.sql.follow.hive.table.location

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

dongjoon-hyun · 2020-06-28T22:05:19Z

ok to test

dongjoon-hyun · 2020-06-28T22:07:43Z

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

+      tableType = CatalogTableType.MANAGED,
+      storage = storageFormat,
+      schema = new StructType().add("col1", "int"),
+      provider = Some("orc"))


Conventionally, we use provider = Some("parquet")) for the tables who names start with parq_.

dongjoon-hyun · 2020-06-28T22:20:46Z

sql/catalyst/src/main/scala/org/apache/spark/sql/internal/StaticSQLConf.scala

@@ -226,4 +226,11 @@ object StaticSQLConf {
      .version("3.0.0")
      .intConf
      .createWithDefault(100)
+
+  val FOLLOW_HIVE_TABLE_LOCATION_ENABLED =
+    buildStaticConf("spark.sql.follow.hive.table.location")


Hi, @TJX2014 . Since you are working on these, I'll inform you some community rules. Not every rule is written clearly, so I understand that you may not be aware of that. This is just a tip for your this and next PRs.

NAMING: configuration naming follows a namespace concept. Please don't make a new namespace, spark.sql.follow if that is a new set of concept. For all Hive related config, spark.sql.hive is used as the namespace. Please see the other existing Hive configurations.

MODULE: Apache Spark follows modular design. In general, for Hive Specific configurations, sql/hive module is the better place to be considered for a new conf instead of sql/catalyst module. Please try to put into HiveUtils.scala . If HiveUtils.scala is insufficient for your PR, then, you can promote your conf into sql/core or sql/catalyst modules.

dongjoon-hyun · 2020-06-28T22:21:28Z

Thank you for your contribution, @TJX2014 . For this feature, it seems that we need more time to review.

SparkQA · 2020-06-29T03:01:41Z

Test build #124610 has finished for PR 28882 at commit 161d54e.

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

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

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

TJX2014 · 2020-06-29T03:20:44Z

@dongjoon-hyun Thank you for your suggestion, I have corrected it.

cloud-fan · 2020-06-29T04:40:32Z

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

@@ -545,7 +545,11 @@ private[spark] class HiveExternalCatalog(conf: SparkConf, hadoopConf: Configurat
  }

  private def getLocationFromStorageProps(table: CatalogTable): Option[String] = {
-    CaseInsensitiveMap(table.storage.properties).get("path")
+    if (conf.get(HiveUtils.FOLLOW_TABLE_LOCATION)) {


It looks tricky to have a config for such a subtle behavior. I think it makes more sense to fail when the path serde property doesn't match the table location, and ask users to either correct the path property or the table location.

@cloud-fan Thank you for your suggestion, I have corrected.

SparkQA · 2020-06-29T07:18:31Z

Test build #124624 has started for PR 28882 at commit 43a949c.

SparkQA · 2020-06-29T07:34:59Z

Test build #124631 has started for PR 28882 at commit 81a5eec.

TJX2014 · 2020-06-29T23:49:58Z

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

+    externalCatalog.client.runSqlHive(
+      "alter table db1.parq_alter rename to db1.parq_alter2")
+
+    val e = intercept[AnalysisException](
+      externalCatalog.getTable("db1", "parq_alter2")
+    )
+    assert(e.getMessage.contains("not equal to table prop path")
+      && e.getMessage.contains("parq_alter2"))
+  }


We will get an exception when the path property is not consistent with storage location.

cloud-fan · 2020-06-30T15:07:32Z

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

@@ -545,7 +545,14 @@ private[spark] class HiveExternalCatalog(conf: SparkConf, hadoopConf: Configurat
  }

  private def getLocationFromStorageProps(table: CatalogTable): Option[String] = {
-    CaseInsensitiveMap(table.storage.properties).get("path")
+    val storageLoc = table.storage.locationUri.map(_.toString)
+    val storageProp = CaseInsensitiveMap(table.storage.properties).get("path")


I'm a bit worried about string comparison. Are you sure the path string is always equal to the URI string? Shall we do normalization before comparing?

We normalization URI as toString => CatalogUtils.URIToString(_) may be better ?

to be safe, can we compare the URI? we can convert path string to URI with CatalogUtils.stringToURI.

SparkQA · 2020-07-01T00:56:57Z

Test build #124697 has finished for PR 28882 at commit 51e1cb4.

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

TJX2014 · 2020-07-01T07:26:41Z

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

-    val parquetTable = CatalogTable(
-      identifier = TableIdentifier("parq_tbl", Some("db1")),
-      tableType = CatalogTableType.MANAGED,
-      storage = storageFormat.copy(locationUri = Some(new URI("file:/some/path"))),
-      schema = new StructType().add("col1", "int").add("col2", "string"),
-      provider = Some("parquet"))
-    catalog.createTable(parquetTable, ignoreIfExists = false)


@brkyvz @cloud-fan #27822
I find storage = storageFormat.copy(locationUri = Some(new URI("file:/some/path"))), maybe useless, and will cause the path incompatible with path property. So shall we remove it and merge the two test cases to one ?

why does it cause path incompatibility? storageFormat doesn't have a path property.

Because this UT failed and tell us：path in location Some(file:/tmp/spark-cc7f24a5-e626-4d91-afc6-4f2702482980/parq_tbl) not equal to table prop path Some(file:/some/path), so could I send another PR to polish this ?

Yes please. This test looks like a valid one and should not fail.

Yes, it is a valid case, and it seems there are something to improve, #28980

SparkQA · 2020-07-01T07:45:10Z

Test build #124754 has finished for PR 28882 at commit 35ec888.

This patch fails to generate documentation.
This patch merges cleanly.
This patch adds no public classes.

… in catalog

…mpatible with hive table

SparkQA · 2020-07-05T20:34:37Z

Test build #124946 has finished for PR 28882 at commit 1496c66.

This patch fails to generate documentation.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2020-07-07T12:47:58Z

can we test non-hive compatible file sources like json? e.g. CREATE TABLE t USING json LOCATION .... It seems like the serde property path will always be different from the table location, as spark doesn't store the table location at all.

TJX2014 · 2020-07-08T14:54:53Z

Seems we should consider CREATE TABLE t USING json LOCATION ...

github-actions · 2020-10-17T00:53:16Z

We're closing this PR because it hasn't been updated in a while. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable.
If you'd like to revive this PR, please reopen it and ask a committer to remove the Stale tag!

probot-autolabeler bot added the SQL label Jun 21, 2020

dongjoon-hyun requested changes Jun 27, 2020

View reviewed changes

TJX2014 commented Jun 28, 2020

View reviewed changes

sql/hive/src/test/scala/org/apache/spark/sql/hive/HiveExternalCatalogSuite.scala Show resolved Hide resolved

TJX2014 requested a review from dongjoon-hyun June 28, 2020 09:13

dongjoon-hyun reviewed Jun 28, 2020

View reviewed changes

TJX2014 commented Jun 29, 2020

View reviewed changes

sql/hive/src/test/scala/org/apache/spark/sql/hive/HiveExternalCatalogSuite.scala Show resolved Hide resolved

TJX2014 commented Jun 29, 2020

View reviewed changes

sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveUtils.scala Outdated Show resolved Hide resolved

TJX2014 commented Jun 29, 2020

View reviewed changes

sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveUtils.scala Outdated Show resolved Hide resolved

TJX2014 requested a review from dongjoon-hyun June 29, 2020 03:17

cloud-fan reviewed Jun 29, 2020

View reviewed changes

TJX2014 commented Jun 29, 2020

View reviewed changes

TJX2014 requested a review from cloud-fan June 30, 2020 13:16

cloud-fan reviewed Jun 30, 2020

View reviewed changes

TJX2014 commented Jul 1, 2020

View reviewed changes

TJX2014 added 5 commits July 3, 2020 06:32

spark.sql.follow.hive.table.location to compatible with legacy path…

87ff39b

… in catalog

UT for hive table shared location

b45d2b3

param loc from catalog module to hive module

e51fd48

param to FOLLOW_TABLE_LOCATION

2d5ba45

throw exception when location is not consistent

fc9143d

TJX2014 added 2 commits July 3, 2020 06:32

toString => CatalogUtils.URIToString(_)

e44c1b9

compact SPARK-31061 test case and make getLocationFromStorageProps co…

1496c66

…mpatible with hive table

TJX2014 force-pushed the master-SPARK-31751-hive-table-location branch from 35ec888 to 1496c66 Compare July 2, 2020 22:35

TJX2014 requested a review from cloud-fan July 2, 2020 22:40

github-actions bot added the Stale label Oct 17, 2020

github-actions bot closed this Oct 18, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-31751][SQL]Serde property `path` overwrites hive table property location #28882

[SPARK-31751][SQL]Serde property `path` overwrites hive table property location #28882

TJX2014 commented Jun 21, 2020 •

edited

Loading

TJX2014 commented Jun 24, 2020

dongjoon-hyun left a comment

TJX2014 commented Jun 28, 2020

dongjoon-hyun commented Jun 28, 2020

dongjoon-hyun Jun 28, 2020

dongjoon-hyun Jun 28, 2020

dongjoon-hyun commented Jun 28, 2020

SparkQA commented Jun 29, 2020

TJX2014 commented Jun 29, 2020

cloud-fan Jun 29, 2020

TJX2014 Jun 29, 2020

SparkQA commented Jun 29, 2020

SparkQA commented Jun 29, 2020

TJX2014 Jun 29, 2020 •

edited

Loading

cloud-fan Jun 30, 2020

TJX2014 Jun 30, 2020

cloud-fan Jul 7, 2020

SparkQA commented Jul 1, 2020

TJX2014 Jul 1, 2020 •

edited

Loading

cloud-fan Jul 1, 2020

TJX2014 Jul 1, 2020 •

edited

Loading

cloud-fan Jul 2, 2020

TJX2014 Jul 2, 2020

SparkQA commented Jul 1, 2020

SparkQA commented Jul 5, 2020

cloud-fan commented Jul 7, 2020

TJX2014 commented Jul 8, 2020

github-actions bot commented Oct 17, 2020

[SPARK-31751][SQL]Serde property path overwrites hive table property location #28882

[SPARK-31751][SQL]Serde property path overwrites hive table property location #28882

Conversation

TJX2014 commented Jun 21, 2020 • edited Loading

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

TJX2014 commented Jun 24, 2020

dongjoon-hyun left a comment

Choose a reason for hiding this comment

TJX2014 commented Jun 28, 2020

dongjoon-hyun commented Jun 28, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dongjoon-hyun commented Jun 28, 2020

SparkQA commented Jun 29, 2020

TJX2014 commented Jun 29, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Jun 29, 2020

SparkQA commented Jun 29, 2020

TJX2014 Jun 29, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Jul 1, 2020

TJX2014 Jul 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TJX2014 Jul 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Jul 1, 2020

SparkQA commented Jul 5, 2020

cloud-fan commented Jul 7, 2020

TJX2014 commented Jul 8, 2020

github-actions bot commented Oct 17, 2020

[SPARK-31751][SQL]Serde property `path` overwrites hive table property location #28882

[SPARK-31751][SQL]Serde property `path` overwrites hive table property location #28882

TJX2014 commented Jun 21, 2020 •

edited

Loading

TJX2014 Jun 29, 2020 •

edited

Loading

TJX2014 Jul 1, 2020 •

edited

Loading

TJX2014 Jul 1, 2020 •

edited

Loading