[SPARK-17271] [SQL] Planner adds un-necessary Sort even if child orde… #14920
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ring is semantically same as required ordering
|
ok to test |
|
LGTM - pending jenkins. |
|
Test build #64786 has finished for PR 14920 at commit
|
asfgit
pushed a commit
that referenced
this pull request
Sep 1, 2016
## What changes were proposed in this pull request? Ports #14841 and #14910 from `master` to `branch-2.0` Jira : https://issues.apache.org/jira/browse/SPARK-17271 Planner is adding un-needed SORT operation due to bug in the way comparison for `SortOrder` is done at https://github.com/apache/spark/blob/master/sql/core/src/main/scala/org/apache/spark/sql/execution/exchange/EnsureRequirements.scala#L253 `SortOrder` needs to be compared semantically because `Expression` within two `SortOrder` can be "semantically equal" but not literally equal objects. eg. In case of `sql("SELECT * FROM table1 a JOIN table2 b ON a.col1=b.col1")` Expression in required SortOrder: ``` AttributeReference( name = "col1", dataType = LongType, nullable = false ) (exprId = exprId, qualifier = Some("a") ) ``` Expression in child SortOrder: ``` AttributeReference( name = "col1", dataType = LongType, nullable = false ) (exprId = exprId) ``` Notice that the output column has a qualifier but the child attribute does not but the inherent expression is the same and hence in this case we can say that the child satisfies the required sort order. This PR includes following changes: - Added a `semanticEquals` method to `SortOrder` so that it can compare underlying child expressions semantically (and not using default Object.equals) - Fixed `EnsureRequirements` to use semantic comparison of SortOrder ## How was this patch tested? - Added a test case to `PlannerSuite`. Ran rest tests in `PlannerSuite` Author: Tejas Patil <tejasp@fb.com> Closes #14920 from tejasapatil/SPARK-17271_2.0_port.
|
Merging to branch-2.0. Thanks for the backport! Could you close the PR (the merge script can only close PRs against master)? |
|
Thanks @hvanhovell !! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
Ports #14841 and #14910 from
mastertobranch-2.0Jira : https://issues.apache.org/jira/browse/SPARK-17271
Planner is adding un-needed SORT operation due to bug in the way comparison for
SortOrderis done at https://github.com/apache/spark/blob/master/sql/core/src/main/scala/org/apache/spark/sql/execution/exchange/EnsureRequirements.scala#L253SortOrderneeds to be compared semantically becauseExpressionwithin twoSortOrdercan be "semantically equal" but not literally equal objects.eg. In case of
sql("SELECT * FROM table1 a JOIN table2 b ON a.col1=b.col1")Expression in required SortOrder:
Expression in child SortOrder:
Notice that the output column has a qualifier but the child attribute does not but the inherent expression is the same and hence in this case we can say that the child satisfies the required sort order.
This PR includes following changes:
semanticEqualsmethod toSortOrderso that it can compare underlying child expressions semantically (and not using default Object.equals)EnsureRequirementsto use semantic comparison of SortOrderHow was this patch tested?
PlannerSuite. Ran rest tests inPlannerSuite