Skip to content

Commit bd2c9af

Browse files
committed
fix: correct nullability of AND/OR expressions to resolve plan mismatch in CSE
Fixes #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.
1 parent 5218379 commit bd2c9af

2 files changed

Lines changed: 98 additions & 0 deletions

File tree

‎datafusion/expr/src/expr_schema.rs‎

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -435,6 +435,28 @@ impl ExprSchemable for Expr {
435435
Expr::ScalarSubquery(subquery) => Ok(scalar_subquery_nullable(subquery)),
436436
Expr::BinaryExpr(BinaryExpr { left, right, op }) => match op {
437437
Operator::IsDistinctFrom | Operator::IsNotDistinctFrom => Ok(false),
438+
Operator::And => {
439+
// Detect pattern: `x IS NOT NULL AND x` (or nested via AND chains).
440+
// If IS NOT NULL guard is present for the other operand,
441+
// the AND expression is guaranteed non-nullable.
442+
if contains_is_not_null(left.as_ref(), right.as_ref())
443+
|| contains_is_not_null(right.as_ref(), left.as_ref())
444+
{
445+
return Ok(false);
446+
}
447+
Ok(left.nullable(input_schema)? || right.nullable(input_schema)?)
448+
}
449+
Operator::Or => {
450+
// Detect pattern: `x IS NULL OR x` (or nested via OR chains).
451+
// If IS NULL guard is present for the other operand,
452+
// the OR expression is guaranteed non-nullable.
453+
if contains_is_null(left.as_ref(), right.as_ref())
454+
|| contains_is_null(right.as_ref(), left.as_ref())
455+
{
456+
return Ok(false);
457+
}
458+
Ok(left.nullable(input_schema)? || right.nullable(input_schema)?)
459+
}
438460
_ => Ok(left.nullable(input_schema)? || right.nullable(input_schema)?),
439461
},
440462
Expr::Like(Like { expr, pattern, .. })
@@ -1623,3 +1645,31 @@ mod tests {
16231645
}
16241646
}
16251647
}
1648+
1649+
/// Returns true if `expr` contains an `IS NOT NULL(target)` check,
1650+
/// recursively searching through nested AND expressions.
1651+
/// Used by `Expr::nullable()` to detect patterns like `x IS NOT NULL AND x`
1652+
/// where the AND result is guaranteed non-nullable.
1653+
fn contains_is_not_null(expr: &Expr, target: &Expr) -> bool {
1654+
if let Expr::IsNotNull(inner) = expr {
1655+
return inner.as_ref() == target;
1656+
}
1657+
if let Expr::BinaryExpr(BinaryExpr { left, right, op: Operator::And }) = expr {
1658+
return contains_is_not_null(left, target) || contains_is_not_null(right, target);
1659+
}
1660+
false
1661+
}
1662+
1663+
/// Returns true if `expr` contains an `IS NULL(target)` check,
1664+
/// recursively searching through nested OR expressions.
1665+
/// Used by `Expr::nullable()` to detect patterns like `x IS NULL OR x`
1666+
/// where the OR result is guaranteed non-nullable.
1667+
fn contains_is_null(expr: &Expr, target: &Expr) -> bool {
1668+
if let Expr::IsNull(inner) = expr {
1669+
return inner.as_ref() == target;
1670+
}
1671+
if let Expr::BinaryExpr(BinaryExpr { left, right, op: Operator::Or }) = expr {
1672+
return contains_is_null(left, target) || contains_is_null(right, target);
1673+
}
1674+
false
1675+
}
Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
# Licensed to the Apache Software Foundation (ASF) under one
2+
# or more contributor license agreements. See the NOTICE file
3+
# distributed with this work for additional information
4+
# regarding copyright ownership. The ASF licenses this file
5+
# to you under the Apache License, Version 2.0 (the
6+
# "License"); you may not use this file except in compliance
7+
# with the License. You may obtain a copy of the License at
8+
#
9+
# http://www.apache.org/licenses/LICENSE-2.0
10+
#
11+
# Unless required by applicable law or agreed to in writing,
12+
# software distributed under the License is distributed on an
13+
# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
14+
# KIND, either express or implied. See the License for the
15+
# specific language governing permissions and limitations
16+
# under the License.
17+
18+
##########
19+
# Regression test for https://github.com/apache/datafusion/issues/25978
20+
# A GROUP BY aggregate over a UNION ALL fails to plan when one branch
21+
# projects coalesce(<nullable boolean expression>, FALSE).
22+
# The CSE optimizer rewrites this into an AND/OR pattern whose
23+
# nullability was incorrectly reported, causing a physical vs logical
24+
# schema mismatch.
25+
##########
26+
27+
statement ok
28+
CREATE TABLE t(v INT, f BOOLEAN) AS VALUES (1, true), (2, NULL), (3, false);
29+
30+
# This query previously failed with:
31+
# Internal error: Physical input schema should be the same as the one
32+
# converted from logical input schema. Differences:
33+
# - field nullability at index 1 [f]: (physical) true vs (logical) false.
34+
query IB rowsort
35+
SELECT v, coalesce(f, false)
36+
FROM (
37+
SELECT v, f FROM t
38+
UNION ALL
39+
SELECT v, f FROM t
40+
)
41+
GROUP BY v, coalesce(f, false)
42+
----
43+
1 true
44+
2 false
45+
3 false
46+
47+
statement ok
48+
DROP TABLE t;

0 commit comments

Comments
 (0)