[SPARK-58601][PYTHON] Tighten mapInPandas return-value contract to require a strict Iterator - #57800
[SPARK-58601][PYTHON] Tighten mapInPandas return-value contract to require a strict Iterator#57800Yicong-Huang wants to merge 3 commits into
Conversation
uros-b
left a comment
There was a problem hiding this comment.
Should we update the migration guide? To explain mapInPandas UDF returning a plain list of DataFrames (e.g. lambda it: [pdf for pdf in it]) previously succeeded and now raises UDF_RETURN_TYPE with "but is list"
Otherwise looks good, thank you @Yicong-Huang!
Thanks @uros-b! I am on the fence here. The public doc declares |
Actually I think we should just keep the change for now, and maybe issue a warning or sth
cloud-fan
left a comment
There was a problem hiding this comment.
0 blocking, 1 non-blocking, 0 nits.
The implementation and focused regression test are sound, but the observable compatibility change should be called out in the PySpark migration guide.
Suggestions (1)
- General: Add a PySpark 4.3 migration note explaining that mapInPandas now rejects lists and other non-Iterator iterables, and recommend returning iter(...).
Verification
Verified that the public mapInPandas documentation requires an iterator return, the worker now performs a strict collections.abc.Iterator check, and the closest mapInArrow analogue enforces the same outer-container contract through verify_return_type. The new test covers the formerly accepted list case and preserves existing coverage for invalid iterator elements.
PR metadata suggestions
- Correct the user-facing-change section: returning a list or another non-Iterator iterable now changes from success to UDF_RETURN_TYPE.
|
I changed it to describe as behavior change. Also added a flag to revert back to the old behavior. the same flag will guard mapInArrow as well. A migration guide from 4.2 to 4.3 is added. Note that I kept the flag @uros-b @HyukjinKwon @cloud-fan could you please check again? |
cloud-fan
left a comment
There was a problem hiding this comment.
1 addressed, 0 remaining, 1 new. (1 newly introduced, 0 late catches, 0 previously raised.)
1 blocking, 0 non-blocking, 0 nits.
The runtime and compatibility paths are coherent, but Spark Connect's public callback annotations need to be aligned with the newly enforced contract.
Correctness (1)
- python/docs/source/migration_guide/pyspark_upgrade.rst:24: Spark Connect's callback aliases still advertise
Iterablereturns even though the shared worker now requiresIterator. -- see inline
Verification
Traced both MapInBatch evaluation types from MapInBatchExec through the Arrow runner configuration into worker.read_udfs. The strict and legacy branches match for pandas and Arrow, and focused tests cover both modes. Spark Connect dispatches those same evaluation types while its callback aliases still declare Iterable returns.
PR metadata suggestions
- Correct the claim that the declared signatures have always used Iterator: Spark Connect's current callback aliases use Iterable, or describe that discrepancy until those aliases are updated.
|
|
||
| Upgrading from PySpark 4.2 to 4.3 | ||
| --------------------------------- | ||
| * In Spark 4.3, a ``mapInPandas`` UDF must return an iterator of ``pandas.DataFrame``\s; returning any other iterable such as a ``list`` now raises ``UDF_RETURN_TYPE``, matching the existing ``mapInArrow`` behavior and the declared ``Iterator[...]`` signature. To restore the previous behavior of accepting any iterable for both ``mapInPandas`` and ``mapInArrow``, set ``spark.sql.execution.pythonUDF.mapInBatch.legacy.acceptAnyIterable.enabled`` to ``true``. |
There was a problem hiding this comment.
Please update the Spark Connect callback aliases to require Iterator as well. connect/_typing.py still declares both PandasMapIterFunction and ArrowMapIterFunction with Iterable returns, but Connect dispatches these same strict evaluation types, so its public annotations currently tell users that returning a list is valid when the worker rejects it.
| .doc("When true, mapInPandas and mapInArrow UDFs may return any iterable (e.g. a list) " + | ||
| "rather than a strict iterator, matching the behavior before 4.3.0. When false, the " + | ||
| "returned value must be an iterator, matching the declared Iterator[...] signatures.") | ||
| .version("4.3.0") |
There was a problem hiding this comment.
4.4 since branch-4.3 is already cut out
What changes were proposed in this pull request?
Tighten the
mapInPandas(SQL_MAP_PANDAS_ITER_UDF) return-value contract inworker.pyto require a strictIterator, matchingmapInArrow(SQL_MAP_ARROW_ITER_UDF). Previously the runtime check wasisinstance(result, Iterator) or hasattr(result, "__iter__"), which accepted any iterable (e.g. a returnedlist); it is nowisinstance(result, Iterator).A legacy escape hatch is added to restore the old behavior for both
mapInPandasandmapInArrow, since they share theMapInBatchExecevaluator:spark.sql.execution.pythonUDF.mapInBatch.legacy.acceptAnyIterable.enabled(internal, defaultfalse). Whentrue, both UDFs may return any iterable rather than a strict iterator, matching the pre-4.3.0 behavior.ArrowPythonRunner.getPythonRunnerConfMap.Why are the changes needed?
The declared signatures have always been
PandasMapIterFunction = Callable[[Iterator[DataFrameLike]], Iterator[DataFrameLike]]andArrowMapIterFunction = Callable[[Iterator[pyarrow.RecordBatch]], Iterator[pyarrow.RecordBatch]], and theDataFrame.mapInPandasdocstring states the function "outputs an iterator of pandas.DataFrames". The runtime, however, leniently accepted any iterable formapInPandas, diverging from the documented contract. This aligns themapInPandasruntime with the declaredIterator[...]signature (the same tighteningmapInArrowalready received in SPARK-56612), while the legacy flag gives users a migration path off the old iterable-accepting behavior for both APIs.Does this PR introduce any user-facing change?
Yes. A
mapInPandasUDF that returns a non-Iteratoriterable (e.g. alist) is now rejected withUDF_RETURN_TYPE("iterator of pandas.DataFrame"). Previously such a return value was accepted. Users who relied on the old behavior can setspark.sql.execution.pythonUDF.mapInBatch.legacy.acceptAnyIterable.enabled=trueto restore it (this flag also coversmapInArrow).Previous behavior:
New behavior:
How was this patch tested?
Updated
test_pandas_map.py: removed the "returning list of DataFrames" positive case fromtest_map_in_pandas; added alist_not_iternegative case tocheck_other_than_dataframe_iterasserting a returnedlistis rejected (mirroringtest_arrow_map.py::test_other_than_recordbatch_iter); and addedtest_map_in_pandas_legacy_accept_any_iterableasserting the legacy flag restores acceptance. Added the matchingtest_map_in_arrow_legacy_accept_any_iterabletotest_arrow_map.py. Ran both suites plus themapInPandasandmapInArrowConnect parity suites. The behavior change is documented in the PySpark 4.2-to-4.3 migration guide.Was this patch authored or co-authored using generative AI tooling?
No