[SPARK-17470][SQL] unify path for data source table and locationUri for hive serde table #15024

cloud-fan · 2016-09-09T13:20:05Z

What changes were proposed in this pull request?

Due to a limitation of hive metastore(table location must be directory path, not file path), we always store path for data source table in storage properties, instead of the locationUri field. However, we should not expose this difference to CatalogTable level, but just treat it as a hack in HiveExternalCatalog, like we store table schema of data source table in table properties.

This PR unifies path and locationUri outside of HiveExternalCatalog, both data source table and hive serde table should use the locationUri field.

This PR also unifies the way we handle default table location for managed table. Previously, the default table location of hive serde managed table is set by external catalog, but the one of data source table is set by command. After this PR, we follow the hive way and the default table location is always set by external catalog.

For managed non-file-based tables, we will assign a default table location and create an empty directory for it, the table location will be removed when the table is dropped. This is reasonable as metastore doesn't care about whether a table is file-based or not, and an empty table directory has no harm.
For external non-file-based tables, ideally we can omit the table location, but due to a hive metastore issue, we will assign a random location to it, and remove it right after the table is created. See SPARK-15269 for more details. This is fine as it's well isolated in HiveExternalCatalog.

To keep the existing behaviour of the path option, in this PR we always add the locationUri to storage properties using key path, before passing storage properties to DataSource as data source options.

How was this patch tested?

existing tests.

cloud-fan · 2016-09-09T13:20:44Z

cc @yhuai @gatorsmile

gatorsmile · 2016-09-09T14:04:34Z

I like this change! For Hive Managed table, locationUri should be filled; otherwise, we might be in trouble if Hive changes the default path to the table location when users do not provide the location.

SparkQA · 2016-09-09T14:42:03Z

Test build #65147 has finished for PR 15024 at commit 978c94c.

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

SparkQA · 2016-09-09T14:54:45Z

Test build #65148 has finished for PR 15024 at commit 1b474e4.

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

cloud-fan · 2016-09-12T12:03:36Z

@gatorsmile yea that's a good point. I have updated my PR to always set location for file-based data source table

SparkQA · 2016-09-12T16:43:30Z

Test build #65258 has finished for PR 15024 at commit d24d6ed.

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

SparkQA · 2016-09-12T16:47:16Z

Test build #65260 has finished for PR 15024 at commit 35def5b.

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

gatorsmile · 2016-09-12T18:59:19Z

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

For managed tables, do we need to set table.properties.get(DATASOURCE_LOCATION) to locationUri? Previously, Hive does it for us. Now, we explicitly remove it at multiple places. For example, alterTable removes it for both external tables and managed tables.

what do you mean? HiveExternalCatlaog.alterTable always add this location property.

gatorsmile · 2016-09-12T19:15:36Z

showDataSourceTableOptions in SHOW CREATE TABLE also needs an update.

clockfly · 2016-09-12T22:37:51Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/InMemoryCatalog.scala

Can we unify the if and else branch? Should we also check whether defaultTableLocation directory path is created if tableDefinition.storage.locationUri is set?

require(defaultTableLocation.toUri.toString == givenTableLocation) doesn't give a clear user-facing message. Should we replace it with an explicit Exception?

this is not user facing, if we hit this, we have a bug

SparkQA · 2016-09-16T08:44:13Z

Test build #65479 has finished for PR 15024 at commit 8efc52f.

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

SparkQA · 2016-09-16T16:20:32Z

Test build #65485 has finished for PR 15024 at commit bccd283.

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

SparkQA · 2016-10-26T11:31:28Z

Test build #67573 has finished for PR 15024 at commit 9a09b18.

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

gatorsmile · 2016-10-27T07:37:58Z

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

