[SPARK-44749][PYTHON][FOLLOWUP][TESTS] Add more tests for named arguments in Python UDTF #42490
Conversation
dtenedor
left a comment
dtenedor
left a comment
There was a problem hiding this comment.
LGTM if @allisonwang-db agrees 👍
allisonwang-db
left a comment
allisonwang-db
left a comment
There was a problem hiding this comment.
Looks great! Thanks for the follow-up PR.
| @udtf | ||
| class TestUDTF: | ||
| @staticmethod | ||
| def analyze(a, b=None): |
There was a problem hiding this comment.
If b is a parameter with a default value (e.g 100), do we need to also assign a default value (e.g. None) here in the analyze method? If we directly use def analyze(a, b) does it throw an exception?
There was a problem hiding this comment.
Yes, if b here doesn't have a default value, it will raise an exception.
Both eval and analyze will be called with the same argument list.
So if eval takes a, analyze should also take a, if eval takes a, b, analyze should also take a, b, and so on.
It can be AnalyzeArgument with the correct parameters, or take **kwargs.
def analyze(a, b=AnalyzeArgument(StringType(), None, False)):def analyze(a, **kwargs):but taking AnalyzeArgument as default is not recommended if the data_type is a struct type, which could cause unexpected behavior similar to taking [] or {} as default.
|
Thanks! merging to master. |
…ents in Python UDTF ### What changes were proposed in this pull request? This is a follow-up of apache#42422. Adds more tests for named arguments in Python UDTF. ### Why are the changes needed? There are more cases to test. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Added related tests. Closes apache#42490 from ueshin/issues/SPARK-44749/tests. Authored-by: Takuya UESHIN <[email protected]> Signed-off-by: Takuya UESHIN <[email protected]>
3 participants
What changes were proposed in this pull request?
This is a follow-up of #42422.
Adds more tests for named arguments in Python UDTF.
Why are the changes needed?
There are more cases to test.
Does this PR introduce any user-facing change?
No.
How was this patch tested?
Added related tests.