dongjoon-hyun commented on PR #58972:
URL: https://github.com/apache/spark/pull/58972#issuecomment-5797569351
Thank you for making a PR, @david-mollitor-db. The change itself looks
correct to me: `CheckOverflow` uses `nullSafeEval`/`nullSafeCodeGen`, so
null-in => null-out holds in both interpreted and codegen paths. I also checked
the consumers of `nullIntolerant` (`NullPropagation`,
`QueryPlanConstraints.scanNullIntolerantAttribute`, `FilterExec.notNullPreds`),
and `NullDownPropagation` is protected by the `supportedNullIntolerant`
allowlist, so `IsNull(CheckOverflow(x))` will not be rewritten into `IsNull(x)`.
A few comments:
1. The PR description and the commit message say that `CheckOverflow` was
missed when SPARK-50241 replaced the `NullIntolerant` mixin with a `def`.
That's not accurate. Before SPARK-50241 (a84ca5e46e6), `CheckOverflow` was
`UnaryExpression with SupportQueryContext` and never mixed in `NullIntolerant`.
So it was never declared, not missed during SPARK-50241. Could you fix the
description?
2. Could you add a test case for the new behavior? Existing suites passing
only shows that nothing changed. For example:
- `InferFiltersFromConstraintsSuite`: `IsNotNull('a)` is inferred from a
filter like `CheckOverflow('a, ...) > 0`.
- `NullPropagation`: `CheckOverflow(Literal(null, DecimalType(...)),
...)` is folded to a null literal.
--
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]
