Skip to content

[SPARK-19667][SQL]create table with hiveenabled in default database use warehouse path instead of the location of default database #17001

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
windpiger wants to merge 31 commits into from
Closed

[SPARK-19667][SQL]create table with hiveenabled in default database use warehouse path instead of the location of default database #17001

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
aebdfc6
[SPARK-19667][SQL]create table with hiveenabled in default database u…
windpiger Feb 20, 2017
825c0ad
rename a conf name
windpiger Feb 20, 2017
a2c9168
fix test faile
windpiger Feb 21, 2017
bacd528
process default database location when create/get database from metas…
windpiger Feb 22, 2017
3f6e061
remove an redundant line
windpiger Feb 22, 2017
96dcc7d
fix empty string location of database
windpiger Feb 22, 2017
f329387
modify the test case
windpiger Feb 22, 2017
83dba73
Merge branch 'master' into defaultDBPathInHive
windpiger Feb 22, 2017
58a0020
fix test failed
windpiger Feb 22, 2017
1dce2d7
add log to find out why jenkins failed
windpiger Feb 22, 2017
12f81d3
add scalastyle:off for println
windpiger Feb 22, 2017
56e83d5
fix test faile
windpiger Feb 22, 2017
901bb1c
make warehouse path qualified for default database
windpiger Feb 23, 2017
99d9746
remove a string s
windpiger Feb 23, 2017
db555e3
modify a comment
windpiger Feb 23, 2017
d327994
fix test failed
windpiger Feb 23, 2017
73c8802
move to sessioncatalog
windpiger Feb 23, 2017
747b31a
remove import
windpiger Feb 23, 2017
8f8063f
remove an import
windpiger Feb 23, 2017
4dc11c1
modify some codestyle and some comment
windpiger Feb 24, 2017
9c0773b
Merge branch 'defaultDBPathInHive' of github.com:windpiger/spark into…
windpiger Feb 24, 2017
80b8133
mv defaultdb path logic to ExternalCatalog
windpiger Feb 27, 2017
41ea115
modify a comment
windpiger Feb 27, 2017
13245e4
modify a comment
windpiger Feb 27, 2017
096ae63
add final def
windpiger Mar 1, 2017
badd61b
modify some code
windpiger Mar 2, 2017
35d2b59
add lazy flag
windpiger Mar 2, 2017
e3a467e
modify test case
windpiger Mar 3, 2017
ae9938a
modify test case
windpiger Mar 3, 2017
7739ccd
mv getdatabase
windpiger Mar 3, 2017
f93f5d3
merge with master
windpiger Mar 8, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
mv defaultdb path logic to ExternalCatalog
  • Loading branch information
@windpiger
windpiger committed Feb 27, 2017
commit 80b8133fa5caf8f17e07526fb89f321d51cb1a5e
22 changes: 20 additions & 2 deletions sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/ExternalCatalog.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,9 @@

package org.apache.spark.sql.catalyst.catalog

import org.apache.hadoop.conf.Configuration

import org.apache.spark.SparkConf
import org.apache.spark.sql.catalyst.analysis.{FunctionAlreadyExistsException, NoSuchDatabaseException, NoSuchFunctionException, NoSuchTableException}
import org.apache.spark.sql.catalyst.expressions.Expression

