[GitHub] [spark] cloud-fan commented on a change in pull request #26053: [SPARK-29379][SQL]SHOW FUNCTIONS show '!=', '<>' , 'between', 'case'
cloud-fan commented on a change in pull request #26053: [SPARK-29379][SQL]SHOW
FUNCTIONS show '!=', '<>' , 'between', 'case'
URL: https://github.com/apache/spark/pull/26053#discussion_r335811752
##
File path:
sql/core/src/test/scala/org/apache/spark/sql/execution/command/DDLSuite.scala
##
@@ -2064,7 +2064,7 @@ abstract class DDLSuite extends QueryTest with
SQLTestUtils {
test("show functions") {
withUserDefinedFunction("add_one" -> true) {
- val numFunctions = FunctionRegistry.functionSet.size.toLong
+ val numFunctions = FunctionRegistry.functionSet.size.toLong + 4L
Review comment:
ah missed this one. We should use `FunctionsCommand.virtualOperators.length`
to be future-proof.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #26053: [SPARK-29379][SQL]SHOW FUNCTIONS show '!=', '<>' , 'between', 'case'
cloud-fan commented on a change in pull request #26053: [SPARK-29379][SQL]SHOW
FUNCTIONS show '!=', '<>' , 'between', 'case'
URL: https://github.com/apache/spark/pull/26053#discussion_r335811775
##
File path:
sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveUDFSuite.scala
##
@@ -563,7 +563,7 @@ class HiveUDFSuite extends QueryTest with
TestHiveSingleton with SQLTestUtils {
checkAnswer(
sql("SELECT testUDFToListInt(s) FROM inputTable"),
Seq(Row(Seq(1, 2, 3
-assert(sql("show functions").count() == numFunc + 1)
+assert(sql("show functions").count() == numFunc + 5)
Review comment:
ditto
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #26053: [SPARK-29379][SQL]SHOW FUNCTIONS show '!=', '<>' , 'between', 'case'
cloud-fan commented on a change in pull request #26053: [SPARK-29379][SQL]SHOW
FUNCTIONS show '!=', '<>' , 'between', 'case'
URL: https://github.com/apache/spark/pull/26053#discussion_r335526097
##
File path: sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala
##
@@ -111,6 +113,19 @@ class SQLQuerySuite extends QueryTest with
SharedSparkSession {
checkKeywordsExist(sql("describe functioN abcadf"), "Function: abcadf not
found.")
}
+ test("drop virtual functions") {
+val e1 = intercept[AnalysisException] {
+ sql(
+"drop function case")
Review comment:
nit: put the `sql("...")` in one line
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #26053: [SPARK-29379][SQL]SHOW FUNCTIONS show '!=', '<>' , 'between', 'case'
cloud-fan commented on a change in pull request #26053: [SPARK-29379][SQL]SHOW
FUNCTIONS show '!=', '<>' , 'between', 'case'
URL: https://github.com/apache/spark/pull/26053#discussion_r335526394
##
File path: sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala
##
@@ -111,6 +113,19 @@ class SQLQuerySuite extends QueryTest with
SharedSparkSession {
checkKeywordsExist(sql("describe functioN abcadf"), "Function: abcadf not
found.")
}
+ test("drop virtual functions") {
Review comment:
since the related changes are reverted, we should remove the test as well
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #26053: [SPARK-29379][SQL]SHOW FUNCTIONS show '!=', '<>' , 'between', 'case'
cloud-fan commented on a change in pull request #26053: [SPARK-29379][SQL]SHOW
FUNCTIONS show '!=', '<>' , 'between', 'case'
URL: https://github.com/apache/spark/pull/26053#discussion_r335526097
##
File path: sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala
##
@@ -111,6 +113,19 @@ class SQLQuerySuite extends QueryTest with
SharedSparkSession {
checkKeywordsExist(sql("describe functioN abcadf"), "Function: abcadf not
found.")
}
+ test("drop virtual functions") {
+val e1 = intercept[AnalysisException] {
+ sql(
+"drop function case")
Review comment:
nit: put the `sql("...")` in one line
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #26053: [SPARK-29379][SQL]SHOW FUNCTIONS show '!=', '<>' , 'between', 'case'
cloud-fan commented on a change in pull request #26053: [SPARK-29379][SQL]SHOW
FUNCTIONS show '!=', '<>' , 'between', 'case'
URL: https://github.com/apache/spark/pull/26053#discussion_r335525544
##
File path:
sql/core/src/main/scala/org/apache/spark/sql/execution/command/functions.scala
##
@@ -170,6 +171,7 @@ case class DropFunctionCommand(
override def run(sparkSession: SparkSession): Seq[Row] = {
val catalog = sparkSession.sessionState.catalog
+
Review comment:
OK this can be improved later. Let's leave it for now. Thanks for the
investigation!
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #26053: [SPARK-29379][SQL]SHOW FUNCTIONS show '!=', '<>' , 'between', 'case'
cloud-fan commented on a change in pull request #26053: [SPARK-29379][SQL]SHOW
FUNCTIONS show '!=', '<>' , 'between', 'case'
URL: https://github.com/apache/spark/pull/26053#discussion_r335475168
##
File path:
sql/core/src/main/scala/org/apache/spark/sql/execution/command/functions.scala
##
@@ -170,6 +171,7 @@ case class DropFunctionCommand(
override def run(sparkSession: SparkSession): Seq[Row] = {
val catalog = sparkSession.sessionState.catalog
+
Review comment:
Have you tried `drop function '='`? Does Spark fail with "function not
found" or "Cannot drop native function"?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #26053: [SPARK-29379][SQL]SHOW FUNCTIONS show '!=', '<>' , 'between', 'case'
cloud-fan commented on a change in pull request #26053: [SPARK-29379][SQL]SHOW
FUNCTIONS show '!=', '<>' , 'between', 'case'
URL: https://github.com/apache/spark/pull/26053#discussion_r335447140
##
File path:
sql/core/src/main/scala/org/apache/spark/sql/execution/command/functions.scala
##
@@ -170,6 +171,11 @@ case class DropFunctionCommand(
override def run(sparkSession: SparkSession): Seq[Row] = {
val catalog = sparkSession.sessionState.catalog
+
+if
(FunctionsCommand.virtualOperators.contains(functionName.toLowerCase(Locale.ROOT)))
{
+ throw new AnalysisException(s"Cannot drop virtual function
'$functionName'")
Review comment:
ditto, we should make sure the behavior is consistent with other operators.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #26053: [SPARK-29379][SQL]SHOW FUNCTIONS show '!=', '<>' , 'between', 'case'
cloud-fan commented on a change in pull request #26053: [SPARK-29379][SQL]SHOW
FUNCTIONS show '!=', '<>' , 'between', 'case'
URL: https://github.com/apache/spark/pull/26053#discussion_r335327656
##
File path:
sql/core/src/main/scala/org/apache/spark/sql/execution/command/functions.scala
##
@@ -73,6 +73,11 @@ case class CreateFunctionCommand(
s"is not allowed: '${databaseName.get}'")
}
+ // Redefine a virtual function is not allowed
+ if
(FunctionsCommand.virtualOperators.contains(functionName.toLowerCase(Locale.ROOT)))
{
+throw new AnalysisException(s"It's not allowed to redefine virtual
function '$functionName'")
Review comment:
If we can't fix the problem completely here, let's keep it unchanged and fix
them all together later.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #26053: [SPARK-29379][SQL]SHOW FUNCTIONS show '!=', '<>' , 'between', 'case'
cloud-fan commented on a change in pull request #26053: [SPARK-29379][SQL]SHOW
FUNCTIONS show '!=', '<>' , 'between', 'case'
URL: https://github.com/apache/spark/pull/26053#discussion_r335306089
##
File path:
sql/core/src/main/scala/org/apache/spark/sql/execution/command/functions.scala
##
@@ -73,6 +73,11 @@ case class CreateFunctionCommand(
s"is not allowed: '${databaseName.get}'")
}
+ // Redefine a virtual function is not allowed
+ if
(FunctionsCommand.virtualOperators.contains(functionName.toLowerCase(Locale.ROOT)))
{
+throw new AnalysisException(s"It's not allowed to redefine virtual
function '$functionName'")
Review comment:
what's the error message if users try to redefine `=`?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #26053: [SPARK-29379][SQL]SHOW FUNCTIONS show '!=', '<>' , 'between', 'case'
cloud-fan commented on a change in pull request #26053: [SPARK-29379][SQL]SHOW
FUNCTIONS show '!=', '<>' , 'between', 'case'
URL: https://github.com/apache/spark/pull/26053#discussion_r335268319
##
File path: sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala
##
@@ -59,7 +59,8 @@ class SQLQuerySuite extends QueryTest with
SharedSparkSession {
test("show functions") {
def getFunctions(pattern: String): Seq[Row] = {
StringUtils.filterPattern(
-
spark.sessionState.catalog.listFunctions("default").map(_._1.funcName), pattern)
+spark.sessionState.catalog.listFunctions("default").map(_._1.funcName)
+++ Seq("!=", "<>", "between", "case"), pattern)
Review comment:
`Seq("!=", "<>", "between", "case")` appears many times, shall we put it in
```
object ShowFunctionsCommand {
// operators that do not have corresponding functions.
// They should be handled in `DropFunctionCommand` as well.
val virtualOperators = Seq("!=", "<>", "between", "case")
}
```
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #26053: [SPARK-29379][SQL]SHOW FUNCTIONS show '!=', '<>' , 'between', 'case'
cloud-fan commented on a change in pull request #26053: [SPARK-29379][SQL]SHOW
FUNCTIONS show '!=', '<>' , 'between', 'case'
URL: https://github.com/apache/spark/pull/26053#discussion_r335267938
##
File path:
sql/core/src/main/scala/org/apache/spark/sql/execution/command/functions.scala
##
@@ -222,6 +223,13 @@ case class ShowFunctionsCommand(
case (f, "USER") if showUserFunctions => f.unquotedString
case (f, "SYSTEM") if showSystemFunctions => f.unquotedString
}
-functionNames.sorted.map(Row(_))
+if (showSystemFunctions) {
+ (functionNames ++
+StringUtils.filterPattern(Seq("!=", "<>", "between", "case"),
pattern.getOrElse("*")))
Review comment:
shall we add comment like
https://github.com/apache/spark/blob/9b8d63e7ce76740eb0fc08fcd2b24849ff7693be/sql/core/src/main/scala/org/apache/spark/sql/execution/command/functions.scala#L117
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #26053: [SPARK-29379][SQL]SHOW FUNCTIONS show '!=', '<>' , 'between', 'case'
cloud-fan commented on a change in pull request #26053: [SPARK-29379][SQL]SHOW
FUNCTIONS show '!=', '<>' , 'between', 'case'
URL: https://github.com/apache/spark/pull/26053#discussion_r332594494
##
File path:
sql/core/src/test/scala/org/apache/spark/sql/execution/command/DDLSuite.scala
##
@@ -2065,14 +2065,14 @@ abstract class DDLSuite extends QueryTest with
SQLTestUtils {
test("show functions") {
withUserDefinedFunction("add_one" -> true) {
val numFunctions = FunctionRegistry.functionSet.size.toLong
Review comment:
shall we update it with `val numFunctions = ... + 4`?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #26053: [SPARK-29379][SQL]SHOW FUNCTIONS show '!=', '<>' , 'between', 'case'
cloud-fan commented on a change in pull request #26053: [SPARK-29379][SQL]SHOW
FUNCTIONS show '!=', '<>' , 'between', 'case'
URL: https://github.com/apache/spark/pull/26053#discussion_r332594197
##
File path: sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala
##
@@ -80,13 +80,16 @@ class SQLQuerySuite extends QueryTest with
SharedSparkSession {
createFunction(functions)
-checkAnswer(sql("SHOW functions"), getFunctions("*"))
+checkAnswer(sql("SHOW functions"), (getFunctions("*") ++
+ Seq(Row("!="), Row("<>"), Row("between"), Row("case"
Review comment:
nit: shall we put this code in `getFunctions`?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
