Skip to content

[SPARK-24544][SQL] Print actual failure cause when look up function failed #21790

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
caneGuy wants to merge 11 commits into from
Closed

[SPARK-24544][SQL] Print actual failure cause when look up function failed #21790

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
690035a
[SPARK-24544][SQL] Print actual failure cause when look up function f…
caneGuy Jul 17, 2018
e1866d1
Refine comment
caneGuy Dec 19, 2018
ec208e0
Add unit test
caneGuy Dec 27, 2018
26aeb58
Refine comment
caneGuy Dec 29, 2018
95ba6e7
Update style
caneGuy Dec 29, 2018
1bff63c
Remove useless import
caneGuy Dec 29, 2018
820faf2
Refine comment
caneGuy Dec 30, 2018
240f3ab
Refine
caneGuy Jan 1, 2019
6d3bd19
Refine
caneGuy Jan 1, 2019
97493c9
Refine
caneGuy Jan 1, 2019
d1d4adf
Add comment
caneGuy Jan 1, 2019
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
9 changes: 6 additions & 3 deletions sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/NoSuchItemException.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ package org.apache.spark.sql.catalyst.analysis

import org.apache.spark.sql.AnalysisException
import org.apache.spark.sql.catalyst.catalog.CatalogTypes.TablePartitionSpec
import org.apache.spark.util.Utils


/**
Expand All @@ -40,10 +41,12 @@ class NoSuchPartitionException(
class NoSuchPermanentFunctionException(db: String, func: String)
extends AnalysisException(s"Function '$func' not found in database '$db'")

class NoSuchFunctionException(db: String, func: String)
class NoSuchFunctionException(db: String, func: String, cause: Option[Throwable] = None)
extends AnalysisException(
s"Undefined function: '$func'. This function is neither a registered temporary function nor " +
s"a permanent function registered in the database '$db'.")
s"Undefined function: '$func'. This function is neither a registered temporary function nor " +
s"a permanent function registered in the database '$db'." +
s"${cause.map(th => s"Exception thrown during look up:" +
Copy link
Member

@srowen srowen Dec 27, 2018

Choose a reason for hiding this comment

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

This is too complex for string interpolation. But why not just set the exception's cause?

Copy link
Contributor Author

@caneGuy caneGuy Dec 28, 2018

Choose a reason for hiding this comment

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

@srowen Thanks.IIUC.Since some times the cause may be undefined.For that case, we do not need to print Exception thrown during look up.

Copy link
Member

@srowen srowen Dec 28, 2018

Choose a reason for hiding this comment

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

I mean, why not set the cause on AnalysisException? the cause may be null if the Option is None, that's fine.

Copy link
Contributor Author

@caneGuy caneGuy Dec 29, 2018

Choose a reason for hiding this comment

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

Update @srowen Thanks!

s" ${Utils.exceptionString(th)}").getOrElse("")}")

class NoSuchPartitionsException(db: String, table: String, specs: Seq[TablePartitionSpec])
extends AnalysisException(
Expand Down
5 changes: 3 additions & 2 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 @@ -1209,9 +1209,10 @@ class SessionCatalog(
databaseExists(db) && externalCatalog.functionExists(db, name.funcName)
}

protected def failFunctionLookup(name: FunctionIdentifier): Nothing = {
protected[sql] def failFunctionLookup(
name: FunctionIdentifier, cause: Option[Throwable] = None): Nothing = {
throw new NoSuchFunctionException(
db = name.database.getOrElse(getCurrentDatabase), func = name.funcName)
db = name.database.getOrElse(getCurrentDatabase), func = name.funcName, cause)
}

/**
Expand Down
8 changes: 8 additions & 0 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 @@ -1445,4 +1445,12 @@ abstract class SessionCatalogSuite extends AnalysisTest {
}
}
}

test("SPARK-24544: test print actual failure cause when look up function failed") {
withBasicCatalog { catalog =>
intercept[NoSuchFunctionException] {
Copy link
Member

@srowen srowen Dec 31, 2018

Choose a reason for hiding this comment

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

Does this test anything about the cause of the exception? maybe get the cause and assert about its message?

catalog.failFunctionLookup(FunctionIdentifier("failureFunc"), Some(new Exception("Test")))
}
}
}
}
9 changes: 6 additions & 3 deletions sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveSessionCatalog.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ import org.apache.spark.sql.catalyst.parser.ParserInterface
import org.apache.spark.sql.hive.HiveShim.HiveFunctionWrapper
import org.apache.spark.sql.internal.SQLConf
import org.apache.spark.sql.types.{DecimalType, DoubleType}
import org.apache.spark.util.Utils


private[sql] class HiveSessionCatalog(
Expand Down Expand Up @@ -141,21 +142,23 @@ private[sql] class HiveSessionCatalog(
// built-in function.
// Hive is case insensitive.
val functionName = funcName.unquotedString.toLowerCase(Locale.ROOT)
logWarning(s"Encounter a failure during looking up function:" +
Copy link
Member

@srowen srowen Dec 29, 2018

Choose a reason for hiding this comment

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

Nit: Encounter -> Encountered, and the first string doesn't need interpolation

Copy link
Contributor Author

@caneGuy caneGuy Dec 30, 2018

Choose a reason for hiding this comment

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

Thanks @srowen Updated!

s" ${Utils.exceptionString(error)}")
if (!hiveFunctions.contains(functionName)) {
failFunctionLookup(funcName)
failFunctionLookup(funcName, Some(error))
}

// TODO: Remove this fallback path once we implement the list of fallback functions
// defined below in hiveFunctions.
val functionInfo = {
try {
Option(HiveFunctionRegistry.getFunctionInfo(functionName)).getOrElse(
failFunctionLookup(funcName))
failFunctionLookup(funcName, Some(error)))
} catch {
// If HiveFunctionRegistry.getFunctionInfo throws an exception,
// we are failing to load a Hive builtin function, which means that
// the given function is not a Hive builtin function.
case NonFatal(e) => failFunctionLookup(funcName)
case NonFatal(e) => failFunctionLookup(funcName, Some(e))
}
}
val className = functionInfo.getFunctionClass.getName
Expand Down