Skip to content

Conversation

@JoshRosen
Copy link
Contributor

SPARK-8317 changed the SQL Exchange operator so that it no longer pushed sorting into Spark's shuffle layer, a change which allowed more efficient SQL-specific sorters to be used.

This patch performs some leftover cleanup based on those changes:

  • Exchange's constructor should no longer accept a newOrdering since it's no longer used and no longer works as expected.
  • addOperatorsIfNecessary looked at shuffle input's output ordering to decide whether to sort, but this is the wrong node to be examining: it needs to look at whether the post-shuffle node has the right ordering, since shuffling will not preserve row orderings. Thanks to @davies for spotting this.

@JoshRosen
Copy link
Contributor Author

/cc @yhuai and @davies for review; this should be a pretty straightforward cleanup patch.

@yhuai
Copy link
Contributor

yhuai commented Jul 14, 2015

LGTM

1 similar comment
@davies
Copy link
Contributor

davies commented Jul 14, 2015

LGTM

@SparkQA
Copy link

SparkQA commented Jul 15, 2015

Test build #37283 has finished for PR 7407 at commit e70be50.

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

@SparkQA
Copy link

SparkQA commented Jul 15, 2015

Test build #37281 has finished for PR 7407 at commit e866494.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • case class Exchange(newPartitioning: Partitioning, child: SparkPlan) extends UnaryNode

@JoshRosen
Copy link
Contributor Author

Alright, I'm going to merge this into master. Thanks for looking this over!

@asfgit asfgit closed this in cc57d70 Jul 15, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants