Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[SPARK-37202][SQL] Treat injectedFunctions as custom built-in functions #34528

Closed
wants to merge 4 commits into from
Closed

[SPARK-37202][SQL] Treat injectedFunctions as custom built-in functions #34528

wants to merge 4 commits into from

Conversation

linhongliu-db
Copy link
Contributor

@linhongliu-db linhongliu-db commented Nov 9, 2021 •
edited
Loading

What changes were proposed in this pull request?

This PR fixes an issue that the injectedFunctions in SparkSessionExtensions can't be
resolved correctly if it's referred by a temporary view. This is because the injected functions
are not UserDefinedExpression. So they will be treated as temporary functions but couldn't
be captured as temporary functions during view creation. As a result, the function resolution
will fail if it's reffered by a temp view.

This PR adds a new concept customBuiltinFunction, so that the injected functions will be
treated as builtin functions intead of tempoary ones. With with way, it's not needed to be
captured by temp view anymore.

Why are the changes needed?

bug fix

Does this PR introduce any user-facing change?

After this PR, the SparkSessionExtensions.injectedFunctions are not temporary functions anymore.

val extensions = create { extensions =>
  extensions.injectFunction(MyExtensions.myFunction)
}
withSession(extensions) { session =>
  // return `false` after this PR, and `true` before this PR
  session.sessionState.catalog.isTemporaryFunction(MyExtensions.myFunction._1)
}

How was this patch tested?

newly added test

@github-actions github-actions bot added the SQL label Nov 9, 2021
@linhongliu-db linhongliu-db marked this pull request as draft November 9, 2021 05:18
linhongliu-db
linhongliu-db commented Nov 9, 2021
View reviewed changes
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionRegistry.scala
builder: FunctionBuilder): Unit = {
// We didn't do any check when replacing a function even if it's a builtin one.
// So no need to do any extra check here.
if (name.database.isEmpty) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we actually shouldn't allow function with a database here. Not sure if it's necessary to add a check here because it may break existing use cases.

linhongliu-db
linhongliu-db commented Nov 9, 2021
View reviewed changes
sql/core/src/main/scala/org/apache/spark/sql/execution/command/functions.scala
@@ -171,6 +171,9 @@ case class DropFunctionCommand(
if (FunctionRegistry.builtin.functionExists(FunctionIdentifier(functionName))) {
throw QueryCompilationErrors.cannotDropNativeFuncError(functionName)
}
if (catalog.isCustomBuiltinFunction(FunctionIdentifier(functionName))) {
throw QueryCompilationErrors.cannotDropCustomBuiltInFuncError(functionName)
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dropping a custom built-in function is fine. but it's weird if we can drop a built-in function.

@SparkQA
Copy link

SparkQA commented Nov 9, 2021

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/49491/

@SparkQA
Copy link

SparkQA commented Nov 9, 2021

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/49491/

@SparkQA
Copy link

SparkQA commented Nov 9, 2021

Test build #145017 has finished for PR 34528 at commit 6221140.

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

@SparkQA
Copy link

SparkQA commented Nov 9, 2021

Test build #145019 has finished for PR 34528 at commit 889cb25.

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

@jerqi
Copy link
Contributor

jerqi commented Nov 9, 2021

but fix

typo? but fix -> bug fix?

@SparkQA
Copy link

SparkQA commented Nov 10, 2021

Kubernetes integration test unable to build dist.

exiting with code: 1
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/49519/

@SparkQA
Copy link

SparkQA commented Nov 10, 2021

Kubernetes integration test unable to build dist.

exiting with code: 1
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/49521/

@SparkQA
Copy link

SparkQA commented Nov 10, 2021

Test build #145048 has finished for PR 34528 at commit 5547056.

  • This patch fails from timeout after a configured wait of 500m.
  • This patch merges cleanly.
  • This patch adds no public classes.

@linhongliu-db
Copy link
Contributor Author

close as I made another PR #34546 to fix this

@SparkQA
Copy link

SparkQA commented Nov 10, 2021

Test build #145050 has finished for PR 34528 at commit 48ffd81.

  • This patch fails from timeout after a configured wait of 500m.
  • This patch merges cleanly.
  • This patch adds no public classes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
SQL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@linhongliu-db @SparkQA @jerqi