[SPARK-23122][PYTHON][SQL] Deprecate register* for UDFs in SQLContext and Catalog in PySpark #20288

HyukjinKwon · 2018-01-17T04:15:11Z

What changes were proposed in this pull request?

This PR proposes to deprecate register* for UDFs in SQLContext and Catalog in Spark 2.3.0.

These are inconsistent with Scala / Java APIs and also these basically do the same things with spark.udf.register*.

Also, this PR moves the logcis from [sqlContext|spark.catalog].register* to spark.udf.register* and reuse the docstring.

This PR also handles minor doc corrections. It also includes #20158

How was this patch tested?

Manually tested, manually checked the API documentation and tests added to check if deprecated APIs call the aliases correctly.

HyukjinKwon · 2018-01-17T04:17:15Z

@ueshin and @icexelloss (docstring reassignment) and @cloud-fan (deprecation), could you guys take a look and see if I understood your suggestions correctly?

SparkQA · 2018-01-17T04:55:59Z

Test build #86234 has finished for PR 20288 at commit f63105c.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class UDFRegistration(object):

SparkQA · 2018-01-17T06:18:58Z

Test build #86247 has finished for PR 20288 at commit 08438ee.

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

cloud-fan · 2018-01-17T06:20:44Z

python/pyspark/sql/session.py

@@ -778,6 +778,146 @@ def __exit__(self, exc_type, exc_val, exc_tb):
        self.stop()


+class UDFRegistration(object):


shall we put it in udf.py?

cloud-fan · 2018-01-17T06:25:45Z

python/pyspark/sql/context.py

@@ -624,6 +536,9 @@ def _test():
    globs['os'] = os
    globs['sc'] = sc
    globs['sqlContext'] = SQLContext(sc)
+    # 'spark' alias is a small hack for reusing doctests. Please see the reassignment
+    # of docstrings above.
+    globs['spark'] = globs['sqlContext']


shall we do globs['spark'] = globs['sqlContext'].sparkSession?

cloud-fan · 2018-01-17T06:26:40Z

python/pyspark/sql/session.py

+        self.sparkSession = sparkSession
+
+    @ignore_unicode_prefix
+    def register(self, name, f, returnType=None):


shall we add since 2.3?

viirya · 2018-01-17T06:16:20Z

python/pyspark/sql/catalog.py

+    registerFunction.__doc__ = """%s
+        .. note:: :func:`spark.catalog.registerFunction` is an alias
+            for :func:`spark.udf.register`.
+        .. note:: Deprecated in 2.3.0. Use :func:`spark.udf.register` instead.


Do we have any plan (e.g. 3.0.0) to remove this alias?

I think we should remove this out WDYT @cloud-fan?

viirya · 2018-01-17T06:17:49Z

python/pyspark/sql/context.py

@@ -147,7 +147,8 @@ def udf(self):

        :return: :class:`UDFRegistration`
        """
-        return UDFRegistration(self)
+        from pyspark.sql.session import UDFRegistration


Why we import UDFRegistration here again? Isn't it imported at the top?

viirya · 2018-01-17T06:37:12Z

python/pyspark/sql/session.py

+            [Row(stringLengthInt(test)=4)]
+
+        2. When `f` is a user-defined function, Spark uses the return type of the given a
+        user-defined function as the return type of the registered a user-defined function.


the registered a user-defined function -> the registered user-defined function

viirya · 2018-01-17T06:37:33Z

python/pyspark/sql/session.py

+            >>> spark.sql("SELECT stringLengthInt('test')").collect()
+            [Row(stringLengthInt(test)=4)]
+
+        2. When `f` is a user-defined function, Spark uses the return type of the given a


of the given a user-defined function -> of the given user-defined function

ueshin · 2018-01-17T07:03:52Z

python/pyspark/sql/context.py

