dongjoon-hyun commented on PR #58978: URL: https://github.com/apache/spark/pull/58978#issuecomment-5792119574
Thank you for the PR. I reviewed it at `68d8aecbe4d` and left inline comments. I found these by reading and tracing the code; I did not run the reproductions. The most serious ones are: - sliced results are read at the wrong offset, which silently returns wrong data - `sys.exit()` in a UDF terminates the executor JVM - common `groupBy` + UDF patterns produce invalid plans Many of the other comments come from `InProcessPythonUDF` / `InProcessEvalPython` not following the `PythonUDF` / `ArrowEvalPython` contracts. Existing rules recognize Python UDFs by type or by the `PYTHON_UDF` tree pattern, so these new nodes bypass their guards. Did you consider plugging the in-process evaluator into the existing `PythonUDF` + `ArrowEvalPython` path, for example as a new eval type? A few smaller items: - `InProcessArrowEvalExec` lacks `producedAttributes`, so explain shows `!`. - The new rules are not in `nonExcludableRules`. - `InProcessPythonPlugin` catches only `Exception`, so the `UnsatisfiedLinkError` / `NoClassDefFoundError` cases it describes are not logged. - The docs list Python 3.8+ and PyArrow 12+, but PySpark requires Python 3.11+ and PyArrow 18.0.0+. - The Dockerfile example uses `ENV ...$(...)`, but Docker's `ENV` does not run command substitution. - `zip -r myvenv.zip myvenv/` extracts to `./myvenv/myvenv/...`. - `__init__.py` and `sql/core/pom.xml` contain non-ASCII em-dashes. -- 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]
