Repository navigation
Fixes #25978: correct nullability of AND/OR expressions to resolve plan mismatch in CSE - #26164
siddubakka wants to merge 1 commit into
Conversation
github-actions
Bot
added
logical-expr
| use datafusion::error::Result; | ||
|
|
||
| #[tokio::test] | ||
| async fn test_issue_25978() -> Result<()> { |
There was a problem hiding this comment.
please write this as a slt test
As it is, this test only verifies the query doesn't error -- using slt you can aso verify its correctness
There was a problem hiding this comment.
makes sense, switched it to slt!
| Expr::BinaryExpr(BinaryExpr { left, right, op }) => match op { | ||
| Operator::IsDistinctFrom | Operator::IsNotDistinctFrom => Ok(false), | ||
| Operator::And => { | ||
| if contains_is_not_null(left.as_ref(), right.as_ref()) { |
There was a problem hiding this comment.
this feels like treating the symptom rather than the root cause -- why would we need to special case IS NOT NULL as part of AND / OR ? Wouldn't it make more sense to perhaps set nullability for IsNotNull itself?
There was a problem hiding this comment.
yeah fair point -- so IsNotNull already returns Ok(false) for nullable since it's always a bool. the thing is when the simplifier turns CASE WHEN x IS NOT NULL THEN x ELSE false END into x IS NOT NULL AND x, the AND sees that x is nullable and reports the whole thing as nullable. but semantically it can't be null -- if x is null then IS NOT NULL(x) is false and alse AND NULL = false. so the special-casing here is really about AND/OR knowing that an IS NOT NULL guard makes the other operand safe. happy to explore a different approach if you have something in mind though!
There was a problem hiding this comment.
Maybe we can add some more comments explaining why we need to explicitly check IS NOT NULL
ALso, the example you gave above is only valid when the argument (x) is on both sides
x IS NOT NULL AND x
However it doesn't hold for:
y IS NOT NULL and x (different arguments).
This seems very similar to something @pepijnve hit a while ago with Case nullability -- specifically that the after simplification the nullability can sometimes be smarter than before the simplification.
I am still missing why we need to make the nullability check smarter (why isn't leaving it as nullable ok, even if overly conservative)?
| // Null-aware RightAnti only supports CollectLeft | ||
| let partition_mode = if hash_join.null_aware { | ||
| PartitionMode::CollectLeft | ||
| if !hash_join.dynamic_expressions_produced().is_empty() { |
There was a problem hiding this comment.
is this tested anywhere? How is it related to the other fix?
There was a problem hiding this comment.
yeah sorry about that, it got mixed in from another branch. removed it now.
…ch in CSE Fixes apache#25978 The CommonSubexprEliminate optimizer rewrites CASE WHEN x IS NOT NULL THEN x ELSE false END into x IS NOT NULL AND x. The default nullable analysis for AND reports true (because x is nullable), even though the expression is semantically guaranteed non-nullable. This commit adds recursive IS NOT NULL / IS NULL guard detection in Expr::nullable() for AND and OR operators, and includes an SLT regression test.
16a6135 to
bd2c9af
Compare
github-actions
Bot
added
sqllogictest
|
hey @alamb, pushed the updates -- switched to an slt test, dropped the unrelated join_selection changes, and rebased on main. should be a clean single commit now. let me know if anything else needs changing! |
2 participants
Fixes #25978
This PR fixes an internal error where the physical input schema mismatches the logical input schema due to incorrect nullability propagation for AND/OR expressions in CommonSubexprEliminate.
It adds robust contains_is_not_null and contains_is_null helper functions to accurately determine if binary expressions are nullable.
cc @alamb @jayzhan211