dongjoon-hyun commented on PR #58185:
URL: https://github.com/apache/spark/pull/58185#issuecomment-5704479487
Thanks for addressing the feedback, @cloud-fan. #2 and #3 look good to me.
For #1, I'm fine with handling SPARK-58628 post-merge given the feature is
experimental and gated, as long as it lands before the next release. Could we
mark it accordingly?
A few remaining items:
1. **PR description is stale.** It says "Numeric and non-literal unary
arithmetic, modulo ... fall back to interpreted Python", but the current code
still lowers `+ - * %` and unary ops with NULL guards (and
`test_udf_transpile_lowers_operators` asserts that). Only string repetition
falls back now. The integral/fractional split and the
`ResolveTranspiledPythonUDFOptions` change are also not mentioned. Since the
description becomes the commit message, could you update it?
2. **Side effects of the integral/fractional split** (all fail-closed, so
not correctness issues, but worth noting):
- `int`/`float` annotations are now pinned to `integral`/`fractional`
unconditionally, so `def f(x: float): return x + 1` on a `long` column now
falls back where it previously transpiled.
- With comparisons and <= 3 untyped params, the option matrix grows from
2^n to 3^n (up to 27), which compounds with the SPARK-58628 growth.
- With > 3 untyped params, mixed long/double inputs no longer match any
homogeneous variant and fall back.
3. Nits:
- `if i in (0, 4, 5)` in `test_udf_transpile_string_operands` is brittle;
splitting the cases into lowered / fallback lists would be clearer.
- The concat NULL error says "arithmetic with NULL".
--
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]