@@ -147,7 +147,8 @@ def udf(self):

        :return: :class:`UDFRegistration`
        """
-        return UDFRegistration(self)
+        from pyspark.sql.session import UDFRegistration
+        return UDFRegistration(self.sparkSession)


How about return self.sparkSession.udf?

HyukjinKwon · 2018-01-17T08:28:21Z

Will try to double check and clean up soon. Comments so far above look all valid.

HyukjinKwon · 2018-01-17T12:08:33Z

python/pyspark/sql/udf.py

@@ -181,3 +183,180 @@ def asNondeterministic(self):
        """
        self.deterministic = False
        return self
+
+
+class UDFRegistration(object):


This seems introduced from 1.3.1 - https://issues.apache.org/jira/browse/SPARK-6603

HyukjinKwon · 2018-01-17T12:09:45Z

python/pyspark/sql/udf.py

+
+    @ignore_unicode_prefix
+    @since(2.3)
+    def registerJavaFunction(self, name, javaClassName, returnType=None):


registerJavaFunction and registerJavaUDAF look introduced from 2.3.0 - https://issues.apache.org/jira/browse/SPARK-19439

HyukjinKwon · 2018-01-17T12:10:33Z

python/pyspark/sql/context.py

+            :func:`spark.udf.registerJavaFunction`
+        .. note:: Deprecated in 2.3.0. Use :func:`spark.udf.registerJavaFunction` instead.
+        .. versionadded:: 2.1
+    """ % _registerJavaFunction_doc[:_registerJavaFunction_doc.rfind('versionadded::')]


We are fine to use this rfind way because since decorator adds versionadded:: at the end.

HyukjinKwon · 2018-01-17T12:11:08Z

python/pyspark/sql/context.py


-    @ignore_unicode_prefix
-    @since(2.3)
-    def registerJavaUDAF(self, name, javaClassName):


We are fine to remove this one because this is added within 2.3.0 timeline - https://issues.apache.org/jira/browse/SPARK-19439

HyukjinKwon · 2018-01-17T12:24:57Z

python/pyspark/sql/catalog.py

+            for :func:`spark.udf.register`.
+        .. note:: Deprecated in 2.3.0. Use :func:`spark.udf.register` instead.
+        .. versionadded:: 2.0
+    """ % _register_doc[:_register_doc.rfind('versionadded::')]


HyukjinKwon · 2018-01-17T12:25:24Z

python/pyspark/sql/context.py

+            :func:`spark.udf.register`.
+        .. note:: Deprecated in 2.3.0. Use :func:`spark.udf.register` instead.
+        .. versionadded:: 1.2
+    """ % _register_doc[:_register_doc.rfind('versionadded::')]


HyukjinKwon · 2018-01-17T12:26:14Z

python/pyspark/sql/context.py

+            :func:`spark.udf.registerJavaFunction`
+        .. note:: Deprecated in 2.3.0. Use :func:`spark.udf.registerJavaFunction` instead.
+        .. versionadded:: 2.1
+    """ % _registerJavaFunction_doc[:_registerJavaFunction_doc.rfind('versionadded::')]


HyukjinKwon · 2018-01-17T12:26:48Z

python/pyspark/sql/udf.py

+
+            .. note:: Registration for a user-defined function (case 2.) was added from
+                Spark 2.3.0.
+        """


HyukjinKwon · 2018-01-17T12:27:13Z

python/pyspark/sql/udf.py

+        ...   "test.org.apache.spark.sql.JavaStringLength")
+        >>> spark.sql("SELECT javaStringLength2('test')").collect()
+        [Row(UDF:javaStringLength2(test)=4)]
+        """


ueshin · 2018-01-17T12:53:11Z

LGTM pending Jenkins.

cloud-fan · 2018-01-17T13:48:01Z

LGTM

HyukjinKwon · 2018-01-17T14:08:50Z

Just fixed minor doc nits and double checked the built API documentation.

viirya · 2018-01-17T14:09:07Z

LGTM

SparkQA · 2018-01-17T15:38:38Z

Test build #86266 has finished for PR 20288 at commit c6ed44a.

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

icexelloss · 2018-01-17T15:43:52Z

python/pyspark/sql/catalog.py

