[SPARK-29379][SQL]SHOW FUNCTIONS show '!=', '<>' , 'between', 'case' by AngersZhuuuu · Pull Request #26053 · apache/spark

AngersZhuuuu · 2019-10-08T07:52:32Z

What changes were proposed in this pull request?

Current Spark SQL SHOW FUNCTIONS don't show !=, <>, between, case
But these expressions is truly functions. We should show it in SQL SHOW FUNCTIONS

Why are the changes needed?

SHOW FUNCTIONS show '!=', '<>' , 'between', 'case'

Does this PR introduce any user-facing change?

SHOW FUNCTIONS show '!=', '<>' , 'between', 'case'

How was this patch tested?

UT

HyukjinKwon · 2019-10-08T12:25:07Z

Actually there are multiple instances (see https://spark.apache.org/docs/latest/api/sql/index.html). Can we remove them from the function list?

AngersZhuuuu · 2019-10-08T13:08:35Z

Actually there are multiple instances (see https://spark.apache.org/docs/latest/api/sql/index.html). Can we remove them from the function list?

Not clear what you mean.

HyukjinKwon · 2019-10-08T13:19:36Z

Oh do you mean =!, <>, etc. Should be a function? I think its operator, not a function, which actually should be removed from "function"s. cc @cloud-fan and @gatorsmile

AngersZhuuuu · 2019-10-08T13:23:40Z

Oh do you mean =!, <>, etc. Should be a function? I think its operator, not a function, which actually should be removed from "function"s. cc @cloud-fan and @gatorsmile

then =, == , < , etc should also be operator?
I want to add this since show functions in hive will get these four expression and desc function extended != is supported .

HyukjinKwon · 2019-10-08T13:41:22Z

Not sure if Hive resuls are correct and we dont have to follow the behaviours that dont make sense.

AngersZhuuuu · 2019-10-08T13:51:17Z

Not sure if Hive resuls are correct and we dont have to follow the behaviours that dont make sense.

Ok, since desc function !=/between/<>/case is supported, maybe we should keep consistent.

cloud-fan · 2019-10-08T14:08:52Z

Yea we should keep them consistent. Do you know why it's inconsistent now? Is it intentional?

AngersZhuuuu · 2019-10-08T14:11:50Z

Yea we should keep them consistent. Do you know why it's inconsistent now? Is it intentional?

Since !=/<>/between/case don't have expression class, they all change to other manifestation。

For desc function !=/between/<>/case, it was supported by hardcode:

spark/sql/core/src/main/scala/org/apache/spark/sql/execution/command/functions.scala

Line 116 in 9b8d63e

override def run(sparkSession: SparkSession): Seq[Row] = {

cloud-fan · 2019-10-08T15:53:25Z

sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala


-    checkAnswer(sql("SHOW functions"), getFunctions("*"))
+    checkAnswer(sql("SHOW functions"), (getFunctions("*") ++
+      Seq(Row("!="), Row("<>"), Row("between"), Row("case"))))


nit: shall we put this code in getFunctions?

nit: shall we put this code in getFunctions?

Good ideal, I will try this.

cloud-fan · 2019-10-08T15:53:59Z

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


shall we update it with val numFunctions = ... + 4?

HyukjinKwon · 2019-10-08T16:30:14Z

Keeping it consistent sounds fine for now but I think we should fix this ambiguity between functions and operators at the end; otherwise, we will keep facing an issue such as #24947

HyukjinKwon · 2019-10-08T16:33:13Z

ok to test

SparkQA · 2019-10-08T20:53:45Z

Test build #111907 has finished for PR 26053 at commit 9b8d63e.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2019-10-09T02:11:49Z

Test build #111929 has started for PR 26053 at commit b066088.

AngersZhuuuu · 2019-10-09T14:55:39Z

Jenkins is crashed ?

HyukjinKwon · 2019-10-10T00:55:06Z

retest this please

AngersZhuuuu · 2019-10-11T06:54:20Z

Keeping it consistent sounds fine for now but I think we should fix this ambiguity between functions and operators at the end; otherwise, we will keep facing an issue such as #24947

Do you have some suggestion about this problem, it's a big change.

HyukjinKwon · 2019-10-11T07:08:04Z

Alright, let's deal with it later separately.

AngersZhuuuu · 2019-10-12T02:55:06Z

@HyukjinKwon can you help to trigger retest for this PR, seems jenkins is ok.

HyukjinKwon · 2019-10-12T03:52:43Z

retest this please

SparkQA · 2019-10-12T07:05:02Z

Test build #111948 has finished for PR 26053 at commit b066088.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

wangyum · 2019-10-15T10:15:45Z

retest this please

SparkQA · 2019-10-15T13:58:12Z

Test build #112099 has finished for PR 26053 at commit b066088.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

AngersZhuuuu · 2019-10-16T01:33:53Z

@HyukjinKwon @cloud-fan
Any more advise

cloud-fan · 2019-10-16T07:01:59Z

sql/core/src/main/scala/org/apache/spark/sql/execution/command/functions.scala


+  // 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'")


what's the error message if users try to redefine =?

what's the error message if users try to redefine =?

can't use create function = ...., since = is a reserved key, we should use

create function `=` ....

Error message:

org.apache.hadoop.hive.ql.metadata.HiveException: InvalidObjectException(message:= is not a valid object name); org.apache.hadoop.hive.ql.metadata.HiveException: InvalidObjectException(message:> is not a valid object name); org.apache.hadoop.hive.ql.metadata.HiveException: InvalidObjectException(message:!= is not a valid object name); but `case` `between` can be registered.

as @HyukjinKwon methoned, we should fix this ambiguity between functions and operators at the end.

If we can't fix the problem completely here, let's keep it unchanged and fix them all together later.

If we can't fix the problem completely here, let's keep it unchanged and fix them all together later.

Ok, have remove these code.

SparkQA · 2019-10-16T07:05:02Z

Test build #112143 has finished for PR 26053 at commit 3d6c85d.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2019-10-16T12:36:31Z

sql/core/src/main/scala/org/apache/spark/sql/execution/command/functions.scala

    val catalog = sparkSession.sessionState.catalog
+
+    if (FunctionsCommand.virtualOperators.contains(functionName.toLowerCase(Locale.ROOT))) {
+      throw new AnalysisException(s"Cannot drop virtual function '$functionName'")


ditto, we should make sure the behavior is consistent with other operators.

ditto

OK,

drop function `!=\case\between\<>`

here it will through exception of function not found

SparkQA · 2019-10-16T12:45:42Z

Test build #112156 has finished for PR 26053 at commit 85556a6.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2019-10-16T13:30:18Z

sql/core/src/main/scala/org/apache/spark/sql/execution/command/functions.scala


  override def run(sparkSession: SparkSession): Seq[Row] = {
    val catalog = sparkSession.sessionState.catalog
+


Have you tried drop function '='? Does Spark fail with "function not found" or "Cannot drop native function"?

Have you tried drop function '='? Does Spark fail with "function not found" or "Cannot drop native function"?

Function not found.

Error in query: Function 'default.=' not found in database 'default';

OK this can be improved later. Let's leave it for now. Thanks for the investigation!

cloud-fan · 2019-10-16T14:52:36Z

sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala

    checkKeywordsExist(sql("describe functioN abcadf"), "Function: abcadf not found.")
  }

+  test("drop virtual functions") {


since the related changes are reverted, we should remove the test as well

This reverts commit 60cd8a8.

SparkQA · 2019-10-16T15:53:47Z

Test build #112168 has finished for PR 26053 at commit 22b3487.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2019-10-16T19:08:31Z

Test build #112174 has finished for PR 26053 at commit a285290.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2019-10-16T19:16:49Z

Test build #112177 has finished for PR 26053 at commit 9f68ead.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2019-10-17T05:10:59Z

sql/core/src/test/scala/org/apache/spark/sql/execution/command/DDLSuite.scala

  test("show functions") {
    withUserDefinedFunction("add_one" -> true) {
-      val numFunctions = FunctionRegistry.functionSet.size.toLong
+      val numFunctions = FunctionRegistry.functionSet.size.toLong + 4L


ah missed this one. We should use FunctionsCommand.virtualOperators.length to be future-proof.

ah missed this one. We should use FunctionsCommand.virtualOperators.length to be future-proof.

Good ideal, make it more clear. Done

cloud-fan · 2019-10-17T05:11:09Z

sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveUDFSuite.scala

          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)


ditto

Could you help to trigger retest ?

SparkQA · 2019-10-17T06:13:28Z

Test build #112199 has finished for PR 26053 at commit bc04f99.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2019-10-17T07:05:01Z

Test build #112201 has finished for PR 26053 at commit 7ac4d16.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2019-10-18T07:36:41Z

retest this please

SparkQA · 2019-10-18T11:21:03Z

Test build #112263 has finished for PR 26053 at commit 7ac4d16.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2019-10-18T16:20:09Z

thanks, merging to master!

AngersZhuuuu added 4 commits October 8, 2019 14:31

fix show functions

911007a

fix bug

ba22b62

fit SYSTEM/USER

074ee0e

fix UT

9b8d63e

cloud-fan reviewed Oct 8, 2019

View reviewed changes

dongjoon-hyun added the SQL label Oct 8, 2019

follow comment

b066088

follow comment

3d6c85d

cloud-fan reviewed Oct 16, 2019

View reviewed changes

follow comment

85556a6

cloud-fan reviewed Oct 16, 2019

View reviewed changes

follow comment

22b3487

cloud-fan reviewed Oct 16, 2019

View reviewed changes

cloud-fan approved these changes Oct 16, 2019

View reviewed changes

AngersZhuuuu added 4 commits October 16, 2019 23:08

Update SQLQuerySuite.scala

60cd8a8

Revert "Update SQLQuerySuite.scala"

522193c

This reverts commit 60cd8a8.

Update SQLQuerySuite.scala

a285290

remove empty line

9f68ead

cloud-fan reviewed Oct 17, 2019

View reviewed changes

follow commment

bc04f99

scalastyle

7ac4d16

cloud-fan closed this in 9a3dcca Oct 18, 2019


		override def run(sparkSession: SparkSession): Seq[Row] = {
		val catalog = sparkSession.sessionState.catalog

Conversation

AngersZhuuuu commented Oct 8, 2019

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

HyukjinKwon commented Oct 8, 2019

Uh oh!

AngersZhuuuu commented Oct 8, 2019

Uh oh!

HyukjinKwon commented Oct 8, 2019

Uh oh!

AngersZhuuuu commented Oct 8, 2019

Uh oh!

HyukjinKwon commented Oct 8, 2019

Uh oh!

AngersZhuuuu commented Oct 8, 2019

Uh oh!

cloud-fan commented Oct 8, 2019

Uh oh!

AngersZhuuuu commented Oct 8, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon commented Oct 8, 2019

Uh oh!

HyukjinKwon commented Oct 8, 2019

Uh oh!

SparkQA commented Oct 8, 2019

Uh oh!

SparkQA commented Oct 9, 2019

Uh oh!

AngersZhuuuu commented Oct 9, 2019

Uh oh!

HyukjinKwon commented Oct 10, 2019

Uh oh!

AngersZhuuuu commented Oct 11, 2019

Uh oh!

HyukjinKwon commented Oct 11, 2019

Uh oh!

AngersZhuuuu commented Oct 12, 2019

Uh oh!

HyukjinKwon commented Oct 12, 2019

Uh oh!

SparkQA commented Oct 12, 2019

Uh oh!

wangyum commented Oct 15, 2019

Uh oh!

SparkQA commented Oct 15, 2019

Uh oh!

AngersZhuuuu commented Oct 16, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Oct 16, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AngersZhuuuu Oct 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Oct 16, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AngersZhuuuu Oct 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

AngersZhuuuu Oct 16, 2019 •

edited

Loading

AngersZhuuuu Oct 16, 2019 •

edited

Loading