Skip to content

[SPARK-18817] [SPARKR] [SQL] Set default warehouse dir to tempdir #16290

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
shivaram wants to merge 9 commits into from
Closed

[SPARK-18817] [SPARKR] [SQL] Set default warehouse dir to tempdir #16290

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
ac05089
Set default warehouse dir to tempdir
shivaram Dec 15, 2016
2583410
Fix drop table command
shivaram Dec 15, 2016
1d0d1d2
Handle default warehouse path in SQLConf
shivaram Dec 15, 2016
014d7e1
Update unit test. Address comments
shivaram Dec 16, 2016
6eec97d
Update comments, fix style
shivaram Dec 16, 2016
f7b4772
Address code review comments
shivaram Dec 18, 2016
047054e
Merge remote-tracking branch 'apache/master' into spark-warehouse-def…
shivaram Mar 6, 2017
b14c302
Revise PR to set tmpdir in SQLUtils
shivaram Mar 6, 2017
7a98b91
Clear default session after test
shivaram Mar 7, 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
Address code review comments
  • Loading branch information
@shivaram
shivaram committed Dec 18, 2016
commit f7b4772a4b72c28047afb7d614e3af3317af896d
4 changes: 3 additions & 1 deletion sql/core/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -820,7 +820,8 @@ private[sql] class SQLConf extends Serializable with CatalystConf with Logging {
def variableSubstituteDepth: Int = getConf(VARIABLE_SUBSTITUTE_DEPTH)

def warehousePath: String = {
if (contains(StaticSQLConf.WAREHOUSE_PATH.key)) {
if (contains(StaticSQLConf.WAREHOUSE_PATH.key) &&
getConf(StaticSQLConf.WAREHOUSE_PATH).isDefined) {
new Path(getConf(StaticSQLConf.WAREHOUSE_PATH).get).toString
} else {
new Path(getConf(StaticSQLConf.DEFAULT_WAREHOUSE_PATH)).toString
Expand Down Expand Up @@ -971,6 +972,7 @@ object StaticSQLConf {
}

val DEFAULT_WAREHOUSE_PATH = buildConf("spark.sql.default.warehouse.dir")
Copy link
Member

@gatorsmile gatorsmile Dec 17, 2016

Choose a reason for hiding this comment

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

Should we make it internal?

Copy link
Contributor Author

@shivaram shivaram Dec 18, 2016

Choose a reason for hiding this comment

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

I am not familiar with this part of the code base - What are the consequences of making it internal ? Is it just in terms of what shows up in documentation or does it affect how users can use it ?

Copy link
Member

@gatorsmile gatorsmile Dec 18, 2016

Choose a reason for hiding this comment

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

For the internal configuration, it will not be printed out. For example, you can try something like

spark.sql("SET -v").show(numRows = 200, truncate = false)

.internal()
.doc("Default location used for managed databases and tables " +
"if spark.sql.warehouse.dir is not set")
.stringConf
Expand Down
44 changes: 44 additions & 0 deletions sql/core/src/test/scala/org/apache/spark/sql/internal/SQLConfSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,16 +222,60 @@ class SQLConfSuite extends QueryTest with SharedSQLContext {
}

test("changing default value of warehouse path") {
Copy link
Member

@gatorsmile gatorsmile Dec 17, 2016

Choose a reason for hiding this comment

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

Currently, this test case only cover one of four cases. spark.sql.default.warehouse.dir is set and spark.sql.warehouse.dir is not set. We also need to check the other three cases:

  • spark.sql.default.warehouse.dir is not set and spark.sql.warehouse.dir is not set
  • spark.sql.default.warehouse.dir is set and spark.sql.warehouse.dir is set
  • spark.sql.default.warehouse.dir is not set and spark.sql.warehouse.dir is set

Copy link
Contributor Author

@shivaram shivaram Dec 18, 2016

Choose a reason for hiding this comment

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

Good point. Added tests for all 4 cases now

// Set sql.default.warehouse.dir but not sql.warehouse.dir
try {
val newWarehouseDefault = "spark-warehouse2"
val newWarehouseDefaultPath = new Path(Utils.resolveURI(newWarehouseDefault)).toString
sparkContext.conf.set("spark.sql.default.warehouse.dir", newWarehouseDefaultPath)
val spark = new SparkSession(sparkContext)
assert(newWarehouseDefaultPath.stripSuffix("/") === spark
.sessionState.conf.warehousePath.stripSuffix("/"))
Copy link
Member

@gatorsmile gatorsmile Dec 17, 2016

Choose a reason for hiding this comment

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

Also need a check for spark.sharedState.warehousePath because we did the logic changes there.

Copy link
Contributor Author

@shivaram shivaram Dec 18, 2016

Choose a reason for hiding this comment

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

Done

assert(newWarehouseDefaultPath.stripSuffix("/") === spark
.sharedState.warehousePath.stripSuffix("/"))
} finally {
sparkContext.conf.remove("spark.sql.default.warehouse.dir")
}

// Set sql.warehouse.dir and sql.default.warehouse.dir. The first one should be used
try {
val newWarehouseDefault = "spark-warehouse2"
val newWarehouseDefaultPath = new Path(Utils.resolveURI(newWarehouseDefault)).toString
sparkContext.conf.set("spark.sql.default.warehouse.dir", newWarehouseDefaultPath)

val newWarehouse = "spark-warehouse3"
val newWarehousePath = new Path(Utils.resolveURI(newWarehouse)).toString
sparkContext.conf.set("spark.sql.warehouse.dir", newWarehousePath)
val spark = new SparkSession(sparkContext)
assert(newWarehousePath.stripSuffix("/") === spark
.sessionState.conf.warehousePath.stripSuffix("/"))
assert(newWarehousePath.stripSuffix("/") === spark
.sharedState.warehousePath.stripSuffix("/"))
} finally {
sparkContext.conf.remove("spark.sql.default.warehouse.dir")
sparkContext.conf.remove("spark.sql.warehouse.dir")
}

// Set sql.warehouse.dir but not sql.default.warehouse.dir
try {
val newWarehouse = "spark-warehouse4"
val newWarehousePath = new Path(Utils.resolveURI(newWarehouse)).toString
sparkContext.conf.set("spark.sql.warehouse.dir", newWarehousePath)
val spark = new SparkSession(sparkContext)
assert(newWarehousePath.stripSuffix("/") === spark
.sessionState.conf.warehousePath.stripSuffix("/"))
assert(newWarehousePath.stripSuffix("/") === spark
.sharedState.warehousePath.stripSuffix("/"))
} finally {
sparkContext.conf.remove("spark.sql.warehouse.dir")
}

// Set neither of the two configs. The default value should be "spark-warehouse"

val spark = new SparkSession(sparkContext)
assert(new Path(Utils.resolveURI("spark-warehouse")).toString.stripSuffix("/") === spark
.sessionState.conf.warehousePath.stripSuffix("/"))
assert(new Path(Utils.resolveURI("spark-warehouse")).toString.stripSuffix("/") === spark
.sharedState.warehousePath.stripSuffix("/"))
}

test("MAX_CASES_BRANCHES") {
Expand Down