[SPARK-39579][SQL][PYTHON][R] Make ListFunctions/getFunction/functionExists compatible with 3 layer namespace #36977

zhengruifeng · 2022-06-24T13:12:24Z

What changes were proposed in this pull request?

Make ListFunctions/getFunction/functionExists compatible with 3 layer namespace

Why are the changes needed?

to support 3 layer namespace

Does this PR introduce any user-facing change?

yes

How was this patch tested?

added UT

zhengruifeng · 2022-06-30T13:15:46Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala

@@ -2139,16 +2139,16 @@ class Analyzer(override val catalogManager: CatalogManager)
    }

    def lookupBuiltinOrTempFunction(name: Seq[String]): Option[ExpressionInfo] = {
-      if (name.length == 1) {
-        v1SessionCatalog.lookupBuiltinOrTempFunction(name.head)
+      if (name.length == 1 || name.length == 3) {


to support look up temp function with a 3-layer ident.

zhengruifeng · 2022-07-01T01:29:04Z

all tests passed, just rebase to fix conflicts

zhengruifeng · 2022-07-01T01:29:33Z

cc @cloud-fan @amaliujia could you please take a look when you find some time?

cloud-fan · 2022-07-01T01:43:47Z

@zhengruifeng sorry it has conflicts now...

zhengruifeng · 2022-07-01T01:53:01Z

@cloud-fan dont worry, let me update the pr

cloud-fan · 2022-07-01T02:53:00Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala

@@ -2080,7 +2080,7 @@ class Analyzer(override val catalogManager: CatalogManager)
            throw QueryCompilationErrors.expectPersistentFuncError(
              nameParts.head, cmd, mismatchHint, u)
          } else {
-            ResolvedNonPersistentFunc(nameParts.head, V1Function(info))
+            ResolvedNonPersistentFunc(nameParts.last, V1Function(info))


is it a bug?

I think it should be a bug. should always be the name:

/** * A plan containing resolved non-persistent (temp or built-in) function. */ case class ResolvedNonPersistentFunc( name: String, func: UnboundFunction) extends LeafNodeWithoutStats { override def output: Seq[Attribute] = Nil }

can we have a separate PR to fix this bug?

sorry, my change was confusing, it is not a bug:

lookupBuiltinOrTempFunction(nameParts) .orElse(lookupBuiltinOrTempTableFunction(nameParts))

could be non-empty only when nameparts.length == 1, so nameParts.head is fine.

cloud-fan · 2022-07-01T02:53:40Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala

@@ -2139,16 +2139,16 @@ class Analyzer(override val catalogManager: CatalogManager)
    }

    def lookupBuiltinOrTempFunction(name: Seq[String]): Option[ExpressionInfo] = {
-      if (name.length == 1) {
-        v1SessionCatalog.lookupBuiltinOrTempFunction(name.head)
+      if (name.length == 1 || name.length == 3) {


why is this change?

~~to look up temp function with name = spark_default.default.add~~

to look up temp function with name = spark_default.default.+

let me try to handle system functions and user functions seperately, then the changes in Analyzer could be reverted.

cloud-fan · 2022-07-01T02:54:41Z

sql/core/src/main/scala/org/apache/spark/sql/internal/CatalogImpl.scala

+    // a qualified namespace with catalog name. We assume it's a single database name
+    // and check if we can find the dbName in sessionCatalog. If so we listFunctions under
+    // that database. Otherwise we try 3-part name parsing and locate the database.
+    if (sessionCatalog.databaseExists(dbName) || sessionCatalog.isGlobalTempViewDB(dbName)) {


we don't need to check global temp view db, as we are listing functions.

cloud-fan · 2022-07-01T02:56:32Z

sql/core/src/main/scala/org/apache/spark/sql/internal/CatalogImpl.scala

  }

  private def makeFunction(funcIdent: FunctionIdentifier): Function = {
-    val metadata = sessionCatalog.lookupFunctionInfo(funcIdent)
+    val metadata = try {
+      Some(sessionCatalog.getFunctionMetadata(funcIdent))


why do we make this change?

let me revert this

cloud-fan · 2022-07-01T09:19:11Z

sql/core/src/main/scala/org/apache/spark/sql/internal/CatalogImpl.scala

+        val ident = sparkSession.sessionState.sqlParser.parseMultipartIdentifier(functionName)
+        val catalog =
+          sparkSession.sessionState.catalogManager.catalog(ident(0)).asFunctionCatalog
+        catalog.functionExists(Identifier.of(Array(ident(1)), ident(2)))


This assumes the first name part is always the catalog, which is not the case. We may use the current catalog. I think we should rely on the analyzer and use UnresolvedFunc.

init add class name fix ut add getFunction & functionExists add ut for testcat update the py side nit

zhengruifeng · 2022-07-04T01:04:32Z

@cloud-fan could you please take another look? thanks!

also cc @HyukjinKwon if you are intrested in this.

zhengruifeng · 2022-07-05T00:33:45Z

gentle ping @cloud-fan

cloud-fan · 2022-07-05T03:11:02Z

python/pyspark/sql/catalog.py

@@ -288,19 +301,75 @@ def functionExists(self, functionName: str, dbName: Optional[str] = None) -> boo
            name of the database to check function existence in.
            If no database is specified, the current database is used

+           .. deprecated:: 3.4.0


@amaliujia we need to do the same deprecation in scala

ack will follow up on the scala side.

cloud-fan · 2022-07-05T03:16:55Z

thanks, merging to master!

zhengruifeng · 2022-07-05T03:29:35Z

@cloud-fan Thank you for reivew

HyukjinKwon

LGTM2

zhengruifeng · 2022-07-05T10:01:51Z

python/pyspark/sql/catalog.py

+                "a future version. Use functionExists(`dbName.tableName`) instead.",
+                FutureWarning,
+            )
+            return self._jcatalog.functionExists(self.currentDatabase(), functionName)


oops, here should be self._jcatalog.functionExists(dbName, functionName)

let me fix it in a follow up

…me) when dbName is not None ### What changes were proposed in this pull request? fix functionExists(functionName, dbName) ### Why are the changes needed? #36977 introduce a bug in `functionExists(functionName, dbName)`, when dbName is not None, should call `self._jcatalog.functionExists(dbName, functionName)` ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? existing testsuite Closes #37088 from zhengruifeng/py_3l_fix_functionExists. Authored-by: Ruifeng Zheng <ruifengz@apache.org> Signed-off-by: Ruifeng Zheng <ruifengz@apache.org>

github-actions bot added the SQL label Jun 24, 2022

zhengruifeng force-pushed the sql_3L_catalog_list_functions branch from aef63ca to 80ebb13 Compare June 28, 2022 08:20

github-actions bot added the R label Jun 28, 2022

zhengruifeng force-pushed the sql_3L_catalog_list_functions branch from f7acee4 to 67e812c Compare June 29, 2022 02:28

zhengruifeng changed the title ~~[SPARK-39579][SQL][WIP] Make ListFunctions compatible with 3 layer namespace~~ [SPARK-39579][SQL] Make ListFunctions compatible with 3 layer namespace Jun 29, 2022

zhengruifeng marked this pull request as ready for review June 29, 2022 10:12

zhengruifeng changed the title ~~[SPARK-39579][SQL] Make ListFunctions compatible with 3 layer namespace~~ [SPARK-39579][SQL] Make ListFunctions/getFunction/functionExists compatible with 3 layer namespace Jun 30, 2022

zhengruifeng force-pushed the sql_3L_catalog_list_functions branch from 67e812c to fe583c7 Compare June 30, 2022 02:44

zhengruifeng changed the title ~~[SPARK-39579][SQL] Make ListFunctions/getFunction/functionExists compatible with 3 layer namespace~~ [SPARK-39579][SQL][WIP] Make ListFunctions/getFunction/functionExists compatible with 3 layer namespace Jun 30, 2022

zhengruifeng force-pushed the sql_3L_catalog_list_functions branch from fe583c7 to eb98e66 Compare June 30, 2022 08:32

zhengruifeng changed the title ~~[SPARK-39579][SQL][WIP] Make ListFunctions/getFunction/functionExists compatible with 3 layer namespace~~ [SPARK-39579][SQL][PYTHON] Make ListFunctions/getFunction/functionExists compatible with 3 layer namespace Jun 30, 2022

github-actions bot added CORE PYTHON labels Jun 30, 2022

zhengruifeng force-pushed the sql_3L_catalog_list_functions branch from 775970c to 6d2e578 Compare June 30, 2022 13:12

zhengruifeng commented Jun 30, 2022

View reviewed changes

zhengruifeng changed the title ~~[SPARK-39579][SQL][PYTHON] Make ListFunctions/getFunction/functionExists compatible with 3 layer namespace~~ [SPARK-39579][SQL][PYTHON][R] Make ListFunctions/getFunction/functionExists compatible with 3 layer namespace Jun 30, 2022

zhengruifeng force-pushed the sql_3L_catalog_list_functions branch 2 times, most recently from b21756d to 93c8793 Compare July 1, 2022 01:28

zhengruifeng force-pushed the sql_3L_catalog_list_functions branch from 93c8793 to a3a50b9 Compare July 1, 2022 02:20

cloud-fan reviewed Jul 1, 2022

View reviewed changes

zhengruifeng force-pushed the sql_3L_catalog_list_functions branch from 1ca922c to 1a491a3 Compare July 1, 2022 07:03

cloud-fan reviewed Jul 1, 2022

View reviewed changes

init

51eaa63

init add class name fix ut add getFunction & functionExists add ut for testcat update the py side nit

zhengruifeng added 6 commits July 2, 2022 08:55

handles_system_user_sepearately

9b884ab

update comments

88a3297

fix py lint

9bba9d5

fix py lint II

5af2f94

address comments

d2c4fac

resolve conflicts & test show functions with testcat

4ea562c

zhengruifeng force-pushed the sql_3L_catalog_list_functions branch from ec0784b to 4ea562c Compare July 2, 2022 01:19

cloud-fan reviewed Jul 5, 2022

View reviewed changes

cloud-fan approved these changes Jul 5, 2022

View reviewed changes

cloud-fan closed this in a2c1038 Jul 5, 2022

zhengruifeng deleted the sql_3L_catalog_list_functions branch July 5, 2022 03:29

HyukjinKwon reviewed Jul 5, 2022

View reviewed changes

zhengruifeng commented Jul 5, 2022

View reviewed changes

zhengruifeng mentioned this pull request Jul 5, 2022

[SPARK-39579][PYTHON][FOLLOWUP] fix functionExists(functionName, dbName) when dbName is not None #37088

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-39579][SQL][PYTHON][R] Make ListFunctions/getFunction/functionExists compatible with 3 layer namespace #36977

[SPARK-39579][SQL][PYTHON][R] Make ListFunctions/getFunction/functionExists compatible with 3 layer namespace #36977

zhengruifeng commented Jun 24, 2022 •

edited

zhengruifeng Jun 30, 2022

zhengruifeng commented Jul 1, 2022

zhengruifeng commented Jul 1, 2022

cloud-fan commented Jul 1, 2022

zhengruifeng commented Jul 1, 2022

cloud-fan Jul 1, 2022

zhengruifeng Jul 1, 2022

cloud-fan Jul 1, 2022

zhengruifeng Jul 1, 2022

zhengruifeng Jul 1, 2022

cloud-fan Jul 1, 2022

zhengruifeng Jul 1, 2022 •

edited

zhengruifeng Jul 1, 2022

cloud-fan Jul 1, 2022

cloud-fan Jul 1, 2022

zhengruifeng Jul 1, 2022

cloud-fan Jul 1, 2022

zhengruifeng commented Jul 4, 2022

zhengruifeng commented Jul 5, 2022

cloud-fan Jul 5, 2022

amaliujia Jul 5, 2022

cloud-fan commented Jul 5, 2022

zhengruifeng commented Jul 5, 2022

HyukjinKwon left a comment

zhengruifeng Jul 5, 2022

[SPARK-39579][SQL][PYTHON][R] Make ListFunctions/getFunction/functionExists compatible with 3 layer namespace #36977

[SPARK-39579][SQL][PYTHON][R] Make ListFunctions/getFunction/functionExists compatible with 3 layer namespace #36977

Conversation

zhengruifeng commented Jun 24, 2022 • edited

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?

Choose a reason for hiding this comment

zhengruifeng commented Jul 1, 2022

zhengruifeng commented Jul 1, 2022

cloud-fan commented Jul 1, 2022

zhengruifeng commented Jul 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhengruifeng Jul 1, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhengruifeng commented Jul 4, 2022

zhengruifeng commented Jul 5, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cloud-fan commented Jul 5, 2022

zhengruifeng commented Jul 5, 2022

HyukjinKwon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhengruifeng commented Jun 24, 2022 •

edited

zhengruifeng Jul 1, 2022 •

edited