+            DeprecationWarning)
+        return self._sparkSession.udf.register(name, f, returnType)
+    # Reuse the docstring from UDFRegistration but with few notes.
+    _register_doc = UDFRegistration.register.__doc__.strip()


An alternative is to do sth like:

An alias for :func:`spark.udf.register` .. note:: Deprecated in 2.3.0. Use :func:`spark.udf.register` instead. .. versionadded:: 2.0

So we don't need to copy the docstring.

But I am fine either way. @HyukjinKwon you can decide.

Oh, wait .. I think this is another good alternative. Will double check and be back today.

icexelloss · 2018-01-17T15:44:21Z

LGTM

SparkQA · 2018-01-17T15:53:36Z

Test build #86264 has finished for PR 20288 at commit 6b9b9c4.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class UDFRegistration(object):

SparkQA · 2018-01-17T16:02:28Z

Test build #86265 has finished for PR 20288 at commit f1fe40a.

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

SparkQA · 2018-01-17T17:59:31Z

Test build #86273 has finished for PR 20288 at commit 08ffa1c.

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

SparkQA · 2018-01-17T18:02:05Z

Test build #86274 has finished for PR 20288 at commit 4367beb.

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

HyukjinKwon

Hey @viirya, @ueshin and @cloud-fan, looks using links are cleaner and simpler. I think we can do such things in this way in the future too. I think we can also avoid duplicatedly running doctests too.

Let me know if you guys prefer the previous way. I can simply revert it. Last change made is basically - 3e0147b.

HyukjinKwon · 2018-01-18T01:41:54Z

python/pyspark/sql/context.py

@@ -29,9 +29,10 @@
 from pyspark.sql.readwriter import DataFrameReader
 from pyspark.sql.streaming import DataStreamReader
 from pyspark.sql.types import IntegerType, Row, StringType
+from pyspark.sql.udf import UDFRegistration


I intendedly kept this to retain the import path pyspark.sql.context.UDFRegistration just in case.

HyukjinKwon · 2018-01-18T01:46:32Z

python/pyspark/sql/context.py

+        """An alias for :func:`spark.udf.register`.
+        See :meth:`pyspark.sql.UDFRegistration.register`.
+
+        .. note:: Deprecated in 2.3.0. Use :func:`spark.udf.register` instead.


It shows the doc as below:

I checked the link, pyspark.sql.UDFRegistration.register is correct.

cloud-fan · 2018-01-18T02:57:29Z

LGTM

ueshin · 2018-01-18T03:32:17Z

LGTM pending Jenkins.

viirya

LGTM

SparkQA · 2018-01-18T05:32:20Z

Test build #86308 has finished for PR 20288 at commit c9512a6.

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

SparkQA · 2018-01-18T05:33:17Z

Test build #86306 has finished for PR 20288 at commit 3e0147b.

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

SparkQA · 2018-01-18T05:43:34Z

Test build #86309 has finished for PR 20288 at commit e121273.

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

ueshin · 2018-01-18T05:50:28Z

Thanks! merging to master/2.3.

… and Catalog in PySpark ## What changes were proposed in this pull request? This PR proposes to deprecate `register*` for UDFs in `SQLContext` and `Catalog` in Spark 2.3.0. These are inconsistent with Scala / Java APIs and also these basically do the same things with `spark.udf.register*`. Also, this PR moves the logcis from `[sqlContext|spark.catalog].register*` to `spark.udf.register*` and reuse the docstring. This PR also handles minor doc corrections. It also includes #20158 ## How was this patch tested? Manually tested, manually checked the API documentation and tests added to check if deprecated APIs call the aliases correctly. Author: hyukjinkwon <[email protected]> Closes #20288 from HyukjinKwon/deprecate-udf. (cherry picked from commit 39d244d) Signed-off-by: Takuya UESHIN <[email protected]>

gatorsmile · 2018-01-18T19:42:37Z

python/pyspark/sql/udf.py

+        :return: a user-defined function.
+
+        `returnType` can be optionally specified when `f` is a Python function but not
+        when `f` is a user-defined function. Please see below.


Could you add another paragraph for explaining how to register a non-deterministic Python function? This sounds a common question from end users.

gatorsmile · 2018-01-18T19:47:40Z

LGTM except one minor comment.

Could you submit a follow-up PR?

HyukjinKwon · 2018-01-22T06:37:01Z

@gatorsmile, I am sorry i don't know why I missed this comment ..

Deprecate register* for UDFs in SQLContext and Catalog in PySpark

f63105c

HyukjinKwon mentioned this pull request Jan 17, 2018

[PySpark] Fix typo in comments in PySpark's udf() definition #20158

Closed

Focus on the issue itself

08438ee

cloud-fan reviewed Jan 17, 2018

View reviewed changes

viirya reviewed Jan 17, 2018

View reviewed changes

ueshin reviewed Jan 17, 2018

View reviewed changes

HyukjinKwon added 2 commits January 17, 2018 20:58

Address comments and clean up

6b9b9c4

Clean up imports

f1fe40a

HyukjinKwon commented Jan 17, 2018

View reviewed changes

Minor doc fix

c6ed44a

HyukjinKwon commented Jan 17, 2018

View reviewed changes

python/pyspark/sql/udf.py

.. note:: Registration for a user-defined function (case 2.) was added from

Spark 2.3.0.

"""

