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

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
add lazy flag
  • Loading branch information
@windpiger
windpiger committed Mar 2, 2017
commit 35d2b59ce0014c919ea284723da4e3677ba7cca8
2 changes: 1 addition & 1 deletion 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 @@ -36,7 +36,7 @@ import org.apache.spark.sql.catalyst.expressions.Expression
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

val defaultDB: CatalogDatabase = {
lazy val defaultDB: CatalogDatabase = {
val qualifiedWarehousePath = SessionCatalog
.makeQualifiedPath(warehousePath, hadoopConf).toString
CatalogDatabase(
Expand Down