Skip to content

[SPARK-46632][SQL] Fix subexpression elimination when equivalent ternary expressions have different children #46135

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
zml1206 wants to merge 1 commit into from
Closed

[SPARK-46632][SQL] Fix subexpression elimination when equivalent ternary expressions have different children #46135

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 0 additions & 4 deletions ...lyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/EquivalentExpressions.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,10 +80,6 @@ class EquivalentExpressions(
case _ =>
if (useCount > 0) {
map.put(wrapper, ExpressionStats(expr)(useCount))
} else {
// Should not happen
throw SparkException.internalError(
s"Cannot update expression: $expr in map: $map with use count: $useCount")
}
false
}
Expand Down
12 changes: 12 additions & 0 deletions .../test/scala/org/apache/spark/sql/catalyst/expressions/SubexpressionEliminationSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -494,6 +494,18 @@ class SubexpressionEliminationSuite extends SparkFunSuite with ExpressionEvalHel
checkShortcut(Or(equal, Literal(true)), 1)
checkShortcut(Not(And(equal, Literal(false))), 1)
}

test("Equivalent ternary expressions have different children") {
Copy link
Contributor

@peter-toth peter-toth Apr 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, this is because ((1 + 2) + 3) semanticEquals to ((3 + 1) + 2) but their children are different. So when we compute the common elements between the above 2 here:

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

Lines 112 to 121 in 0d553d0

var localEquivalenceMap = mutable.HashMap.empty[ExpressionEquals, ExpressionStats]
updateExprTree(exprs.head, localEquivalenceMap)
exprs.tail.foreach { expr =>
val otherLocalEquivalenceMap = mutable.HashMap.empty[ExpressionEquals, ExpressionStats]
updateExprTree(expr, otherLocalEquivalenceMap)
localEquivalenceMap = localEquivalenceMap.filter { case (key, _) =>
otherLocalEquivalenceMap.contains(key)
}
}

only ((1 + 2) + 3) remains in localEquivalenceMap but it's children don't. So later when we want to remove ((1 + 2) + 3) we can't find its children...

I think we could fix the above code that computes localEquivalenceMap, but the change in PR is simpler and doesn't seem to do any harm.

val add1 = Add(Add(Literal(1), Literal(2)), Literal(3))
val add2 = Add(Add(Literal(3), Literal(1)), Literal(2))
val conditions1 = (GreaterThan(add1, Literal(3)), Literal(1)) ::
(GreaterThan(add2, Literal(0)), Literal(2)) :: Nil

val caseWhenExpr1 = CaseWhen(conditions1, Literal(0))
val equivalence1 = new EquivalentExpressions
equivalence1.addExprTree(caseWhenExpr1)
assert(equivalence1.getCommonSubexpressions.size == 1)
}
}

case class CodegenFallbackExpression(child: Expression)
Expand Down