Expand All @@ -30,7 +33,7 @@ import org.apache.spark.sql.catalyst.expressions.Expression
*
* Implementations should throw [[NoSuchDatabaseException]] when databases don't exist.
*/
abstract class ExternalCatalog {
abstract class ExternalCatalog(conf: SparkConf, hadoopConf: Configuration) {
Copy link
Contributor

@cloud-fan cloud-fan Mar 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about we just pass in a defaultDB: CatalogDatabase? then we don't need to add the protected def warehousePath: String

Copy link
Contributor Author

@windpiger windpiger Mar 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think conf/hadoopConf is more useful, later logic can use it. and it's subclass also has these two conf

Copy link
Contributor

@cloud-fan cloud-fan Mar 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we still have conf/hadoopConf in InMemoryCatalog and HiveExternalCatalog, we can just add one more parameter.

Copy link
Contributor Author

@windpiger windpiger Mar 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we pass a defaultDB, it seems like we introduce an instance of defaultDB as we discussed above

Copy link
Contributor

@cloud-fan cloud-fan Mar 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but it will be only used in getDatabase, and we can save a metastore call to get the default database.

Copy link
Contributor Author

@windpiger windpiger Mar 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok~ let me fix it~

Copy link
Contributor Author

@windpiger windpiger Mar 2, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cloud-fan I found it that if we add a parameter defaultDB for ExternalCatalog and its subclass InMemoryCatalog and HiveExternalCatalog, this change will cause a lot of related code to be modified, such as test cases ,and other logic where create InMemoryCatalog and HiveExternalCatalog

For example:

currently all the parameters of InMemoryCatalog have its own default value

class InMemoryCatalog(conf: SparkConf = new SparkConf,hadoopConfig: Configuration = new Configuration)

we can create it without an parameters, but if we add a defaultDB, we should new a defaultDB in the parameter, while we can not create a legal deafultDB because we can not get the warehouse path for the defaultDB like this:

class InMemoryCatalog(conf: SparkConf = new SparkConf,hadoopConfig: Configuration = new Configuration, defaultDB: CatalogDatabase = CatalogDatabase("default","","${can not get the warehouse path}",Map.empty))

if we don't provide a default value for defautDB in the parameter, this will cause more code change which I think it is not proper.

what about we keep the provided def warehousePath in ExternalCatalog, and add a
lazy val defaultDB = { val qualifiedWarehousePath = SessionCatalog .makeQualifiedPath(warehousePath, hadoopConf).toString CatalogDatabase("default","", qualifiedWarehousePath, Map.empty) }

this can also avoid call getDatabase

Copy link
Contributor Author

@windpiger windpiger Mar 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have modify the code by adding

lazy val defaultDB = { val qualifiedWarehousePath = SessionCatalog .makeQualifiedPath(warehousePath, hadoopConf).toString CatalogDatabase("default","", qualifiedWarehousePath, Map.empty) }

in ExternalCatalog

if it is not ok ,I will revert it, thanks~

import CatalogTypes.TablePartitionSpec

protected def requireDbExists(db: String): Unit = {
Expand Down Expand Up @@ -74,7 +77,19 @@ abstract class ExternalCatalog {
*/
def alterDatabase(dbDefinition: CatalogDatabase): Unit

def getDatabase(db: String): CatalogDatabase
def getDatabase(db: String): CatalogDatabase = {
Copy link
Contributor

@cloud-fan cloud-fan Feb 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

final def

val database = getDatabaseInternal(db)
Copy link
Contributor

@cloud-fan cloud-fan Mar 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

put this in the else branch

// The default database's location always uses the warehouse path.
// Since the location of database stored in metastore is qualified,
// we also make the warehouse location qualified.
if (db == SessionCatalog.DEFAULT_DATABASE) {
Copy link
Contributor

@cloud-fan cloud-fan Feb 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we totally make default database a virtual concept? i.e. we never create it or ask metastore to retrieve it, just keep an instance of CatalogDatabase for default database, and return it in ExternalCatalog.getDatabase

Copy link
Contributor Author

@windpiger windpiger Mar 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think if we create an default instance of CatalogDatabase, we will add more logic to other interfaces, such as databaseExists ,we need to add the logic that if it is default database, we always return true even if there is no default database in hive.

Copy link
Contributor

@cloud-fan cloud-fan Mar 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense

val qualifiedWarehousePath = SessionCatalog
.makeQualifiedPath(warehousePath, hadoopConf).toString
database.copy(locationUri = qualifiedWarehousePath)
} else {
database
}
}

def databaseExists(db: String): Boolean

Expand Down Expand Up @@ -269,4 +284,7 @@ abstract class ExternalCatalog {

def listFunctions(db: String, pattern: String): Seq[String]

protected def getDatabaseInternal(db: String): CatalogDatabase

protected def warehousePath: String
}
6 changes: 4 additions & 2 deletions sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/InMemoryCatalog.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ import org.apache.spark.sql.catalyst.util.StringUtils
class InMemoryCatalog(
conf: SparkConf = new SparkConf,
hadoopConfig: Configuration = new Configuration)
extends ExternalCatalog {
extends ExternalCatalog(conf, hadoopConfig) {

import CatalogTypes.TablePartitionSpec

Expand Down Expand Up @@ -93,6 +93,8 @@ class InMemoryCatalog(
}
}

protected override def warehousePath: String =
catalog(SessionCatalog.DEFAULT_DATABASE).db.locationUri
// --------------------------------------------------------------------------
// Databases
// --------------------------------------------------------------------------
Expand Down Expand Up @@ -156,7 +158,7 @@ class InMemoryCatalog(
catalog(dbDefinition.name).db = dbDefinition
}

override def getDatabase(db: String): CatalogDatabase = synchronized {
protected override def getDatabaseInternal(db: String): CatalogDatabase = synchronized {
requireDbExists(db)
catalog(db).db
}
Expand Down
13 changes: 1 addition & 12 deletions sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalog.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,18 +193,7 @@ class SessionCatalog(
def getDatabaseMetadata(db: String): CatalogDatabase = {
val dbName = formatDatabaseName(db)
requireDbExists(dbName)
val database = externalCatalog.getDatabase(dbName)

// The default database's location always uses the warehouse path.
// Since the location of database stored in metastore is qualified,
// we also make the warehouse location qualified.
val dbLocation = if (dbName == SessionCatalog.DEFAULT_DATABASE) {
SessionCatalog.makeQualifiedPath(conf.warehousePath, hadoopConf).toString
} else {
database.locationUri
}

database.copy(locationUri = dbLocation)
externalCatalog.getDatabase(dbName)
}

def databaseExists(db: String): Boolean = {
Expand Down
15 changes: 4 additions & 11 deletions sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveExternalCatalog.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ import org.apache.spark.sql.types.{DataType, StructType}
* All public methods must be synchronized for thread-safety.
*/
private[spark] class HiveExternalCatalog(conf: SparkConf, hadoopConf: Configuration)
extends ExternalCatalog with Logging {
extends ExternalCatalog(conf, hadoopConf) with Logging {

import CatalogTypes.TablePartitionSpec
import HiveExternalCatalog._
Expand Down Expand Up @@ -129,6 +129,7 @@ private[spark] class HiveExternalCatalog(conf: SparkConf, hadoopConf: Configurat
}
}

protected override def warehousePath: String = conf.get(WAREHOUSE_PATH)
// --------------------------------------------------------------------------
// Databases
// --------------------------------------------------------------------------
Expand Down Expand Up @@ -162,7 +163,7 @@ private[spark] class HiveExternalCatalog(conf: SparkConf, hadoopConf: Configurat
client.alterDatabase(dbDefinition)
}

override def getDatabase(db: String): CatalogDatabase = withClient {
protected override def getDatabaseInternal(db: String): CatalogDatabase = withClient {
client.getDatabase(db)
}

Expand Down Expand Up @@ -408,15 +409,7 @@ private[spark] class HiveExternalCatalog(conf: SparkConf, hadoopConf: Configurat
}

private def defaultTablePath(tableIdent: TableIdentifier): String = {
// The default database's location always uses the warehouse path.
// Since the location of database stored in metastore is qualified,
// we also make the warehouse location qualified.
val dbLocation = if (tableIdent.database.orNull == SessionCatalog.DEFAULT_DATABASE) {
SessionCatalog.makeQualifiedPath(conf.get(WAREHOUSE_PATH), hadoopConf).toString
} else {
getDatabase(tableIdent.database.get).locationUri
}

val dbLocation = getDatabase(tableIdent.database.get).locationUri
new Path(new Path(dbLocation), tableIdent.table).toString
}

Expand Down