[SPARK-25560][SQL] Allow FunctionInjection in SparkExtensions #22576

RussellSpitzer · 2018-09-28T02:05:33Z

This allows an implementer of Spark Session Extensions to utilize a
method "injectFunction" which will add a new function to the default
Spark Session Catalogue.

What changes were proposed in this pull request?

Adds a new function to SparkSessionExtensions

def injectFunction(functionDescription: FunctionDescription)

Where function description is a new type

type FunctionDescription = (FunctionIdentifier, FunctionBuilder)

The functions are loaded in BaseSessionBuilder when the function registry does not have a parent
function registry to get loaded from.

How was this patch tested?

New unit tests are added for the extension in SparkSessionExtensionSuite

RussellSpitzer · 2018-09-28T02:06:08Z

@hvanhovell Made a full PR for the change we discussed. Also updated the signature to match the new defined types for the registry and Identifier.

SparkQA · 2018-09-28T05:02:37Z

Test build #96728 has finished for PR 22576 at commit 5fdca38.

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

SparkQA · 2018-09-28T16:21:55Z

Test build #96757 has finished for PR 22576 at commit 450dd73.

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

RussellSpitzer · 2018-09-28T16:39:55Z

Looks like the session with extensions from the test suite is leaking to other suites ... Investigating

…

On Fri, Sep 28, 2018 at 11:25 AM UCB AMPLab ***@***.***> wrote: Test FAILed. Refer to this link for build results (access rights to CI server needed): https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/96757/ Test FAILed. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#22576 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAZNYTu9vCM3h2Z-SgFqxpB3PLKg5o7kks5ufk1igaJpZM4W9uz9> .

RussellSpitzer · 2018-09-28T17:32:28Z

Ah I was registering functions with the built-in registry which is not reset. I've changed it to register only with a clone of the built-in registry. This would allow multiple extensions, or some sessions to use extensions and others not to

SparkQA · 2018-09-28T21:26:55Z

Test build #96769 has finished for PR 22576 at commit 718c8ec.

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

hvanhovell · 2018-10-11T08:47:46Z

sql/core/src/main/scala/org/apache/spark/sql/SparkSessionExtensions.scala

