[SPARK-33845][SQL][FOLLOWUP] fix SimplifyConditionals #30953
Conversation
There was a problem hiding this comment.
Check result here to make sure there is no correctness issue.
| case p: LogicalPlan => p transformExpressions { | ||
| // For `EqualNullSafe` with a `TrueLiteral`, whether the other side is null or false has no | ||
| // difference, as `null <=> true` and `false <=> true` both return false. | ||
| case EqualNullSafe(left, TrueLiteral) => |
There was a problem hiding this comment.
The change is needed here to not fail the existing tests. When we convert If to EqualNullSafe, we should make sure the rule ReplaceNullWithFalseInPredicate still applies.
There was a problem hiding this comment.
It would be great if someone can pull this out and open a separated PR/JIRA. Otherwise, I'll do it tomorrow.
There was a problem hiding this comment.
NullPropagation should optimize EqualNullSafe(null, TrueLiteral) to false too, isn't?
There was a problem hiding this comment.
It can, but not with And, e.g. EqualNullSafe(col && null, true) => EqualNullSafe(col && false, true) => EqualNullSafe(false, true) => false.
|
Test build #133457 has finished for PR 30953 at commit
|
There was a problem hiding this comment.
+1, LGTM. Thank you, @cloud-fan and @viirya .
Merged to master.
|
Late LGTM. |
|
Thanks, LGTM |
|
good catch, late lgtm |
7 participants
What changes were proposed in this pull request?
This is a followup of #30849, to fix a correctness issue caused by null value handling.
Why are the changes needed?
Fix a correctness issue.
If(null, true, false)should return false, not true.Does this PR introduce any user-facing change?
Yes, but the bug only exist in the master branch.
How was this patch tested?
updated tests.