Fix a bug in semanticEqual. Add some comment.

Author: codingjaguar

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Expression.scala

@@ -162,7 +162,7 @@ abstract class Expression extends TreeNode[Expression]{
 
   /**
    * Returns a sequence of expressions by removing from q the first expression that is semantically
-   * equivalent to e.
+   * equivalent to e. If such an expression was not found, return seq.
    */
   def removeFirstSemanticEquivalent(seq: Seq[Expression], e: Expression): Seq[Expression] = {
     seq match {

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/predicates.scala

@@ -261,8 +261,17 @@ case class And(left: Expression, right: Expression) extends BinaryOperator
     // Recursively call semanticEquals on subexpressions to check the equivalency of two seqs.
     var elements1 = splitConjunctivePredicates(this)
     val elements2 = splitConjunctivePredicates(other)
-    for (e <- elements2) elements1 = removeFirstSemanticEquivalent(elements1, e)
-    elements1.isEmpty
+    // We can recursively call semanticEquals to check the equivalency for subexpressions, but
+    // there is no simple solution to compare the equivalency of sequence of expressions.
+    // Expression class doesn't have order, so we couldn't sort them. We can neither use
+    // set comparison as Set doesn't support custom compare function, which is semanticEquals.
+    // To check the equivalency of elements1 and elements2, we first compare their size. Then
+    // for each element in elements2, we remove its first semantically equivalent expression from
+    // elements1. If they are semantically equivalent, elements1 should be empty at the end.
+    elements1.size == elements2.size && {
+      for (e <- elements2) elements1 = removeFirstSemanticEquivalent(elements1, e)
+      elements1.isEmpty
+    }
   }
 
   override def genCode(ctx: CodeGenContext, ev: GeneratedExpressionCode): String = {
@@ -319,12 +328,20 @@ case class Or(left: Expression, right: Expression) extends BinaryOperator
     // Non-deterministic expressions cannot be semantic equal
     if (!deterministic || !other.deterministic) return false
 
-    // we know both expressions are Or, so we can tolerate ordering different
-    // Recursively call semanticEquals on subexpressions to check the equivalency of two seqs.
+    // We know both expressions are Or, so we can tolerate ordering different
     var elements1 = splitDisjunctivePredicates(this)
     val elements2 = splitDisjunctivePredicates(other)
-    for (e <- elements2) elements1 = removeFirstSemanticEquivalent(elements1, e)
-    elements1.isEmpty
+    // We can recursively call semanticEquals to check the equivalency for subexpressions, but
+    // there is no simple solution to compare the equivalency of sequence of expressions.
+    // Expression class doesn't have order, so we couldn't sort them. We can neither use
+    // set comparison as Set doesn't support custom compare function, which is semanticEquals.
+    // To check the equivalency of elements1 and elements2, we first compare their size. Then
+    // for each element in elements2, we remove its first semantically equivalent expression from
+    // elements1. If they are semantically equivalent, elements1 should be empty at the end.
+    elements1.size == elements2.size && {
+      for (e <- elements2) elements1 = removeFirstSemanticEquivalent(elements1, e)
+      elements1.isEmpty
+    }
   }
 
   override def genCode(ctx: CodeGenContext, ev: GeneratedExpressionCode): String = {