Skip to content
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

Jump to bottom

[SPARK-14410] [SQL] : SessionCatalog needs to check database/table/function existence #12183

Closed
wants to merge 9 commits into from
Closed

[SPARK-14410] [SQL] : SessionCatalog needs to check database/table/function existence #12183

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
e3677c9
Merge pull request #1 from apache/master
rekhajoshm May 5, 2015
106fd8e
Merge pull request #2 from apache/master
rekhajoshm May 8, 2015
0be142d
Merge pull request #3 from apache/master
rekhajoshm Jun 22, 2015
6c6ee12
Merge pull request #4 from apache/master
rekhajoshm Sep 17, 2015
b123c60
Merge pull request #5 from apache/master
rekhajoshm Nov 25, 2015
c73c32a
Merge pull request #6 from apache/master
rekhajoshm Mar 18, 2016
7dbf732
Merge pull request #8 from apache/master
rekhajoshm Apr 5, 2016
59c60a6
SessionCatalog function/database existence checks, metadata
rekhajoshm Apr 5, 2016
e040216
SessionCatalog function/database existence checks, metadata
rekhajoshm Apr 5, 2016
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
26 changes: 12 additions & 14 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 @@ -84,15 +84,21 @@ class SessionCatalog(
// ----------------------------------------------------------------------------

def createDatabase(dbDefinition: CatalogDatabase, ignoreIfExists: Boolean): Unit = {
externalCatalog.createDatabase(dbDefinition, ignoreIfExists)
if (!databaseExists(dbDefinition.name)) {
externalCatalog.createDatabase(dbDefinition, ignoreIfExists)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

For createDatabase, we should throw an exception if db already exists and ignoreIfExists is false, right?

}

def dropDatabase(db: String, ignoreIfNotExists: Boolean, cascade: Boolean): Unit = {
externalCatalog.dropDatabase(db, ignoreIfNotExists, cascade)
if (databaseExists(db)) {
externalCatalog.dropDatabase(db, ignoreIfNotExists, cascade)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This is now wrong. We used to pass the flag ignoreIfNotExists to the external catalog, which will handle the exception throwing. Now we'll ignore this flag if the database doesn't exist.

}

def alterDatabase(dbDefinition: CatalogDatabase): Unit = {
externalCatalog.alterDatabase(dbDefinition)
if (databaseExists(dbDefinition.name)) {
externalCatalog.alterDatabase(dbDefinition)
}
}

def getDatabase(db: String): CatalogDatabase = {
Expand Down Expand Up @@ -460,8 +466,7 @@ class SessionCatalog(
* If a database is specified in `name`, this will return the function in that database.
* If no database is specified, this will return the function in the current database.
*/
// TODO: have a better name. This method is actually for fetching the metadata of a function.
def getFunction(name: FunctionIdentifier): CatalogFunction = {
def getMetadataOfFunction(name: FunctionIdentifier): CatalogFunction = {
val db = name.database.getOrElse(currentDb)
externalCatalog.getFunction(db, name.funcName)
}
Expand All @@ -474,15 +479,8 @@ class SessionCatalog(
// This function exists in the FunctionRegistry.
true
} else {
// Need to check if this function exists in the metastore.
try {
// TODO: It's better to ask external catalog if this function exists.
// So, we can avoid of having this hacky try/catch block.
getFunction(name) != null
} catch {
case _: NoSuchFunctionException => false
case _: AnalysisException => false // HiveExternalCatalog wraps all exceptions with it.
}
val db = name.database.getOrElse(currentDb)
externalCatalog.listFunctions(db,name.unquotedString).size > 0
Copy link
Contributor

Choose a reason for hiding this comment

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

No, the right thing to do here is to implement functionExists in the external catalog and HiveClientImpl. That's what the TODO meant.

}
}

Expand Down
8 changes: 4 additions & 4 deletions sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalogSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -753,19 +753,19 @@ class SessionCatalogSuite extends SparkFunSuite {
val expected =
CatalogFunction(FunctionIdentifier("func1", Some("db2")), funcClass,
Seq.empty[(String, String)])
assert(catalog.getFunction(FunctionIdentifier("func1", Some("db2"))) == expected)
assert(catalog.getMetadataOfFunction(FunctionIdentifier("func1", Some("db2"))) == expected)
// Get function without explicitly specifying database
catalog.setCurrentDatabase("db2")
assert(catalog.getFunction(FunctionIdentifier("func1")) == expected)
assert(catalog.getMetadataOfFunction(FunctionIdentifier("func1")) == expected)
}

test("get function when database/function does not exist") {
val catalog = new SessionCatalog(newBasicCatalog())
intercept[AnalysisException] {
catalog.getFunction(FunctionIdentifier("func1", Some("does_not_exist")))
catalog.getMetadataOfFunction(FunctionIdentifier("func1", Some("does_not_exist")))
}
intercept[AnalysisException] {
catalog.getFunction(FunctionIdentifier("does_not_exist", Some("db2")))
catalog.getMetadataOfFunction(FunctionIdentifier("does_not_exist", Some("db2")))
}
}

Expand Down