[GitHub] spark pull request #18544: [SPARK-21318][SQL]Improve exception message throw...

[asfgit]

Github user asfgit closed the pull request at:

https://github.com/apache/spark/pull/18544


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #18544: [SPARK-21318][SQL]Improve exception message throw...

[cloud-fan]

Github user cloud-fan commented on a diff in the pull request:

https://github.com/apache/spark/pull/18544#discussion_r219505379
  
--- Diff: 
sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveSessionCatalog.scala ---
@@ -131,14 +131,14 @@ private[sql] class HiveSessionCatalog(
 Try(super.lookupFunction(funcName, children)) match {
   case Success(expr) => expr
   case Failure(error) =>
-if (functionRegistry.functionExists(funcName)) {
-  // If the function actually exists in functionRegistry, it means 
that there is an
-  // error when we create the Expression using the given children.
+if (super.functionExists(name)) {
+  // If the function actually exists in functionRegistry or 
externalCatalog,
+  // it means that there is an error when we create the Expression 
using the given children.
   // We need to throw the original exception.
   throw error
 } else {
-  // This function is not in functionRegistry, let's try to load 
it as a Hive's
-  // built-in function.
+  // This function is not in functionRegistry or externalCatalog,
--- End diff --

`This function does not exist(neither in functionRegistry or 
externalCatalog) ...`


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #18544: [SPARK-21318][SQL]Improve exception message throw...

[cloud-fan]

Github user cloud-fan commented on a diff in the pull request:

https://github.com/apache/spark/pull/18544#discussion_r219505121
  
--- Diff: 
sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveSessionCatalog.scala ---
@@ -131,14 +131,14 @@ private[sql] class HiveSessionCatalog(
 Try(super.lookupFunction(funcName, children)) match {
   case Success(expr) => expr
   case Failure(error) =>
-if (functionRegistry.functionExists(funcName)) {
-  // If the function actually exists in functionRegistry, it means 
that there is an
-  // error when we create the Expression using the given children.
+if (super.functionExists(name)) {
+  // If the function actually exists in functionRegistry or 
externalCatalog,
--- End diff --

`If the function exists (either in functionRegistry or externalCatalog), 
...`


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #18544: [SPARK-21318][SQL]Improve exception message throw...

[stanzhai]

Github user stanzhai commented on a diff in the pull request:

https://github.com/apache/spark/pull/18544#discussion_r219485843
  
--- Diff: sql/hive/src/test/scala/org/apache/spark/sql/hive/UDFSuite.scala 
---
@@ -193,4 +193,29 @@ class UDFSuite
   }
 }
   }
+
+  test("SPARK-21318: The correct exception message should be thrown " +
+"if a UDF/UDAF has already been registered") {
+val UDAFName = "empty"
+val UDAFClassName = 
classOf[org.apache.spark.sql.hive.execution.UDAFEmpty].getCanonicalName
+
+withTempDatabase { dbName =>
--- End diff --

@cloud-fan I just copied and modified the code from another test case, the 
default database works well.

The test case has been simplified now.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #18544: [SPARK-21318][SQL]Improve exception message throw...

[stanzhai]

Github user stanzhai commented on a diff in the pull request:

https://github.com/apache/spark/pull/18544#discussion_r219468948
  
--- Diff: 
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalogSuite.scala
 ---
@@ -1440,6 +1441,8 @@ abstract class SessionCatalogSuite extends 
AnalysisTest {
 }
 
 assert(cause.getMessage.contains("Undefined function: 
'undefined_fn'"))
+// SPARK-21318: the error message should contains the current 
database name
--- End diff --

org.apache.spark.sql.AnalysisException: Undefined function: 'undefined_fn'. 
This function is neither a registered temporary function nor a permanent 
function registered in the database 'db1'.;


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #18544: [SPARK-21318][SQL]Improve exception message throw...

[cloud-fan]

Github user cloud-fan commented on a diff in the pull request:

https://github.com/apache/spark/pull/18544#discussion_r219413664
  
--- Diff: sql/hive/src/test/scala/org/apache/spark/sql/hive/UDFSuite.scala 
---
@@ -193,4 +193,29 @@ class UDFSuite
   }
 }
   }
+
+  test("SPARK-21318: The correct exception message should be thrown " +
+"if a UDF/UDAF has already been registered") {
+val UDAFName = "empty"
+val UDAFClassName = 
classOf[org.apache.spark.sql.hive.execution.UDAFEmpty].getCanonicalName
+
+withTempDatabase { dbName =>
--- End diff --

why do we have to test it inside a database? can't the default database 
work?


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #18544: [SPARK-21318][SQL]Improve exception message throw...

[cloud-fan]

Github user cloud-fan commented on a diff in the pull request:

https://github.com/apache/spark/pull/18544#discussion_r219412088
  
--- Diff: 
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalogSuite.scala
 ---
@@ -1440,6 +1441,8 @@ abstract class SessionCatalogSuite extends 
AnalysisTest {
 }
 
 assert(cause.getMessage.contains("Undefined function: 
'undefined_fn'"))
+// SPARK-21318: the error message should contains the current 
database name
--- End diff --

what's the full error message?


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #18544: [SPARK-21318][SQL]Improve exception message throw...

[stanzhai]

Github user stanzhai commented on a diff in the pull request:

https://github.com/apache/spark/pull/18544#discussion_r202607295
  
--- Diff: 
sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveSessionCatalog.scala ---
@@ -129,14 +129,14 @@ private[sql] class HiveSessionCatalog(
 Try(super.lookupFunction(funcName, children)) match {
   case Success(expr) => expr
   case Failure(error) =>
-if (functionRegistry.functionExists(funcName)) {
-  // If the function actually exists in functionRegistry, it means 
that there is an
-  // error when we create the Expression using the given children.
+if (super.functionExists(name)) {
+  // If the function actually exists in functionRegistry or 
externalCatalog,
+  // it means that there is an error when we create the Expression 
using the given children.
   // We need to throw the original exception.
   throw error
 } else {
-  // This function is not in functionRegistry, let's try to load 
it as a Hive's
-  // built-in function.
+  // This function is not in functionRegistry or externalCatalog,
+  // let's try to load it as a Hive's built-in function.
   // Hive is case insensitive.
   val functionName = 
funcName.unquotedString.toLowerCase(Locale.ROOT)
   if (!hiveFunctions.contains(functionName)) {
--- End diff --

Yes, that's right.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #18544: [SPARK-21318][SQL]Improve exception message throw...

[gatorsmile]

Github user gatorsmile commented on a diff in the pull request:

https://github.com/apache/spark/pull/18544#discussion_r202259987
  
--- Diff: 
sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveSessionCatalog.scala ---
@@ -129,14 +129,14 @@ private[sql] class HiveSessionCatalog(
 Try(super.lookupFunction(funcName, children)) match {
   case Success(expr) => expr
   case Failure(error) =>
-if (functionRegistry.functionExists(funcName)) {
-  // If the function actually exists in functionRegistry, it means 
that there is an
-  // error when we create the Expression using the given children.
+if (super.functionExists(name)) {
+  // If the function actually exists in functionRegistry or 
externalCatalog,
+  // it means that there is an error when we create the Expression 
using the given children.
   // We need to throw the original exception.
   throw error
 } else {
-  // This function is not in functionRegistry, let's try to load 
it as a Hive's
-  // built-in function.
+  // This function is not in functionRegistry or externalCatalog,
+  // let's try to load it as a Hive's built-in function.
   // Hive is case insensitive.
   val functionName = 
funcName.unquotedString.toLowerCase(Locale.ROOT)
   if (!hiveFunctions.contains(functionName)) {
--- End diff --

We do not need to change the other parts. We just need to throw the 
exception in `failFunctionLookup(funcName)`, right?


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #18544: [SPARK-21318][SQL]Improve exception message throw...

[gatorsmile]

Github user gatorsmile commented on a diff in the pull request:

https://github.com/apache/spark/pull/18544#discussion_r202257183
  
--- Diff: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
 ---
@@ -1155,7 +1155,8 @@ class Analyzer(
 override def apply(plan: LogicalPlan): LogicalPlan = 
plan.transformAllExpressions {
   case f: UnresolvedFunction if !catalog.functionExists(f.name) =>
 withPosition(f) {
-  throw new 
NoSuchFunctionException(f.name.database.getOrElse("default"), f.name.funcName)
+  val db = f.name.database.getOrElse(catalog.getCurrentDatabase)
+  throw new NoSuchFunctionException(db, f.name.funcName)
--- End diff --

The issue has been resolved. Can you revert the changes?


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #18544: [SPARK-21318][SQL]Improve exception message throw...

[stanzhai]

Github user stanzhai commented on a diff in the pull request:

https://github.com/apache/spark/pull/18544#discussion_r201579348
  
--- Diff: 
sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveSessionCatalog.scala ---
@@ -129,14 +129,14 @@ private[sql] class HiveSessionCatalog(
 Try(super.lookupFunction(funcName, children)) match {
   case Success(expr) => expr
   case Failure(error) =>
-if (functionRegistry.functionExists(funcName)) {
-  // If the function actually exists in functionRegistry, it means 
that there is an
-  // error when we create the Expression using the given children.
+if (super.functionExists(name)) {
--- End diff --

We should keep use `super.functionExists(name)`, we can not load  a Hive's 
built-in function  if replaced by `functionExists(name)` and 
`org.apache.spark.sql.AnalysisException: Undefined function: 
'histogram_numeric'` will be thrown.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #18544: [SPARK-21318][SQL]Improve exception message throw...

[gatorsmile]

Github user gatorsmile commented on a diff in the pull request:

https://github.com/apache/spark/pull/18544#discussion_r201157527
  
--- Diff: 
sql/hive/src/test/java/org/apache/spark/sql/hive/execution/UDAFEmpty.java ---
@@ -0,0 +1,32 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.spark.sql.hive.execution;
+
+import org.apache.hadoop.hive.ql.parse.SemanticException;
+import org.apache.hadoop.hive.ql.udf.generic.AbstractGenericUDAFResolver;
+import org.apache.hadoop.hive.ql.udf.generic.GenericUDAFEvaluator;
+import org.apache.hadoop.hive.serde2.typeinfo.TypeInfo;
+
+/**
+ * An empty UDAF that throws a semantic exception
+ */
+public class UDAFEmpty extends AbstractGenericUDAFResolver {
+@Override
+public GenericUDAFEvaluator getEvaluator(TypeInfo[] info) throws 
SemanticException {
+throw new SemanticException("Can not get a evaluator of the empty 
UDAF");
--- End diff --

Nit: `a evaluator` -> `an evaluator`


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #18544: [SPARK-21318][SQL]Improve exception message throw...

[gatorsmile]

Github user gatorsmile commented on a diff in the pull request:

https://github.com/apache/spark/pull/18544#discussion_r201157378
  
--- Diff: 
sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveSessionCatalog.scala ---
@@ -129,14 +129,14 @@ private[sql] class HiveSessionCatalog(
 Try(super.lookupFunction(funcName, children)) match {
   case Success(expr) => expr
   case Failure(error) =>
-if (functionRegistry.functionExists(funcName)) {
-  // If the function actually exists in functionRegistry, it means 
that there is an
-  // error when we create the Expression using the given children.
+if (super.functionExists(name)) {
--- End diff --

This should be replaced by `functionExists(name)`


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #18544: [SPARK-21318][SQL]Improve exception message throw...

[stanzhai]

GitHub user stanzhai opened a pull request:

https://github.com/apache/spark/pull/18544

[SPARK-21318][SQL]Improve exception message thrown by `lookupFunction`

## What changes were proposed in this pull request?

The function actually exists in current selected database, and it's failed 
to init during `lookupFunciton`, but the exception message is: 
```
This function is neither a registered temporary function nor a permanent 
function registered in the database 'default'.
```

This is not conducive to positioning problems. This PR fix the problem.

## How was this patch tested?

Exists tests + manual tests

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/stanzhai/spark fix-udf-error-message

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/spark/pull/18544.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #18544


commit 373fc5cacb77bb6e6be02eb3608497cbcaa7edef
Author: Stan Zhai 
Date:   2017-07-05T14:41:02Z

optimized udf lookup exception message




---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org