-
Notifications
You must be signed in to change notification settings - Fork 29k
[SPARK-35439][SQL] Children subexpr should come first than parent subexpr #32586
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.
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
Changes from 3 commits
7b6b589
f777855
71b67cb
55bc9ec
c143ce2
b517df8
b13bc65
3819bf3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -42,7 +42,7 @@ class EquivalentExpressions { | |
| } | ||
|
|
||
| // For each expression, the set of equivalent expressions. | ||
| private val equivalenceMap = mutable.HashMap.empty[Expr, mutable.ArrayBuffer[Expression]] | ||
| private val equivalenceMap = mutable.LinkedHashMap.empty[Expr, mutable.ArrayBuffer[Expression]] | ||
|
|
||
| /** | ||
| * Adds each expression to this data structure, grouping them with existing equivalent | ||
|
|
@@ -167,7 +167,25 @@ class EquivalentExpressions { | |
| * Returns all the equivalent sets of expressions. | ||
| */ | ||
| def getAllEquivalentExprs: Seq[Seq[Expression]] = { | ||
| equivalenceMap.values.map(_.toSeq).toSeq | ||
| equivalenceMap.values.map(_.toSeq).toSeq.sortBy(_.head)(new ExpressionOrdering) | ||
| } | ||
|
|
||
| /** | ||
| * Orders [Expression] by parent/child relations. The child expression is smaller | ||
viirya marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| * than parent expression. If there is child-parent relationships among the subexpressions, | ||
| * we want the child expressions come first than parent expressions, so we can replace | ||
| * child expressions in parent expressions with subexpression evaluation. | ||
| */ | ||
| class ExpressionOrdering extends Ordering[Expression] { | ||
| override def compare(x: Expression, y: Expression): Int = { | ||
| if (x.semanticEquals(y)) { | ||
| 0 | ||
| } else if (x.find(_.semanticEquals(y)).isDefined) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we run
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok. Let me compare before/after this PR.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. BTW, I think better approach is to sort after filter (e.g. size > 1 in most use-case), because the number of sub-exprs should be smaller.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I changed the call usage of
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ran Before (master): After: I don't see significant difference there. |
||
| 1 | ||
| } else { | ||
| -1 | ||
dongjoon-hyun marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
| } | ||
| } | ||
|
|
||
| /** | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
after the new change, does it still need to be
LinkedHashMap?Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea, can be reverted back to HashMap, if we are going to sort it at all.