Just FYI, we do not have any test case to cover this. ALTER TABLE SET LOCATION for data source tables is only tested in DDLSuite (when Hive support is not enabled). That means, these code changes are not tested. Actually, this is not the only one. : (

Shall we make HiveDDLSuite extends DDLSuite? then all these tests will be run with hive support automatically.

cloud-fan · 2016-10-28T15:02:35Z

I have updated this PR according to these rules:

outside of ExternalCatalog, data source and hive tables should both use locationUri to store table location.
inside HiveExternalCatalog:
- do nothing for hive tables, same as before.
- for data source tables, always store locationUri in table properties(like provider, schema, etc.), and set locationUri to None if this table is not hive-compatible.
before passing storage properties to DataSource as data source options, add locationUri as path option, to keep the previous behaviour, i.e. the path option always exists(even users didn't specify it when creating table), and always equal to table location.

cc @rxin @yhuai @ericl @gatorsmile

SparkQA · 2016-10-28T17:20:20Z

Test build #67708 has finished for PR 15024 at commit 508648d.

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

gatorsmile · 2016-10-28T22:29:51Z

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

  val STATISTICS_COL_STATS_PREFIX = STATISTICS_PREFIX + "colStats."

  val TABLE_PARTITION_PROVIDER = SPARK_SQL_PREFIX + "partitionProvider"
+  val TABLE_LOCATION = SPARK_SQL_PREFIX + "tableLocation"


Should we use DATASOURCE_PREFIX here?

I don't have a strong preference.

gatorsmile · 2016-10-28T23:19:11Z

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

+      // may be not Hive-compatible and can not set the `locationUri` field. We should respect the
+      // old `locationUri` even it's None.
+      val oldLocation = getLocationFromRawTable(oldTableDef)
+      val locationUri = if (oldLocation == tableDefinition.storage.locationUri) {


When we using in-memory catalog, the location is changed for renameTable on managed data source tables.

It sounds like we are not having the same behavior when using hive external catalog?

The locationUri of raw table is not the final locationUri returned by HiveExternalCatalog, as we have restoreTableMetadata, so the behaviour is consistent.

test("alter table - rename") { val tabName = "tab1" val newTabName = "tab2" withTable(tabName, newTabName) { spark.range(10).write.saveAsTable(tabName) val catalog = spark.sessionState.catalog sql(s"ALTER TABLE $tabName RENAME TO $newTabName") sql(s"DESC FORMATTED $newTabName").show(100, false) assert(!catalog.tableExists(TableIdentifier(tabName))) assert(catalog.tableExists(TableIdentifier(newTabName))) } }

You can try to run the above test case in DDLSuite.scala and HiveDDLSuite.scala. The locations are different. One is using the new table name; another is using the old one.

good catch! I have fixed it in HiveExternalCatalog.renameTable

yhuai · 2016-10-31T15:45:26Z

before passing storage properties to DataSource as data source options, add locationUri as path option, to keep the previous behaviour, i.e. the path option always exists(even users didn't specify it when creating table), and always equal to table location.

How many places where we need to do this?

cloud-fan · 2016-11-01T08:12:03Z

@yhuai 3 places. It's easy to track it via code diff, as if we miss it somewhere, the test will fail because no path is specified.(data source tables never pass the paths parameter to DataSource).

BTW, I also added a new test suite to guarantee the semantic of the path option.

SparkQA · 2016-11-01T10:09:58Z

Test build #67880 has finished for PR 15024 at commit af53eee.

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

SparkQA · 2016-11-01T10:20:03Z

Test build #67881 has finished for PR 15024 at commit 97f04cf.

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

gatorsmile · 2016-11-02T02:53:31Z

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

-    //  1. Put table provider, schema, partition column names, bucket specification and partition
-    //     provider in table properties.
+    //  1. Put table provider, location URI, schema, partition column names, bucket specification
+    //     and partition provider in table properties.


Could we create a separate function for this step? The related codes become longer and longer.

Could we change the description order to match the order of codes?

Put table provider, partition provider, location URI, schema, partition column names, bucket specification in table properties.

yhuai · 2016-11-02T03:10:26Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/InMemoryCatalog.scala

+      // to create the table directory and write out data before we create this table, to avoid
+      // exposing a partial written table.
+      val needDefaultTableLocation =
+      tableDefinition.tableType == CatalogTableType.MANAGED &&


indentation

yhuai · 2016-11-02T03:56:10Z

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

    }

-    val optionsWithPath = if (table.tableType == CatalogTableType.MANAGED) {
-      table.storage.properties + ("path" -> sessionState.catalog.defaultTablePath(table.identifier))


where do we assign the default location?

yhuai · 2016-11-02T03:59:16Z

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

        tableType = CatalogTableType.MANAGED,
-        storage = newStorage,
+        // We are creating a new managed table, which should not have custom table location.
+        storage = sourceTableDesc.storage.copy(locationUri = None),


When will we set the location? Is it set by hive metastore?

yhuai · 2016-11-02T04:01:51Z

sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/DataSource.scala

+    "org.apache.spark.Logging")
+
+  /** Given a provider name, look up the data source class definition. */
+  def lookupDataSource(provider0: String): Class[_] = {


why not just provider?

yhuai · 2016-11-02T04:04:48Z

sql/core/src/test/scala/org/apache/spark/sql/sources/PathOptionSuite.scala

+  override def createRelation(
+      sqlContext: SQLContext,
+      parameters: Map[String, String]): BaseRelation = {
+    new TestOptionsRelation(parameters)(sqlContext.sparkSession)


Can we also add comment to explain which tests exercise this method?

yhuai · 2016-11-02T04:06:53Z

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

        tableProperties.put(TABLE_PARTITION_PROVIDER, "hive")
      }
+      tableDefinition.storage.locationUri.foreach { location =>
+        tableProperties.put(TABLE_LOCATION, location)


why do we need this? Why not just use path in serde properties?

yhuai · 2016-11-02T04:08:53Z

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

+          require(tableDefinition.storage.locationUri.isDefined,
            "External file-based data source table must have a `path` entry in storage properties.")
-          Some(new Path(map("path")).toUri.toString)
+          Some(new Path(tableDefinition.storage.locationUri.get).toUri.toString)


This part looks weird since we already have a location uri.

shall we rename locationUri to something else? this can be any string given by users in path option, and may not be a uri

yhuai · 2016-11-02T04:10:10Z

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

+    val newTable = rawTable.copy(
+      identifier = TableIdentifier(newName, Some(db)),
+      properties = tableProps)
+


I am not sure if I am following at here. So, after rename, we will not have a table property representing the location?

yhuai · 2016-11-02T04:15:31Z

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

+  private def getLocationFromRawTable(rawTable: CatalogTable): Option[String] = {
+    rawTable.properties.get(TABLE_LOCATION).orElse {
+      // In older version of spark, we store the table location in storage properties with key
+      // `path`, instead of table properties with key `spark.sql.tableLocation`. We should


Why do we need spark.sql.tableLocation instead of just relying on hive's location field and path in serde properties?

gatorsmile · 2016-11-02T05:25:32Z

sql/core/src/test/scala/org/apache/spark/sql/sources/PathOptionSuite.scala

+
+    withTable("src", "src2") {
+      sql(s"CREATE TABLE src(i int) USING ${classOf[TestOptionsSource].getCanonicalName}")
+      sql("ALTER TABLE src RENAME TO src2")


This test case is still calling the InMemoryCatalog.renameTable. Thus, we still need a test case to verify the behavior of HiveExternalCatalog.renameTable

It's nothing about ExternalCatalog, as ExternalCatalog doesn't need to know about the path option. We generate the path option outside of ExternalCatalog, and we only need ExternalCatalog to put table location in the locationUri field correctly.

Agree. But we still need a test case to verify the code path and the behavior you mentioned above. So far, it sounds like we do not have any end-to-end test case for RENAME TABLE using HiveExternalCatalog. I manually verified it and it works.

yea, we should create a JIRA for improving the DDL command test coverage.

SparkQA · 2016-11-02T17:21:25Z

Test build #67989 has finished for PR 15024 at commit d1bcb49.

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

yhuai · 2016-11-03T00:45:56Z

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

-          }
-        }
+      val needDefaultTableLocation = tableDefinition.tableType == MANAGED &&
+        tableDefinition.storage.locationUri.isEmpty


Explain when the table is managed but the location uri is not empty.

yhuai · 2016-11-03T00:53:45Z

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

+      val storageWithNewLocation = if (oldLocation == newLocation) {
+        oldTableDef.storage
+      } else {
+        updateLocationInStorageProps(oldTableDef, newLocation).copy(locationUri = newLocation)


Also explain this copy.

yhuai · 2016-11-03T01:01:33Z

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

+        // path option in storage properties, to avoid exposing this concept externally.
+        val storageWithLocation = {
+          val tableLocation = getLocationFromStorageProps(table)
+          updateLocationInStorageProps(table, None).copy(locationUri = tableLocation)


Explain the reason that we are passing None.

yhuai · 2016-11-03T01:04:04Z

sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/SQLQuerySuite.scala

        userSpecifiedLocation match {
          case Some(location) =>
-            assert(r.options("path") === location)
+            assert(table.storage.locationUri.get === location)


Seems we can revert this changes.

yhuai · 2016-11-03T01:04:26Z

LGTM. Merging to master and branch 2.1.

…or hive serde table ## What changes were proposed in this pull request? Due to a limitation of hive metastore(table location must be directory path, not file path), we always store `path` for data source table in storage properties, instead of the `locationUri` field. However, we should not expose this difference to `CatalogTable` level, but just treat it as a hack in `HiveExternalCatalog`, like we store table schema of data source table in table properties. This PR unifies `path` and `locationUri` outside of `HiveExternalCatalog`, both data source table and hive serde table should use the `locationUri` field. This PR also unifies the way we handle default table location for managed table. Previously, the default table location of hive serde managed table is set by external catalog, but the one of data source table is set by command. After this PR, we follow the hive way and the default table location is always set by external catalog. For managed non-file-based tables, we will assign a default table location and create an empty directory for it, the table location will be removed when the table is dropped. This is reasonable as metastore doesn't care about whether a table is file-based or not, and an empty table directory has no harm. For external non-file-based tables, ideally we can omit the table location, but due to a hive metastore issue, we will assign a random location to it, and remove it right after the table is created. See SPARK-15269 for more details. This is fine as it's well isolated in `HiveExternalCatalog`. To keep the existing behaviour of the `path` option, in this PR we always add the `locationUri` to storage properties using key `path`, before passing storage properties to `DataSource` as data source options. ## How was this patch tested? existing tests. Author: Wenchen Fan <[email protected]> Closes #15024 from cloud-fan/path. (cherry picked from commit 3a1bc6f) Signed-off-by: Yin Huai <[email protected]>

yhuai · 2016-11-03T01:06:19Z

OK. Let's get #14750 updated to fix SPARK-17183.

…or hive serde table ## What changes were proposed in this pull request? Due to a limitation of hive metastore(table location must be directory path, not file path), we always store `path` for data source table in storage properties, instead of the `locationUri` field. However, we should not expose this difference to `CatalogTable` level, but just treat it as a hack in `HiveExternalCatalog`, like we store table schema of data source table in table properties. This PR unifies `path` and `locationUri` outside of `HiveExternalCatalog`, both data source table and hive serde table should use the `locationUri` field. This PR also unifies the way we handle default table location for managed table. Previously, the default table location of hive serde managed table is set by external catalog, but the one of data source table is set by command. After this PR, we follow the hive way and the default table location is always set by external catalog. For managed non-file-based tables, we will assign a default table location and create an empty directory for it, the table location will be removed when the table is dropped. This is reasonable as metastore doesn't care about whether a table is file-based or not, and an empty table directory has no harm. For external non-file-based tables, ideally we can omit the table location, but due to a hive metastore issue, we will assign a random location to it, and remove it right after the table is created. See SPARK-15269 for more details. This is fine as it's well isolated in `HiveExternalCatalog`. To keep the existing behaviour of the `path` option, in this PR we always add the `locationUri` to storage properties using key `path`, before passing storage properties to `DataSource` as data source options. ## How was this patch tested? existing tests. Author: Wenchen Fan <[email protected]> Closes apache#15024 from cloud-fan/path.

cloud-fan force-pushed the path branch from 978c94c to 1b474e4 Compare September 9, 2016 13:34

cloud-fan mentioned this pull request Sep 9, 2016

[SPARK-17446] [SQL] No total size for data source tables in InMemoryCatalog #15012

Closed

cloud-fan force-pushed the path branch from 1b474e4 to d24d6ed Compare September 12, 2016 11:56

gatorsmile reviewed Sep 12, 2016
View reviewed changes

gatorsmile mentioned this pull request Sep 12, 2016

[SPARK-17183][SPARK-17983][SPARK-18101][SQL] put hive serde table schema to table properties like data source table #14750

Closed

clockfly reviewed Sep 12, 2016
View reviewed changes

cloud-fan force-pushed the path branch from 35def5b to 8efc52f Compare September 16, 2016 07:30

cloud-fan force-pushed the path branch 2 times, most recently from 82c67cf to b09d8bb Compare September 16, 2016 15:15

gatorsmile reviewed Oct 27, 2016

View reviewed changes

unify path for data source table and locationUri for hive serde table

508648d

cloud-fan force-pushed the path branch from 9a09b18 to 508648d Compare October 28, 2016 15:02

gatorsmile reviewed Oct 28, 2016

View reviewed changes

address comments

97f04cf

cloud-fan force-pushed the path branch from af53eee to 97f04cf Compare November 1, 2016 08:01

gatorsmile reviewed Nov 2, 2016

View reviewed changes

yhuai reviewed Nov 2, 2016

View reviewed changes

gatorsmile reviewed Nov 2, 2016

View reviewed changes

cloud-fan added 2 commits November 2, 2016 15:29

Merge remote-tracking branch 'origin/master' into path

6fd4dda

address comments

d1bcb49

yhuai reviewed Nov 3, 2016

View reviewed changes

asfgit closed this in 3a1bc6f Nov 3, 2016

cloud-fan mentioned this pull request Nov 24, 2016

[SPARK-18482][SQL] make sure Spark can access the table metadata created by older version of spark #16003

Closed

[SPARK-17470][SQL] unify path for data source table and locationUri for hive serde table #15024

[SPARK-17470][SQL] unify path for data source table and locationUri for hive serde table #15024

Uh oh!

Conversation

cloud-fan commented Sep 9, 2016 • 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!

cloud-fan commented Sep 9, 2016

Uh oh!

gatorsmile commented Sep 9, 2016

Uh oh!

SparkQA commented Sep 9, 2016

Uh oh!

SparkQA commented Sep 9, 2016

Uh oh!

cloud-fan commented Sep 12, 2016

Uh oh!

SparkQA commented Sep 12, 2016

Uh oh!

SparkQA commented Sep 12, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gatorsmile commented Sep 12, 2016

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!

SparkQA commented Sep 16, 2016

Uh oh!

SparkQA commented Sep 16, 2016

Uh oh!

SparkQA commented Oct 26, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cloud-fan commented Oct 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SparkQA commented Oct 28, 2016

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yhuai commented Oct 31, 2016

Uh oh!

cloud-fan commented Nov 1, 2016

Uh oh!

SparkQA commented Nov 1, 2016

Uh oh!

SparkQA commented Nov 1, 2016

Uh oh!

gatorsmile Nov 2, 2016 • 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!

cloud-fan commented Sep 9, 2016 •

edited

Loading

cloud-fan commented Oct 28, 2016 •

edited

Loading

gatorsmile Nov 2, 2016 •

edited

Loading

cloud-fan Nov 2, 2016 •

edited

Loading