Skip to content

Commit 50cdbde

Browse files
lyne7-sclyne7-sc
authored
fix: preserve NULL semantics in bitwise xor simplification (#24248)
## Which issue does this PR close? <!-- We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123. --> - Part of #24246 . ## Rationale for this change Bitwise XOR simplifications cancelled repeated nullable operands, which could incorrectly produce a non-NULL result when the operand was NULL. For example: ```sql SELECT i XOR i, (i XOR 7) XOR i, i XOR (7 XOR i) FROM (VALUES (NULL::INT)) AS t(i); ``` These expressions should all return NULL, but simplification could replace them with 0, 7, and 7. <!-- Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed. Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes. Please explain the problem you are trying to solve in terms of the user-visible behavior, rather than the implementation. For example, "The code in `foo.rs` doesn't handle nulls" is a symptom of the implementation. "COUNT(DISTINCT) returns wrong results when the column contains nulls" is the user-visible problem. --> ## What changes are included in this PR? Only cancel repeated XOR operands when the removed operand is non-nullable. <!-- There is no need to duplicate the description in the issue here, but it is sometimes worth providing a summary of the individual changes in this PR. --> ## Are these changes tested? <!-- We typically require tests for all PRs in order to: 1. Prevent the code from being accidentally broken by subsequent changes 2. Serve as another way to document the expected behavior of the code If tests are not included in your PR, please explain why (for example, are they covered by existing tests)? --> Yes. Added sqllogictests covering results. ## Are there any user-facing changes? Yes. Bitwise XOR expressions with repeated nullable operands now correctly preserve NULLs. <!-- If there are user-facing changes then we may require documentation to be updated before approving the PR. If there are any breaking changes to public APIs, please add the `api change` label. -->
1 parent 90164e1 commit 50cdbde

2 files changed

Lines changed: 73 additions & 26 deletions

File tree

‎datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs‎

Lines changed: 62 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -1372,7 +1372,7 @@ impl TreeNodeRewriter for Simplifier<'_> {
13721372
left,
13731373
op: BitwiseXor,
13741374
right,
1375-
}) if expr_contains(&left, &right, BitwiseXor) => {
1375+
}) if !info.nullable(&right)? && expr_contains(&left, &right, BitwiseXor) => {
13761376
let expr = delete_xor_in_complex_expr(&left, &right, false);
13771377
Transformed::yes(if expr == *right {
13781378
Expr::Literal(
@@ -1389,7 +1389,7 @@ impl TreeNodeRewriter for Simplifier<'_> {
13891389
left,
13901390
op: BitwiseXor,
13911391
right,
1392-
}) if expr_contains(&right, &left, BitwiseXor) => {
1392+
}) if !info.nullable(&left)? && expr_contains(&right, &left, BitwiseXor) => {
13931393
let expr = delete_xor_in_complex_expr(&right, &left, true);
13941394
Transformed::yes(if expr == *left {
13951395
Expr::Literal(
@@ -3020,16 +3020,19 @@ mod tests {
30203020
// c2 ^ ((c2 ^ (c2 | c1)) ^ (c1 & c2)) --> (c2 | c1) ^ (c1 & c2)
30213021

30223022
let expr = bitwise_xor(
3023-
col("c2"),
3023+
col("c2_non_null"),
30243024
bitwise_xor(
3025-
bitwise_xor(col("c2"), bitwise_or(col("c2"), col("c1"))),
3026-
bitwise_and(col("c1"), col("c2")),
3025+
bitwise_xor(
3026+
col("c2_non_null"),
3027+
bitwise_or(col("c2_non_null"), col("c1")),
3028+
),
3029+
bitwise_and(col("c1"), col("c2_non_null")),
30273030
),
30283031
);
30293032

30303033
let expected = bitwise_xor(
3031-
bitwise_or(col("c2"), col("c1")),
3032-
bitwise_and(col("c1"), col("c2")),
3034+
bitwise_or(col("c2_non_null"), col("c1")),
3035+
bitwise_and(col("c1"), col("c2_non_null")),
30333036
);
30343037

30353038
assert_eq!(simplify(expr), expected);
@@ -3038,18 +3041,24 @@ mod tests {
30383041
// c2 ^ (c2 ^ (c2 | c1)) ^ ((c1 & c2) ^ c2) --> c2 ^ ((c2 | c1) ^ (c1 & c2))
30393042

30403043
let expr = bitwise_xor(
3041-
col("c2"),
3044+
col("c2_non_null"),
30423045
bitwise_xor(
3043-
bitwise_xor(col("c2"), bitwise_or(col("c2"), col("c1"))),
3044-
bitwise_xor(bitwise_and(col("c1"), col("c2")), col("c2")),
3046+
bitwise_xor(
3047+
col("c2_non_null"),
3048+
bitwise_or(col("c2_non_null"), col("c1")),
3049+
),
3050+
bitwise_xor(
3051+
bitwise_and(col("c1"), col("c2_non_null")),
3052+
col("c2_non_null"),
3053+
),
30453054
),
30463055
);
30473056

30483057
let expected = bitwise_xor(
3049-
col("c2"),
3058+
col("c2_non_null"),
30503059
bitwise_xor(
3051-
bitwise_or(col("c2"), col("c1")),
3052-
bitwise_and(col("c1"), col("c2")),
3060+
bitwise_or(col("c2_non_null"), col("c1")),
3061+
bitwise_and(col("c1"), col("c2_non_null")),
30533062
),
30543063
);
30553064

@@ -3060,15 +3069,18 @@ mod tests {
30603069

30613070
let expr = bitwise_xor(
30623071
bitwise_xor(
3063-
bitwise_xor(col("c2"), bitwise_or(col("c2"), col("c1"))),
3064-
bitwise_and(col("c1"), col("c2")),
3072+
bitwise_xor(
3073+
col("c2_non_null"),
3074+
bitwise_or(col("c2_non_null"), col("c1")),
3075+
),
3076+
bitwise_and(col("c1"), col("c2_non_null")),
30653077
),
3066-
col("c2"),
3078+
col("c2_non_null"),
30673079
);
30683080

30693081
let expected = bitwise_xor(
3070-
bitwise_or(col("c2"), col("c1")),
3071-
bitwise_and(col("c1"), col("c2")),
3082+
bitwise_or(col("c2_non_null"), col("c1")),
3083+
bitwise_and(col("c1"), col("c2_non_null")),
30723084
);
30733085

30743086
assert_eq!(simplify(expr), expected);
@@ -3078,23 +3090,47 @@ mod tests {
30783090

30793091
let expr = bitwise_xor(
30803092
bitwise_xor(
3081-
bitwise_xor(col("c2"), bitwise_or(col("c2"), col("c1"))),
3082-
bitwise_xor(bitwise_and(col("c1"), col("c2")), col("c2")),
3093+
bitwise_xor(
3094+
col("c2_non_null"),
3095+
bitwise_or(col("c2_non_null"), col("c1")),
3096+
),
3097+
bitwise_xor(
3098+
bitwise_and(col("c1"), col("c2_non_null")),
3099+
col("c2_non_null"),
3100+
),
30833101
),
3084-
col("c2"),
3102+
col("c2_non_null"),
30853103
);
30863104

30873105
let expected = bitwise_xor(
30883106
bitwise_xor(
3089-
bitwise_or(col("c2"), col("c1")),
3090-
bitwise_and(col("c1"), col("c2")),
3107+
bitwise_or(col("c2_non_null"), col("c1")),
3108+
bitwise_and(col("c1"), col("c2_non_null")),
30913109
),
3092-
col("c2"),
3110+
col("c2_non_null"),
30933111
);
30943112

30953113
assert_eq!(simplify(expr), expected);
30963114
}
30973115

3116+
#[test]
3117+
fn test_does_not_cancel_nullable_bitwise_xor() {
3118+
let nullable = col("c3");
3119+
let other = col("c3_non_null");
3120+
3121+
let expr = bitwise_xor(nullable.clone(), nullable.clone());
3122+
assert_eq!(simplify(expr.clone()), expr);
3123+
3124+
let expr = bitwise_xor(
3125+
bitwise_xor(nullable.clone(), other.clone()),
3126+
nullable.clone(),
3127+
);
3128+
assert_eq!(simplify(expr.clone()), expr);
3129+
3130+
let expr = bitwise_xor(nullable.clone(), bitwise_xor(other, nullable));
3131+
assert_eq!(simplify(expr.clone()), expr);
3132+
}
3133+
30983134
#[test]
30993135
fn test_simplify_negated_bitwise_and() {
31003136
// !c3 & c3 --> 0
@@ -3184,13 +3220,13 @@ mod tests {
31843220
#[test]
31853221
fn test_simplify_simple_bitwise_xor() {
31863222
// c4 ^ c4 -> 0
3187-
let expr = (col("c4")).bitxor(col("c4"));
3223+
let expr = (col("c4_non_null")).bitxor(col("c4_non_null"));
31883224
let expected = lit(0u32);
31893225

31903226
assert_eq!(simplify(expr), expected);
31913227

31923228
// c3 ^ c3 -> 0
3193-
let expr = col("c3").bitxor(col("c3"));
3229+
let expr = col("c3_non_null").bitxor(col("c3_non_null"));
31943230
let expected = lit(0i64);
31953231

31963232
assert_eq!(simplify(expr), expected);

‎datafusion/sqllogictest/test_files/scalar.slt‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1444,6 +1444,17 @@ select a ^ b, c ^ d, e ^ f from signed_integers;
14441444
-998 -133 -16
14451445
NULL NULL NULL
14461446

1447+
# repeated nullable operands must not be cancelled
1448+
query III rowsort
1449+
select
1450+
(a XOR b) XOR a AS left_associative,
1451+
a XOR (b XOR a) AS right_associative,
1452+
a XOR a AS self_xor
1453+
from (values (NULL::INT, 7), (3, 7)) as t(a, b);
1454+
----
1455+
7 7 0
1456+
NULL NULL NULL
1457+
14471458
# bitwise xor with other operators
14481459
query II rowsort
14491460
select 2 * c - 1 ^ 856 + d + 3, d ^ 7 >> 4 from signed_integers;

0 commit comments

Comments
 (0)