-
Notifications
You must be signed in to change notification settings - Fork 29.3k
[SPARK-22106][PYSPARK][SQL] Disable 0-parameter pandas_udf and add doctests #19325
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.
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
Changes from 4 commits
c0eec8d
7b0da10
56a8409
6dc89b0
6fc639a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3256,11 +3256,20 @@ def test_vectorized_udf_null_string(self): | |
|
|
||
| def test_vectorized_udf_zero_parameter(self): | ||
| from pyspark.sql.functions import pandas_udf | ||
| import pandas as pd | ||
| df = self.spark.range(10) | ||
| f0 = pandas_udf(lambda **kwargs: pd.Series(1).repeat(kwargs['length']), LongType()) | ||
| res = df.select(f0()) | ||
| self.assertEquals(df.select(lit(1)).collect(), res.collect()) | ||
| error_str = '0-parameter pandas_udfs.*not.*supported' | ||
| with QuietTest(self.sc): | ||
| with self.assertRaisesRegexp(NotImplementedError, error_str): | ||
| pandas_udf(lambda: 1, LongType()) | ||
|
|
||
| with self.assertRaisesRegexp(NotImplementedError, error_str): | ||
| @pandas_udf | ||
| def zero_no_type(): | ||
| return 1 | ||
|
|
||
| with self.assertRaisesRegexp(NotImplementedError, error_str): | ||
| @pandas_udf(LongType()) | ||
| def zero_with_type(): | ||
| return 1 | ||
|
|
||
| def test_vectorized_udf_datatype_string(self): | ||
| from pyspark.sql.functions import pandas_udf, col | ||
|
|
@@ -3308,12 +3317,12 @@ def test_vectorized_udf_invalid_length(self): | |
| from pyspark.sql.functions import pandas_udf, col | ||
| import pandas as pd | ||
| df = self.spark.range(10) | ||
| raise_exception = pandas_udf(lambda: pd.Series(1), LongType()) | ||
| raise_exception = pandas_udf(lambda _: pd.Series(1), LongType()) | ||
| with QuietTest(self.sc): | ||
| with self.assertRaisesRegexp( | ||
| Exception, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here too, while we are here, let's catch narrower exception type. Looks
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This one is actually a |
||
| 'Result vector from pandas_udf was not the required length'): | ||
| df.select(raise_exception()).collect() | ||
| df.select(raise_exception(col('id'))).collect() | ||
|
|
||
| def test_vectorized_udf_mix_udf(self): | ||
| from pyspark.sql.functions import pandas_udf, udf, col | ||
|
|
@@ -3328,22 +3337,44 @@ def test_vectorized_udf_mix_udf(self): | |
|
|
||
| def test_vectorized_udf_chained(self): | ||
| from pyspark.sql.functions import pandas_udf, col | ||
| df = self.spark.range(10).toDF('x') | ||
| df = self.spark.range(10) | ||
| f = pandas_udf(lambda x: x + 1, LongType()) | ||
| g = pandas_udf(lambda x: x - 1, LongType()) | ||
| res = df.select(g(f(col('x')))) | ||
| res = df.select(g(f(col('id')))) | ||
| self.assertEquals(df.collect(), res.collect()) | ||
|
|
||
| def test_vectorized_udf_wrong_return_type(self): | ||
| from pyspark.sql.functions import pandas_udf, col | ||
| df = self.spark.range(10).toDF('x') | ||
| df = self.spark.range(10) | ||
| f = pandas_udf(lambda x: x * 1.0, StringType()) | ||
| with QuietTest(self.sc): | ||
| with self.assertRaisesRegexp( | ||
| Exception, | ||
| 'Invalid.*type.*string'): | ||
| df.select(f(col('x'))).collect() | ||
| with self.assertRaisesRegexp(Exception, 'Invalid.*type.*string'): | ||
| df.select(f(col('id'))).collect() | ||
|
|
||
| def test_vectorized_udf_return_scalar(self): | ||
| from pyspark.sql.functions import pandas_udf, col | ||
| df = self.spark.range(10) | ||
| f = pandas_udf(lambda x: 1.0, DoubleType()) | ||
| with QuietTest(self.sc): | ||
| with self.assertRaisesRegexp(Exception, 'Return.*type.*pandas_udf.*Series'): | ||
| df.select(f(col('id'))).collect() | ||
|
|
||
| def test_vectorized_udf_decorator(self): | ||
| from pyspark.sql.functions import pandas_udf, col | ||
| df = self.spark.range(10) | ||
|
|
||
| @pandas_udf(returnType=LongType()) | ||
| def identity(x): | ||
| return x | ||
| res = df.select(identity(col('id'))) | ||
| self.assertEquals(df.collect(), res.collect()) | ||
|
|
||
| def test_vectorized_udf_empty_partition(self): | ||
| from pyspark.sql.functions import pandas_udf, col | ||
| df = self.spark.createDataFrame(self.sc.parallelize([Row(id=1)], 2)) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe I miss something, but what this test is intended to test?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh. I see. One partition is empty and it is related to the added stuff in
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, an empty partition leads to an empty iterator, so this is to make sure it can handle that. |
||
| f = pandas_udf(lambda x: x, LongType()) | ||
| res = df.select(f(col('id'))) | ||
| self.assertEquals(df.collect(), res.collect()) | ||
|
|
||
| if __name__ == "__main__": | ||
| from pyspark.sql.tests import * | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -60,12 +60,9 @@ def read_command(serializer, file): | |
| return command | ||
|
|
||
|
|
||
| def chain(f, g, eval_type): | ||
| def chain(f, g): | ||
| """chain two functions together """ | ||
| if eval_type == PythonEvalType.SQL_PANDAS_UDF: | ||
| return lambda *a, **kwargs: g(f(*a, **kwargs), **kwargs) | ||
| else: | ||
| return lambda *a: g(f(*a)) | ||
| return lambda *a: g(f(*a)) | ||
|
|
||
|
|
||
| def wrap_udf(f, return_type): | ||
|
|
@@ -80,14 +77,14 @@ def wrap_pandas_udf(f, return_type): | |
| arrow_return_type = toArrowType(return_type) | ||
|
|
||
| def verify_result_length(*a): | ||
| kwargs = a[-1] | ||
| result = f(*a[:-1], **kwargs) | ||
| if len(result) != kwargs["length"]: | ||
| result = f(*a) | ||
| if not hasattr(result, "__len__"): | ||
| raise TypeError("Return type of pandas_udf should be a Pandas.Series") | ||
| if len(result) != len(a[0]): | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess we are not guaranteed to have
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we verify the returned is a Pandas.Series?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point. We should probably have a test that returns a scalar value too. I'm not sure we should limit the return type so much. As long as pyarrow can consume it, then it should be ok - it can also take a numpy array which might be useful. Otherwise it should raise a clear exception. Maybe checking that it has |
||
| raise RuntimeError("Result vector from pandas_udf was not the required length: " | ||
| "expected %d, got %d\nUse input vector length or kwargs['length']" | ||
| % (kwargs["length"], len(result))) | ||
| return result, arrow_return_type | ||
| return lambda *a: verify_result_length(*a) | ||
| "expected %d, got %d" % (len(a[0]), len(result))) | ||
| return result | ||
| return lambda *a: (verify_result_length(*a), arrow_return_type) | ||
|
|
||
|
|
||
| def read_single_udf(pickleSer, infile, eval_type): | ||
|
|
@@ -99,11 +96,9 @@ def read_single_udf(pickleSer, infile, eval_type): | |
| if row_func is None: | ||
| row_func = f | ||
| else: | ||
| row_func = chain(row_func, f, eval_type) | ||
| row_func = chain(row_func, f) | ||
| # the last returnType will be the return type of UDF | ||
| if eval_type == PythonEvalType.SQL_PANDAS_UDF: | ||
| # A pandas_udf will take kwargs as the last argument | ||
| arg_offsets = arg_offsets + [-1] | ||
| return arg_offsets, wrap_pandas_udf(row_func, return_type) | ||
| else: | ||
| return arg_offsets, wrap_udf(row_func, return_type) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -51,10 +51,12 @@ case class ArrowEvalPythonExec(udfs: Seq[PythonUDF], output: Seq[Attribute], chi | |
| outputIterator.map(new ArrowPayload(_)), context) | ||
|
|
||
| // Verify that the output schema is correct | ||
| val schemaOut = StructType.fromAttributes(output.drop(child.output.length).zipWithIndex | ||
| .map { case (attr, i) => attr.withName(s"_$i") }) | ||
| assert(schemaOut.equals(outputRowIterator.schema), | ||
| s"Invalid schema from pandas_udf: expected $schemaOut, got ${outputRowIterator.schema}") | ||
| if (outputRowIterator.hasNext) { | ||
| val schemaOut = StructType.fromAttributes(output.drop(child.output.length).zipWithIndex | ||
| .map { case (attr, i) => attr.withName(s"_$i") }) | ||
| assert(schemaOut.equals(outputRowIterator.schema), | ||
| s"Invalid schema from pandas_udf: expected $schemaOut, got ${outputRowIterator.schema}") | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks like we don't have a test against this case. We should add a test for invalid schema.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I tried to make one but since we are now casting the return Series in |
||
| } | ||
|
|
||
| outputRowIterator | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have we installed pyarrow on Jenkins? The failed test complains
ImportError: No module named pyarrow.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could just do
# doctest: +SKIPmaybe.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I thought that the Jenkins environment for unit tests would be the same for doctests and have pyarrow installed. @holdenk or @shaneknapp do you know if that is the case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
adding @JoshRosen too.
the doc building node (amp-jenkins-worker-01) doesn't have arrow installed for the default conda python 2.7 environment. for the python 3 environment, we're running arrow 0.4.0.
i looked at the script and it seems to be agnostic to python 2 vs 3... once i know which version of python we'll be running i can make sure that the version of arrow installed is correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, thanks @shaneknapp!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm.. but shouldn't we skip those doctests because they are not hard dependencies anyway?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's true, I see that
toPandas()also skips doctests. I'll skip this now and can always enable later if we decide differently. @shaneknapp , looks like we will hold off on this so no need to do anything to Jenkins I believe, sorry to bug you.