Copy link

Member Author

HyukjinKwon Jan 17, 2018

Choose a reason for hiding this comment

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

HyukjinKwon commented Jan 17, 2018

View reviewed changes

HyukjinKwon mentioned this pull request Jan 17, 2018

[SPARK-22274][PYTHON][SQL] User-defined aggregation functions with pandas udf (full shuffle) #19872

Closed

HyukjinKwon added 2 commits January 17, 2018 23:05

Fix minor nits found

08ffa1c

one more space

4367beb

icexelloss reviewed Jan 17, 2018

View reviewed changes

HyukjinKwon added 4 commits January 18, 2018 10:40

Use link instead of doc copy

3e0147b

Clean up imports

c9512a6

Clean up imports

00f5d19

Clean up imports

e121273

HyukjinKwon commented Jan 18, 2018

View reviewed changes

viirya approved these changes Jan 18, 2018

View reviewed changes

asfgit closed this in 39d244d Jan 18, 2018

gatorsmile reviewed Jan 18, 2018

View reviewed changes

HyukjinKwon deleted the deprecate-udf branch October 16, 2018 12:44

		@@ -778,6 +778,146 @@ def __exit__(self, exc_type, exc_val, exc_tb):
		self.stop()


		class UDFRegistration(object):

[SPARK-23122][PYTHON][SQL] Deprecate register* for UDFs in SQLContext and Catalog in PySpark #20288

[SPARK-23122][PYTHON][SQL] Deprecate register* for UDFs in SQLContext and Catalog in PySpark #20288

Conversation

HyukjinKwon commented Jan 17, 2018 • edited Loading

What changes were proposed in this pull request?

How was this patch tested?

HyukjinKwon commented Jan 17, 2018

SparkQA commented Jan 17, 2018

SparkQA commented Jan 17, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HyukjinKwon commented Jan 17, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ueshin commented Jan 17, 2018

cloud-fan commented Jan 17, 2018

HyukjinKwon commented Jan 17, 2018

viirya commented Jan 17, 2018

SparkQA commented Jan 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

icexelloss commented Jan 17, 2018

SparkQA commented Jan 17, 2018

SparkQA commented Jan 17, 2018

SparkQA commented Jan 17, 2018

SparkQA commented Jan 17, 2018

HyukjinKwon left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cloud-fan commented Jan 18, 2018

ueshin commented Jan 18, 2018

viirya left a comment

Choose a reason for hiding this comment

SparkQA commented Jan 18, 2018

SparkQA commented Jan 18, 2018

SparkQA commented Jan 18, 2018

ueshin commented Jan 18, 2018

Choose a reason for hiding this comment

gatorsmile commented Jan 18, 2018

HyukjinKwon commented Jan 22, 2018

HyukjinKwon commented Jan 17, 2018 •

edited

Loading

HyukjinKwon left a comment •

edited

Loading