Skip to content

[SPARK-9054] [SQL] Rename RowOrdering to InterpretedOrdering; use newOrdering in SMJ #7973

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

Jump to bottom
Closed
wants to merge 3 commits into from
Closed

[SPARK-9054] [SQL] Rename RowOrdering to InterpretedOrdering; use newOrdering in SMJ #7973

wants to merge 3 commits into from

Conversation

@JoshRosen
Copy link
Contributor

@JoshRosen JoshRosen commented Aug 5, 2015

This patches renames RowOrdering to InterpretedOrdering and updates SortMergeJoin to use the SparkPlan methods for constructing its ordering so that it may benefit from codegen.

This is an updated version of #7408.

@JoshRosen
Copy link
Contributor Author

JoshRosen commented Aug 5, 2015

/cc @rxin @yhuai, this is needed to fix an issue that I found in my SMJ patch.

@JoshRosen JoshRosen mentioned this pull request Aug 5, 2015
Closed
yhuai
yhuai reviewed Aug 5, 2015
View reviewed changes
sql/core/src/main/scala/org/apache/spark/sql/execution/joins/SortMergeJoin.scala
Copy link
Contributor

@yhuai yhuai Aug 5, 2015

Choose a reason for hiding this comment

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

How about we at a comment at https://github.com/apache/spark/pull/7973/files#diff-b669f8cf35f1d2d786582f4d8c49ed14R59 to explain the direction must be Ascending. So, the direction in requiredOrders is consistent with the direction we are using at here.

Copy link
Contributor Author

@JoshRosen JoshRosen Aug 5, 2015

Choose a reason for hiding this comment

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

Will do.

Copy link
Contributor Author

@JoshRosen JoshRosen Aug 5, 2015

Choose a reason for hiding this comment

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

Done.

yhuai
yhuai reviewed Aug 5, 2015
View reviewed changes
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/ordering.scala
Copy link
Contributor

@yhuai yhuai Aug 5, 2015

Choose a reason for hiding this comment

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

Should this name also reflect its using Ascending?

Copy link
Contributor

@yhuai yhuai Aug 5, 2015

Choose a reason for hiding this comment

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

(super minor)

Copy link
Contributor Author

@JoshRosen JoshRosen Aug 5, 2015

Choose a reason for hiding this comment

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

Meh, it could. I only left it as-is out of a desire to minimize changes. Can fix later.

Copy link
Contributor

@yhuai yhuai Aug 5, 2015

Choose a reason for hiding this comment

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

yeah, it is totally fine to leave it as-is.

@yhuai
Copy link
Contributor

yhuai commented Aug 5, 2015

LGTM

@SparkQA
Copy link

SparkQA commented Aug 5, 2015

Test build #39915 has finished for PR 7973 at commit 34b8e0c.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Aug 5, 2015

Test build #39918 has finished for PR 7973 at commit e610655.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@JoshRosen
Copy link
Contributor Author

JoshRosen commented Aug 5, 2015

This passed on an earlier run and only failed the latest run due to a known flaky test, so I'm going to merge this to master and branch-1.5 in order to unblock work on my other patch.

asfgit pushed a commit that referenced this pull request Aug 5, 2015
@JoshRosen
[SPARK-9054] [SQL] Rename RowOrdering to InterpretedOrdering; use new…
618dc63 
…Ordering in SMJ

This patches renames `RowOrdering` to `InterpretedOrdering` and updates SortMergeJoin to use the `SparkPlan` methods for constructing its ordering so that it may benefit from codegen.

This is an updated version of #7408.

Author: Josh Rosen <[email protected]>

Closes #7973 from JoshRosen/SPARK-9054 and squashes the following commits:

e610655 [Josh Rosen] Add comment RE: Ascending ordering
34b8e0c [Josh Rosen] Import ordering
be19a0f [Josh Rosen] [SPARK-9054] [SQL] Rename RowOrdering to InterpretedOrdering; use newOrdering in more places.

(cherry picked from commit 9c87892)
Signed-off-by: Josh Rosen <[email protected]>
@asfgit asfgit closed this in 9c87892 Aug 5, 2015
@JoshRosen JoshRosen deleted the SPARK-9054 branch August 29, 2016 19:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@JoshRosen @yhuai @SparkQA