Repository: tobymao/sqlglot
Pull request: fix(optimizer): Remove XOR from connector simplifications
Fixes #3372

The `simplify` rule aims to simplify `AND` & `OR` connectors by removing duplicate exprs such as `A OR A -> A` but fails to consider boolean `XOR` (supported by MySQL) which has different semantics i.e for `A = True`, `A XOR A = False`

Problem statement:
XOR transformed into an OR depending on the alphabetical order of operands
Hello,

I stumbled upon an apparent bug still present in the 23.12.1 release (I was using 20.9.0 when I first saw the bug). When using the `optimize` function on a MySQL constraint, result depends on the alphabetical order of the operands (with one of the results being wrong):

```python
from sqlglot import parse_one
from sqlglot.optimizer import optimize

# Returns '`bar` IS NULL XOR `foo` IS NULL'
optimize(parse_one('(bar IS NULL) XOR (foo IS NULL)', read="mysql")).sql("mysql")

# Returns '`bar` IS NULL OR `foo` IS NULL'
optimize(parse_one('(foo IS NULL) XOR (bar IS NULL)', read="mysql")).sql("mysql")
```

In the second example, the XOR is transformed into an OR which in this case does not seem to be equivalent. I would expect both results to be the same (or at least not converting the XOR into OR).

Is there something wrong in the way I used those functions or is this a bug ?

I can give more details if needed !
Thank you

Hints from issue discussion:
This look like a bug, thanks for reporting. We'll take a look soon.

Commit message: fix(optimizer): Remove XOR from connector simplifications

Patch to review:
diff --git a/sqlglot/expressions.py b/sqlglot/expressions.py
index 862b3f7a01..d62b463ee3 100644
--- a/sqlglot/expressions.py
+++ b/sqlglot/expressions.py
@@ -6563,6 +6563,29 @@ def or_(
     return t.cast(Condition, _combine(expressions, Or, dialect, copy=copy, **opts))
 
 
+def xor_(
+    *expressions: t.Optional[ExpOrStr], dialect: DialectType = None, copy: bool = True, **opts
+) -> Condition:
+    """
+    Combine multiple conditions with an XOR logical operator.
+
+    Example:
+        >>> xor_("x=1", xor_("y=1", "z=1")).sql()
+        'x = 1 XOR (y = 1 XOR z = 1)'
+
+    Args:
+        *expressions: the SQL code strings to parse.
+            If an Expression instance is passed, this is used as-is.
+        dialect: the dialect used to parse the input expression.
+        copy: whether to copy `expressions` (only applies to Expressions).
+        **opts: other options to use to parse the input expressions.
+
+    Returns:
+        Xor: the new condition
+    """
+    return t.cast(Condition, _combine(expressions, Xor, dialect, copy=copy, **opts))
+
+
 def not_(expression: ExpOrStr, dialect: DialectType = None, copy: bool = True, **opts) -> Not:
     """
     Wrap a condition with a NOT operator.
diff --git a/sqlglot/optimizer/simplify.py b/sqlglot/optimizer/simplify.py
index 99105721bc..65b11d025f 100644
--- a/sqlglot/optimizer/simplify.py
+++ b/sqlglot/optimizer/simplify.py
@@ -223,7 +223,7 @@ def flatten(expression):
 
 def simplify_connectors(expression, root=True):
     def _simplify_connectors(expression, left, right):
-        if left == right:
+        if left == right and not isinstance(expression, exp.Xor):
             return left
         if isinstance(expression, exp.And):
             if is_false(left) or is_false(right):
@@ -365,10 +365,17 @@ def uniq_sort(expression, root=True):
     C AND A AND B AND B -> A AND B AND C
     """
     if isinstance(expression, exp.Connector) and (root or not expression.same_parent):
-        result_func = exp.and_ if isinstance(expression, exp.And) else exp.or_
         flattened = tuple(expression.flatten())
-        deduped = {gen(e): e for e in flattened}
-        arr = tuple(deduped.items())
+
+        if isinstance(expression, exp.Xor):
+            result_func = exp.xor_
+            # Do not deduplicate XOR as A XOR A != A if A == True
+            deduped = None
+            arr = tuple([(gen(e), e) for e in flattened])
+        else:
+            result_func = exp.and_ if isinstance(expression, exp.And) else exp.or_
+            deduped = {gen(e): e for e in flattened}
+            arr = tuple(deduped.items())
 
         # check if the operands are already sorted, if not sort them
         # A AND C AND B -> A AND B AND C
@@ -378,7 +385,7 @@ def uniq_sort(expression, root=True):
                 break
         else:
             # we didn't have to sort but maybe we need to dedup
-            if len(deduped) < len(flattened):
+            if deduped and len(deduped) < len(flattened):
                 expression = result_func(*deduped.values(), copy=False)
 
     return expression
diff --git a/tests/fixtures/optimizer/simplify.sql b/tests/fixtures/optimizer/simplify.sql
index 6af51bf9e3..6aff8c6b66 100644
--- a/tests/fixtures/optimizer/simplify.sql
+++ b/tests/fixtures/optimizer/simplify.sql
@@ -232,6 +232,9 @@ x - 1;
 A AND D AND B AND E AND F AND G AND E AND A;
 A AND B AND D AND E AND F AND G;
 
+A OR D OR B OR E OR F OR G OR E OR A;
+A OR B OR D OR E OR F OR G;
+
 A AND NOT B AND C AND B;
 FALSE;
 
diff --git a/tests/test_optimizer.py b/tests/test_optimizer.py
index 758b60c5a2..6d0b73488e 100644
--- a/tests/test_optimizer.py
+++ b/tests/test_optimizer.py
@@ -427,6 +427,12 @@ def test_simplify(self):
 """.strip(),
         )
 
+        xor_query = parse_one("A XOR D XOR B XOR E XOR F XOR G XOR E XOR A", read="mysql")
+        simplified_xor = optimizer.simplify.simplify(xor_query)
+        self.assertEqual(
+            simplified_xor.sql(dialect="mysql"), "A XOR A XOR B XOR D XOR E XOR E XOR F XOR G"
+        )
+
     def test_unnest_subqueries(self):
         self.check_file(
             "unnest_subqueries",


Code review comments:
diff_hunk: @@ -427,6 +427,12 @@ def test_simplify(self):
 """.strip(),
         )
 
+        xor_query = parse_one("A XOR D XOR B XOR E XOR F XOR G XOR E XOR A", read="mysql")
+        simplified_xor = optimizer.simplify.simplify(xor_query)
+        self.assertEqual(
+            simplified_xor.sql(dialect="mysql"), "A XOR A XOR B XOR D XOR E XOR E XOR F XOR G"
path: tests/test_optimizer.py
text: @user1:
Can't we simplify this further? Eg. A XOR A is always false.

diff_hunk: @@ -6563,6 +6563,29 @@ def or_(
     return t.cast(Condition, _combine(expressions, Or, dialect, copy=copy, **opts))
 
 
+def xor_(
path: sqlglot/expressions.py
text: @user1:
The underscore is used eg. in `or_` because `or` is a reserved keyword

```suggestion
def xor(
```