HyukjinKwon commented on PR #57804:
URL: https://github.com/apache/spark/pull/57804#issuecomment-5235285604
Thanks @dongjoon-hyun for the careful pass and for confirming the dedup /
worker re-nesting / ragged-zip semantics. Addressed the suggestions in 8d5e880:
1. **Eager-evaluation docs** - Added a note to the Arrow Python UDF tutorial
('Notes' section, next to the existing non-short-circuiting `WHERE` note): a
UDF inside a higher-order function lambda is precomputed over the whole array,
so it runs once per element (and over every pair for a two-argument
`array_sort` comparator) regardless of `exists`/`when` short-circuiting, and a
UDF that can fail must handle skipped inputs itself. Also expanded the config
doc to say the same.
2. **Pairwise `array_sort` cost** - Documented the O(n^2) call/memory cost
in the config doc and advise avoiding it for large arrays. I also corrected the
code comment: the shared-reference carry is O(n^2) only in interpreted
evaluation; a copy into the Unsafe format materializes each reference,
degrading to O(n^3), as you noted. I stopped short of a hard runtime cap: it
needs a new config + an injected error, and since these queries are opt-in and
already O(n^2) in Python calls by construction, a documented warning seemed the
more proportionate choice for this PR - happy to add a cap in a follow-up if
you would prefer one.
3. **Defense-in-depth** - `liftableHof` now also calls
`PythonUDF.hasFreeLambdaVariable(hof)` (made non-private and shared), so the
rule re-checks the nested-lambda guard itself instead of trusting that
`CheckAnalysis` ran. `transform(arr, i -> transform(i, x -> f(x)))` is now
rejected by the rule independently.
4. **Test gaps** - Added: the SQL string syntax path (`spark.sql("SELECT
transform(v, x -> pyUdf(x)) ...")`); an end-to-end kwargs call (`udf(x,
y=lit(1))`) still failing analysis; a rewritable HOF inside another HOF's
lambda over a real column (`transform(arr2, i -> array_max(transform(arr, x ->
f(x))) + i)`, both as a Scala plan-shape test and an e2e result test); and
decimal / timestamp / struct element types.
5. **`transform_keys` null-key error class** - Agreed this is expected for a
new feature; the rebuild goes through `MapFromArrays` so null/duplicate-key
dedup-policy behavior matches native, and the only difference is the error
class text on a null key. Leaving as-is; noting it here for the record.
Verified locally: `ExtractPythonUDFFromLambdaSuite` (22 tests) and the
end-to-end `pyspark.sql.tests.test_udf_in_higher_order_function` suite both
pass.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
