kumarUjjawal commented on code in PR #23615:
URL: https://github.com/apache/datafusion/pull/23615#discussion_r3718041726
##########
datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs:
##########
@@ -2531,6 +2531,74 @@ mod tests {
}
}
+ #[test]
+ fn test_simplify_swapped_operands_in_and_or_no_canonicalize() {
+ // Regression test for
https://github.com/apache/datafusion/issues/14943
+ //
+ // `SimplifyExpressions` disables canonicalization for
`LogicalPlan::Join`
+ // (see https://github.com/apache/datafusion/pull/8780), so commutative
+ // operands like `A = B` and `B = A` cannot be normalized to a single
+ // form before AND/OR dedup runs. The dedup itself must therefore
+ // recognize commutative equivalence directly.
+
+ // c3 = 5 AND 5 = c3 --> c3 = 5
+ let expr = col("c3_non_null")
+ .eq(lit(5_i64))
+ .and(lit(5_i64).eq(col("c3_non_null")));
+ let expected = col("c3_non_null").eq(lit(5_i64));
+ assert_eq!(simplify_no_canonicalize(expr), expected);
+
+ // 5 = c3 AND c3 = 5 --> 5 = c3
+ let expr = lit(5_i64)
+ .eq(col("c3_non_null"))
+ .and(col("c3_non_null").eq(lit(5_i64)));
+ let expected = lit(5_i64).eq(col("c3_non_null"));
+ assert_eq!(simplify_no_canonicalize(expr), expected);
+
+ // c3 = 5 OR 5 = c3 --> c3 = 5
+ let expr = col("c3_non_null")
+ .eq(lit(5_i64))
+ .or(lit(5_i64).eq(col("c3_non_null")));
+ let expected = col("c3_non_null").eq(lit(5_i64));
+ assert_eq!(simplify_no_canonicalize(expr), expected);
+
+ // (c3 = 5 AND c4 > 0) AND (5 = c3) --> c3 = 5 AND c4 > 0
+ let expr = col("c3_non_null")
+ .eq(lit(5_i64))
+ .and(col("c4_non_null").gt(lit(0_u32)))
+ .and(lit(5_i64).eq(col("c3_non_null")));
+ let expected = col("c3_non_null")
+ .eq(lit(5_i64))
+ .and(col("c4_non_null").gt(lit(0_u32)));
+ assert_eq!(simplify_no_canonicalize(expr), expected);
+ }
+
+ #[test]
+ fn test_simplify_swapped_operands_in_xor_no_canonicalize() {
+ // `expr_contains` guards the XOR rules, so
`delete_xor_in_complex_expr` has to
+ // use the same equality relation. Comparing structurally there while
the guard
+ // normalizes makes the rule rebuild its input and still report
`Transformed::yes`,
+ // which spins the simplifier until it hits the cycle limit.
+
+ // (c4 + c4_non_null) ^ (c4_non_null + c4) --> 0
+ let expr = bitwise_xor(
+ col("c4") + col("c4_non_null"),
+ col("c4_non_null") + col("c4"),
+ );
+ let (simplified, cycles) =
simplify_no_canonicalize_with_cycle_count(expr);
+ assert_eq!(simplified, lit(0_u32));
Review Comment:
This test currently locks in incorrect NULL behavior. c4 is nullable, so
when it is NULL, (c4 + c4_non_null) ^ (c4_non_null + c4) evaluates to NULL, but
the rule returns non-null 0. Could we add a non-null check to both complex XOR
rules, use non-null operands for this cycle regression, and add a nullable case
that remains unchanged?
--
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]
