spark-test commented on code in PR #59742:
URL: https://github.com/apache/spark/pull/59742#discussion_r4356997667
##########
python/pyspark/worker.py:
##########
@@ -235,10 +237,9 @@ def read_single_udtf(pickleSer, udtf_info, handler_cls):
f"The return type of a UDTF must be a struct type, but got
{type(return_type)}."
)
- # Update the handler that creates a new UDTF instance to first try calling
the UDTF constructor
- # with one argument containing the previous AnalyzeResult. If that fails,
then try a constructor
- # with no arguments. In this way each UDTF class instance can decide if it
wants to inspect the
- # AnalyzeResult.
+ # Pick how to construct the UDTF from its __init__ signature. With an
AnalyzeResult, an
+ # __init__ taking one argument receives a copy of it and one taking none
is called as is;
+ # without one, __init__ must take no arguments. Any other signature is
rejected.
Review Comment:
**Nit (P3):** The check below only counts
`inspect.getfullargspec(handler).args`, which lists named positional
parameters. Variadic and keyword-only parameters are not counted. So
`__init__(self, *args, **kwargs)` and `__init__(self, *, k)` both report `args
== ['self']` and pass the check. With an AnalyzeResult, the variadic one is
called as `handler()` and silently gets no AnalyzeResult. A required
keyword-only argument fails only at instantiation, as `UDTF_EXEC_ERROR` rather
than a `UDTF_CONSTRUCTOR_INVALID_*` error. Neither is rejected here, so a
reader could look for an up-front rejection that does not exist. The error
messages in error-conditions.json already phrase this as an argument count.
Something like:
```suggestion
# Pick how to construct the UDTF from the count of named positional
__init__ parameters
# after self. With an AnalyzeResult, one parameter receives a copy of
it, zero means the
# class is called with no arguments, and more are an error; without one,
any is an error.
```
**Verification:**
- **Inspection:** Confirm that each case in the reworded UDTF constructor
comment matches a branch of the argument-count check below it: one positional
parameter after self receives a copy of the AnalyzeResult, none is called as
is, and larger counts raise the matching UDTF_CONSTRUCTOR_INVALID_* error.
Confirm that the comment makes no claim about variadic or keyword-only
constructors being rejected.
- **Inspection:** Confirm that the Python worker module is unchanged apart
from comments and docstrings, so the correction keeps the PR free of behavior
changes.
--
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]
