-
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 5 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 |
|---|---|---|
|
|
@@ -167,7 +167,28 @@ 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 ExpressionContainmentOrdering) | ||
| } | ||
|
|
||
| /** | ||
| * Orders [Expression] by parent/child relations. The child expression is smaller | ||
| * 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. Note that | ||
| * this is not for general expression ordering. For example, two irrelevant expressions | ||
| * will be considered as e1 < e2 and e2 < e1 by this ordering. But for the usage here, | ||
| * the order of irrelevant expressions does not matter. | ||
| */ | ||
| class ExpressionContainmentOrdering 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
|
||
| } | ||
| } | ||
| } | ||
|
|
||
| /** | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.