+  private[this] val injectedFunctions =
+    mutable.Buffer.empty[FunctionDescription]
+
+    private[sql] def registerFunctions(functionRegistry: FunctionRegistry) = {


Can you fix the indentation for the next 14 lines?

hvanhovell · 2018-10-11T08:53:18Z

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

@@ -95,7 +95,8 @@ abstract class BaseSessionStateBuilder(
   * This either gets cloned from a pre-existing version or cloned from the built-in registry.
   */
  protected lazy val functionRegistry: FunctionRegistry = {
-    parentState.map(_.functionRegistry).getOrElse(FunctionRegistry.builtin).clone()
+    parentState.map(_.functionRegistry.clone())
+      .getOrElse{extensions.registerFunctions(FunctionRegistry.builtin.clone())}


NIT: Use parenthesis instead of curly braces?

hvanhovell · 2018-10-11T08:53:41Z

sql/core/src/main/scala/org/apache/spark/sql/SparkSessionExtensions.scala

@@ -168,4 +173,22 @@ class SparkSessionExtensions {
  def injectParser(builder: ParserBuilder): Unit = {
    parserBuilders += builder
  }
+
+  private[this] val injectedFunctions =


NIT: no new line?

hvanhovell

Couple of style remarks, but looks good in general.

This allows an implementer of Spark Session Extensions to utilize a method "injectFunction" which will add a new function to the default Spark Session Catalog.

RussellSpitzer · 2018-10-16T16:07:55Z

Cleaned up

SparkQA · 2018-10-16T19:42:00Z

Test build #97462 has finished for PR 22576 at commit 32e0a78.

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

HyukjinKwon · 2018-10-19T02:51:30Z

retest this please

SparkQA · 2018-10-19T06:32:02Z

Test build #97576 has finished for PR 22576 at commit 32e0a78.

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

hvanhovell · 2018-10-19T08:35:53Z

sql/core/src/main/scala/org/apache/spark/sql/SparkSessionExtensions.scala

+  private[this] val injectedFunctions = mutable.Buffer.empty[FunctionDescription]
+
+  private[sql] def registerFunctions(functionRegistry: FunctionRegistry) = {
+    for ((name, expressionInfo, function) <- injectedFunctions) {


Can you move the stuff that changes the FunctionRegistry into the BaseSessionStateBuilder and just make this return the Seq[FunctionDescription]? The return type of this function a FunctionRegistry sort of implies that you are getting back a new registry instead of a mutated one. If we are mutating then I prefer to do that in the BaseSessionBuilder so it is obvious that this is safe to do because we mutating a clone. It also makes this code more inline with the rest of the extension class (not mutating). Sorry for the late change of heart.

Ha we just changed a function in the opposite direction on my other commit. The project should probably pick one dorm and put it in the style guide. I'll make the chznge

hvanhovell · 2018-10-19T08:36:46Z

@RussellSpitzer I am merging this, can you address my comment in a follow up? Thanks!

This allows an implementer of Spark Session Extensions to utilize a method "injectFunction" which will add a new function to the default Spark Session Catalogue. ## What changes were proposed in this pull request? Adds a new function to SparkSessionExtensions def injectFunction(functionDescription: FunctionDescription) Where function description is a new type type FunctionDescription = (FunctionIdentifier, FunctionBuilder) The functions are loaded in BaseSessionBuilder when the function registry does not have a parent function registry to get loaded from. ## How was this patch tested? New unit tests are added for the extension in SparkSessionExtensionSuite Closes apache#22576 from RussellSpitzer/SPARK-25560. Authored-by: Russell Spitzer <Russell.Spitzer@gmail.com> Signed-off-by: Herman van Hovell <hvanhovell@databricks.com>

hvanhovell reviewed Oct 11, 2018

View reviewed changes

hvanhovell requested changes Oct 11, 2018

View reviewed changes

[SPARK-25560][SQL] Allow FunctionInjection in SparkExtensions

32e0a78

This allows an implementer of Spark Session Extensions to utilize a method "injectFunction" which will add a new function to the default Spark Session Catalog.

hvanhovell reviewed Oct 19, 2018

View reviewed changes

asfgit closed this in 6e0fc8b Oct 19, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-25560][SQL] Allow FunctionInjection in SparkExtensions #22576

[SPARK-25560][SQL] Allow FunctionInjection in SparkExtensions #22576

RussellSpitzer commented Sep 28, 2018

RussellSpitzer commented Sep 28, 2018

SparkQA commented Sep 28, 2018

SparkQA commented Sep 28, 2018

RussellSpitzer commented Sep 28, 2018 via email

RussellSpitzer commented Sep 28, 2018

SparkQA commented Sep 28, 2018

hvanhovell Oct 11, 2018

hvanhovell Oct 11, 2018 •

edited

Loading

hvanhovell Oct 11, 2018

hvanhovell left a comment

RussellSpitzer commented Oct 16, 2018

SparkQA commented Oct 16, 2018

HyukjinKwon commented Oct 19, 2018

SparkQA commented Oct 19, 2018

hvanhovell Oct 19, 2018

RussellSpitzer Oct 19, 2018

hvanhovell Oct 19, 2018

hvanhovell commented Oct 19, 2018

[SPARK-25560][SQL] Allow FunctionInjection in SparkExtensions #22576

[SPARK-25560][SQL] Allow FunctionInjection in SparkExtensions #22576

Conversation

RussellSpitzer commented Sep 28, 2018

What changes were proposed in this pull request?

How was this patch tested?

RussellSpitzer commented Sep 28, 2018

SparkQA commented Sep 28, 2018

SparkQA commented Sep 28, 2018

RussellSpitzer commented Sep 28, 2018 via email

RussellSpitzer commented Sep 28, 2018

SparkQA commented Sep 28, 2018

hvanhovell Oct 11, 2018

Choose a reason for hiding this comment

hvanhovell Oct 11, 2018 • edited Loading

Choose a reason for hiding this comment

hvanhovell Oct 11, 2018

Choose a reason for hiding this comment

hvanhovell left a comment

Choose a reason for hiding this comment

RussellSpitzer commented Oct 16, 2018

SparkQA commented Oct 16, 2018

HyukjinKwon commented Oct 19, 2018

SparkQA commented Oct 19, 2018

hvanhovell Oct 19, 2018

Choose a reason for hiding this comment

RussellSpitzer Oct 19, 2018

Choose a reason for hiding this comment

hvanhovell Oct 19, 2018

Choose a reason for hiding this comment

hvanhovell commented Oct 19, 2018

hvanhovell Oct 11, 2018 •

edited

Loading