dongjoon-hyun commented on PR #58185:
URL: https://github.com/apache/spark/pull/58185#issuecomment-5417860086
Thanks for the follow-up. The direction looks right to me -- failing closed
on NULL arithmetic, handling NaN in comparisons, and guarding string concat are
all real fidelity improvements, and CI is green. Three things I'd like to raise
before this goes in.
### 1. The NULL guard duplicates operand subtrees, so nested arithmetic
grows exponentially
In `_convert_chunk`'s `BinOp` arm, `left_col` / `right_col` appear both in
the guard condition and in the `otherwise` branch:
```python
null_guard = left_col.isNull() | right_col.isNull()
...
return when(null_guard,
raise_error(null_error)).otherwise(left_col.__add__(right_col))
```
Each nesting level therefore emits **two copies** of each child subtree.
Counting node occurrences, `a_n = 7 + 2*a_(n-1) + 2`, i.e. `a_n = 20 * 2^(n-1)
- 9`:
| expression | before this PR | after this PR |
|---|---|---|
| `x + 1` | 3 | 11 |
| `x + 1` chained 10x | 21 | ~10,200 |
| `x + 1` chained 20x | 41 | ~10,500,000 |
Catalyst expressions are immutable case classes, so the initial object graph
is shared, but `transform` / `canonicalized` / codegen all visit **each
occurrence**, so analysis and optimization time grows with the occurrence
count, not the shared node count. Subexpression elimination only mitigates
runtime CPU, not planning; and the generated code can blow past the 64KB method
limit and fall back to interpreted eval. The `Mod` case is worse: `sb`
references `right_col` twice, so the right operand is duplicated ~6x per level.
To be clear about what is new here: the same duplication pattern already
existed in `_lower_value_compare` and `_lower_eq` on master. It did not bite,
because a comparison usually sits once at the top of a UDF body while the
arithmetic underneath it was linear. Extending the pattern to `+ - * %` and
unary -- where nesting actually happens -- is what turns it from linear into
exponential.
Suggestion: track nullability statically on the Python side and only emit an
`isNull()` term for operands that can actually be NULL:
- `ast.Constant` (non-`None`) -> not nullable
- an already-guarded arithmetic / concat result -> not nullable (under ANSI,
`Add`/`concat`/`pmod` over non-NULL operands cannot produce NULL)
- `ast.Name` (a bound parameter) -> nullable
With that, `x + 1 + 1 + ...` guards `x` once at the innermost level and
emits no guard above it, which brings the chain back to linear. This has to
live in the transpiler: Catalyst sees `CaseWhen.nullable = true` because of the
`RaiseError` branch, so it cannot infer it for us. The unary arm has the same
shape (`operand_col` referenced twice) and would benefit from the same
treatment.
### 2. `isnan` on integral columns adds a per-row cast whose result is
always false
`IsNaN` declares `inputTypes = Seq(TypeCollection(DoubleType, FloatType))`
with `ImplicitCastInputTypes`, while the `"numeric"` category on the JVM side
matches `NumericType && !DecimalType`
(`ResolveTranspiledPythonUDFOptions.optionMatchesTypes`) -- so
Byte/Short/Int/Long bind to it too.
That means a plain `lambda x: x > 0` on a `long` column now evaluates
`IsNaN(cast(a as double))` for every row, and the answer is always false.
Analysis does resolve (`Cast.canANSIStoreAssign(LongType, DoubleType)` is true
under ANSI), so this is a cost issue rather than a correctness one, but it
lands on what is probably the most common transpiled shape.
Would it be worth splitting the numeric category into integral / fractional
variants so the NaN guard is only emitted where NaN is representable? If that
is too much for v0, a comment noting the overhead plus a follow-up JIRA would
at least keep it visible.
### 3. Leftover `repeat` references now that the lowering is gone
These lines were accurate on master and became stale when this PR dropped
the `repeat` lowering:
- `_category` still returns `"string"` for `{numeric, string}` `Mult` with
the comment `# str * int / int * str -> repeat`. Behavior is still fail-closed
(`_convert_chunk` raises and the variant is dropped), but it is now a dead
branch with a misleading comment.
- the `_category` catch-all comment still says "don't drive concat/repeat
selection".
- `ResolveTranspiledPythonUDFOptions.scala`: "so the string lowerings (e.g.
`repeat`) never see it" now points at a lowering that no longer exists.
Worth cleaning up in this PR since it is the one removing the feature.
--
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]
