-
Notifications
You must be signed in to change notification settings - Fork 29k
[SPARK-35439][SQL][FOLLOWUP] ExpressionContainmentOrdering should not sort unrelated expressions #32870
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
[SPARK-35439][SQL][FOLLOWUP] ExpressionContainmentOrdering should not sort unrelated expressions #32870
Changes from all commits
562ab33
c27fbfc
308cd91
1f8e7e2
019faba
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 |
|---|---|---|
|
|
@@ -193,27 +193,6 @@ class EquivalentExpressions { | |
| .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) { | ||
| 1 | ||
| } else { | ||
| -1 | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Returns the state of the data structure as a string. If `all` is false, skips sets of | ||
| * equivalent expressions with cardinality 1. | ||
|
|
@@ -229,3 +208,27 @@ class EquivalentExpressions { | |
| sb.toString() | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * 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 or semantically-equal | ||
| * expressions will be considered as equal by this ordering. But for the usage here, the order of | ||
| * irrelevant expressions does not matter. | ||
| */ | ||
| class ExpressionContainmentOrdering extends Ordering[Expression] { | ||
|
Member
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. Just curious, is there a reason of this move?
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. Oh, as it is a nested class, I cannot allocate it separately, but val equivalence = new EquivalentExpressions
val exprOrdering = new equivalence.ExpressionContainmentOrderingI can revert to nested class if you think it's unnecessary change.
Member
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. Never mind. New one also looks good~ |
||
| override def compare(x: Expression, y: Expression): Int = { | ||
| if (x.find(_.semanticEquals(y)).isDefined) { | ||
| // `y` is child expression of `x`. | ||
| 1 | ||
| } else if (y.find(_.semanticEquals(x)).isDefined) { | ||
| // `x` is child expression of `y`. | ||
| -1 | ||
| } else { | ||
| // Irrelevant or semantically-equal expressions | ||
| 0 | ||
| } | ||
| } | ||
| } | ||
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.
To be complete, could you add some description about the semantically-equal expressions?
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.
Sure. Added.