[SPARK-32401][SQL] Migrate function related commands to use UnresolvedFunc to resolve function identifier #29198

imback82 · 2020-07-23T04:09:08Z

What changes were proposed in this pull request?

This PR proposes to migrate the following function related commands to use UnresolvedFunc to resolve function identifier:

DROP FUNCTION
DESCRIBE FUNCTION
SHOW FUNCTIONS

DropFunctionStatement, DescribeFunctionStatement and ShowFunctionsStatement logical plans are replaced with DropFunction, DescribeFunction and ShowFunctions logical plans respectively, and each contains UnresolvedFunc as its child so that it can be resolved in Analyzer.

Why are the changes needed?

Migrating to the new resolution framework, which resolves UnresolvedFunc in Analyzer.

Does this PR introduce any user-facing change?

The message of exception thrown when a catalog is resolved to v2 has been merged to:
function is only supported in v1 catalog

Previously, it printed out the command used. E.g.,:
CREATE FUNCTION is only supported in v1 catalog

How was this patch tested?

Updated existing tests.

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala

SparkQA · 2020-07-23T06:29:12Z

Test build #126377 has finished for PR 29198 at commit 38756fc.

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

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

cloud-fan · 2020-07-24T04:41:35Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala

    val pattern = Option(ctx.pattern).map(string(_))
-    val functionName = Option(ctx.multipartIdentifier).map(visitMultipartIdentifier)
-    ShowFunctionsStatement(userScope, systemScope, pattern, functionName)
+    val functionName = Option(ctx.multipartIdentifier)


nit: maybe unresolvedFunc? it's not a name anymore.

or keep it functionName, and do

ShowFunctions(UnresolvedFunc(functionName), userScope, systemScope, pattern)

Changed it to unresolvedFuncOpt since functionName in the second suggestion will be Option[Seq[String]].

cloud-fan · 2020-07-24T04:47:39Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala

+    }
+
+    CreateFunction(
+      func,


I'm not sure how to deal with CREATE commands yet. For now, CREATE TABLE and VIEW still use statement plans. We don't need to do lookup for CREATE commands, so UnresolvedFunnc looks weird here.

Can we keep it unchanged?

Ok. Will revert this change.

SparkQA · 2020-07-24T23:54:46Z

Test build #126511 has finished for PR 29198 at commit 27f2fb1.

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

viirya

Would it be better to briefly describe "the new resolution framework" so the reviewers can look at this quickly?

viirya · 2020-07-26T05:39:17Z

sql/catalyst/src/main/scala/org/apache/spark/sql/connector/catalog/LookupCatalog.scala

        }

-      case _ => throw new AnalysisException(s"$sql is only supported in v1 catalog")
+      case _ => throw new AnalysisException("function is only supported in v1 catalog")


function -> function command?

This is called from Analyzer.scala except for CREATE FUNCTION, so I thought more general name would be better here. But if you think function command is better here, I am happy to change it. Please let me know!

imback82 · 2020-07-27T23:18:17Z

Would it be better to briefly describe "the new resolution framework" so the reviewers can look at this quickly?

Updated.

cloud-fan · 2020-07-28T10:16:47Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/v2Commands.scala

+    userScope: Boolean,
+    systemScope: Boolean,
+    pattern: Option[String]) extends Command {
+  override def children: Seq[LogicalPlan] = if (child.isDefined) { child.get :: Nil } else { Nil }


nit: child.toSeq

cloud-fan · 2020-07-28T10:18:09Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala

-    DropFunctionStatement(
-      functionName,
+    DropFunction(
+      UnresolvedFunc(functionName),


will analyzer give a nice error if UnresolvedFunc is left to CheckAnalysis?

nvm, this won't happen for now.

cloud-fan · 2020-07-28T10:24:15Z

The PR itself LGTM. @viirya the new framework is to resolve SQL objects (table, view, function, etc.) in the analyzer, not at runtime in RunnableCommand.run. See v2ResolutionPlans.scala.

SparkQA · 2020-07-28T21:36:01Z

Test build #126725 has finished for PR 29198 at commit c662619.

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

cloud-fan · 2020-07-29T03:56:04Z

thanks, merging to master!

imback82 added 2 commits July 22, 2020 20:59

initial commit

04f629d

spacing

38756fc

probot-autolabeler bot added the SQL label Jul 23, 2020

imback82 commented Jul 23, 2020

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala Outdated Show resolved Hide resolved

imback82 mentioned this pull request Jul 23, 2020

[SPARK-31999][SQL] Add REFRESH FUNCTION command #28840

Closed

cloud-fan reviewed Jul 24, 2020

View reviewed changes

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

cloud-fan reviewed Jul 24, 2020

View reviewed changes

imback82 added 4 commits July 24, 2020 11:30

Merge branch 'master' into function_framework

f149cf7

address PR comments

0e9f12f

remove unused import

daf4d6b

remove unused import

27f2fb1

viirya reviewed Jul 26, 2020

View reviewed changes

imback82 changed the title ~~[SPARK-32401][SQL] Migrate function related commands to new resolution framework~~ [SPARK-32401][SQL] Migrate function related commands to use UnresolvedFunc to resolve function identifier Jul 27, 2020

cloud-fan reviewed Jul 28, 2020

View reviewed changes

imback82 added 2 commits July 28, 2020 09:15

Merge branch 'master' into function_framework

7924572

Address PR comment

c662619

cloud-fan closed this in 45b7212 Jul 29, 2020

[SPARK-32401][SQL] Migrate function related commands to use UnresolvedFunc to resolve function identifier #29198

[SPARK-32401][SQL] Migrate function related commands to use UnresolvedFunc to resolve function identifier #29198

Uh oh!

Conversation

imback82 commented Jul 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

Uh oh!

SparkQA commented Jul 23, 2020

Uh oh!

Uh oh!

cloud-fan Jul 24, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

imback82 Jul 24, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jul 24, 2020

Uh oh!

viirya left a comment

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!

imback82 commented Jul 27, 2020

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!

cloud-fan commented Jul 28, 2020

Uh oh!

SparkQA commented Jul 28, 2020

Uh oh!

cloud-fan commented Jul 29, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

imback82 commented Jul 23, 2020 •

edited

Loading

cloud-fan Jul 24, 2020 •

edited

Loading

imback82 Jul 24, 2020 •

edited

Loading